Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NO SQUASH] Add hypertext support to tooltips #14728

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Wuzzy2
Copy link
Contributor

@Wuzzy2 Wuzzy2 commented Jun 4, 2024

Background of this PR

I was asked by @kilbith to submit this PR, who did the work for the first commit. @kilbith is unable to do it alone because of a ban. I have looked at the new feature and tested it myself, checked the code and it looks OK. I believe it will be like a very good addition to Minetest, but it needs work. Please read the code carefully yourself before running it.

I do not know if our policies allow a PR to be made like this. If you dislike what I try to do here, I feel very sorry. Just close this PR and it's over. But if this is OK:

We have explicit, written permission to use this work under LGPLv2.1+. Quote from e-mail conversation with @kilbith:

  1. Permission is expressly granted by my organization to license this code
    under the GNU LGPL v2.1+ terms.

This e-mail has been cc'ed to @celeron55 as well.

IMPORTANT: DO NOT SQUASH! The first commit MUST be left unaltered, which contains the requested author identity. I was given the patch only under this condition, to which I agreed. The agreement allows to add new commits to build on the initial commit tho. So bugfixes and changes all have to go in NEW commits.

Feature description

This feature will add hypertext support for tooltips by introducing a new supertip tooltip element. This element works like tooltip but you can write hypertext (like for the hypertext element).

There are two modes: dynamic and static. Dynamic supertips will show up next to the cursor, just like normal ones. Static supertips appear at a fixed formspec coordinate.

Also, supertips can be styled.

See the lua_api.md for details.

Demo

Here is a video demonstration:

https://www.youtube.com/watch?v=RAASW_Rj8eQ

In the 2nd commit, I have added a simple text/example in DevTest in the /testformspec command (new "Tooltip" tab).

Known bugs

This PR is a DRAFT. Expect more commits to come.

There are various known bugs at the moment:

  • [FIXED!] SEGFAULT if a mod makes a syntax error in a supertip (like a wrong argument count)
  • [FIXED!] Dynamic supertips may appear FAR away from the cursor, even out of window (depends on window size)
  • At small window sizes, the text of a supertip might be cut off
  • [FIXED!] When you hover a supertip the first time, it can be very large for a split second (may need multiple tries to reproduce)
  • [FIXED!] The supertip on a button does not appear with a delay on the 2nd time you hover the button, unlike the normal tooltip

@kilbith has suggested that no further work on this can be expected. This means it's now up to me to fix the rest. Help is appreciated.

Feedback

This PR is a draft, but feedback of the concept is greatly appreciated. Especially please comment on the syntax and design choices.

Also report any bugs I didn't find myself before.

@nerzhul
Copy link
Member

nerzhul commented Jun 5, 2024

it sounds to be a very good enhancement.
A question about this, cannot we just have our current tip code using this new parsing method ?
Is supertip the good name ?
If it's a new UI addition, don't we need a protocol bump ? what happen if a server uses it and the client is older ?

src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
@nerzhul
Copy link
Member

nerzhul commented Jun 5, 2024

another comment, can we have multiple examples in devtest ?

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jun 5, 2024

Well, all these things are open for debate. I haven't deeply analyzed the code myself yet, only read it twice. The only thing that matters is that the 1st commit remains intact.

I agree that supertip might not be the ideal name, but I'm neutral about this. I don't know why @kilbith chose to add a new formspec element rather than extending tooltip. But I can think of some reasons myself: Keeping the default tooltip simple and for compability. Also, please keep in mind that the supertip syntax will parse hypertext, so < and > have a special meaning. While the default tooltip does not give them special treatment. We cannot make tooltip suddenly parse hypertext, it will break tooltips that already use < and > and other special characters as plain text.

Although a new param could be added to toggle hypertext parsing. And it is off by default.

supertip does other things as well: It is also stylable and allows absolute positions.

But yeah, thinking about it, it MAY be possible to extend the tooltip element instead, the hard part is to make sure it'll all be backwards-compatible and not making the new syntax too messy. Annoyingly, for this to work, I'm afraid we'd have to add the new parameters after the text. But as long it works …

Another open question is whether this will require a new formspec version. I don't the rules for that.

As I understand the code, it does not currently handle the case of an old client receiving a supertip. This was not tested yet, so assume the worst. :D

Anyway. I think the top priority for me is to fix all the bugs first to get it to work properly. Optimization/cleanup can come later.

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jun 5, 2024

There are multiple exampels in DevTest, in the /test_formspec command. Or did you mean the current examples are not enough? If so, I agree. Adding more DevTest examples is on my TODO list. Currently missing is an example for styling.

About this code:

/* Issue with floating tooltips' positioning here.
Set hardcoded values that only works on 16:10/4K displays. */

@kilbith told me this is a workaround for a known bug, to "force" the feature to work at all. The bug here is that the supertip is not positioned correctly if it's dynamic. It is often placed far away from the cursor, changing with screen size. So what needs to be done is to fix the bug, making sure dynamic supertips appear correctly at the cursor. (dynamic supertip = tooltip that appears relative to the cursor, like the default tooltip)

@nerzhul
Copy link
Member

nerzhul commented Jun 5, 2024

for the exteneded parsing, we can imagine extended=true attribute in tooltip directly, switching the parsing mode maybe ? that'll permit to extend it ? you seems to agree with me on this

@nerzhul
Copy link
Member

nerzhul commented Jun 5, 2024

for the devtest part, ignore my comment, i just skipped the file in my brain, it's ok :D

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jun 6, 2024

Progress report: Dynamic supertips (i.e. supertips that follow the cursor like normal tooltips) now have the correct positioning. I also changed the syntax. The posX,posY argument is now called staticPos and is mandatory, meaning the element now has a fixed argument count. This should make things clearer and easier to understand. I also fixed a segfault when this argument had invalid syntax.

2 known bugs remain (see above).

for the exteneded parsing, we can imagine extended=true attribute in tooltip directly, switching the parsing mode maybe ?

Yeah, I was thinking about something like thit. I could probably work, I just need to figure out the proper syntax while maintaining compat.

Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts:

  • "hypertext + tooltip = supertip"? Not rather "hypertip"?

  • I feel like having a new element for hypertext tooltips, instead of reusing the existing tooltip element by adding new parameters at the end, is less confusing. There's also the fontcolor and bgcolor parameters for regular tooltips, which wouldn't make much sense for hypertext tooltips.

  • Compared to regular tooltips, hypertext tooltips seem to miss the feature where you can "attach" them to formspec elements. It might make sense to support this for consistency.

  • Not required in this PR, but I think it would be great if item (metadata) tooltips could also optionally use hypertext.

@rubenwardy
Copy link
Member

I recommend the obvious name: tooltip_hypertext

src/gui/guiHyperText.cpp Show resolved Hide resolved
@@ -177,6 +179,8 @@ class TextDrawer
inline s32 getHeight() { return m_height; };
void draw(const core::rect<s32> &clip_rect,
const core::position2d<s32> &dest_offset);
void drawBackgroundImage(video::IVideoDriver *driver, const ParsedText &m_text, const core::rect<s32> &clip_rect);
ParsedText &getText() { return m_text; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't like this very dangerous pattern, it's better to have a copy and a setting i think. ok it's optimized memory side, but having full access to a member ref sounds bad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants