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

Formspec: Add HTML-like element #8680

Open
wants to merge 3 commits into
base: master
from

Conversation

@kilbith
Copy link
Contributor

commented Jul 15, 2019

This PR offered by Kidscode allows to format a formspec content almost like HTML.

Demo video: https://www.youtube.com/watch?v=_AtifpDQ-5M

Demo mod: https://paste.ubuntu.com/p/Nf6XfXnSZX/


screenshot_20190715_163150

@kilbith kilbith changed the title Formspec: Add HTML-like text[] element Formspec: Add HTML-like element Jul 15, 2019
@kilbith kilbith force-pushed the EvidenceBKidscode:backport-html branch 2 times, most recently from 5b30781 to 79ff06a Jul 15, 2019
@v-rob

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

This looks pretty nice from the screenshot and video, but I have a few suggestions and questions:

Questions:

  • Does it collapse multiple whitespaces into one? (I hope it doesn't, since that is so annoying IMO.)
  • Are the width and height parameters in pixels or coordinates? Since this is scalable, it should probably be in coordinates.
  • In the <style> tag, when setting the font size, is the value the height in pixels or is it relative to the current size?

And a few suggestions:

  • I noticed that margins and things like that are in pixels. Since formspecs are scalable, I think that they should be coordinates
  • Other things should be relative like text size (if it isn't already) so that the form can fit everybody's screen size and settings.
  • Add support for real_coordinates in the text element, or just drop support for the old system and always use real_coordinates (which reduces code size, and the old system doesn't matter much any more since there's no practical use for it anymore.)
  • Use guiScrollBar instead of IGUIScrollBar since the former has a modifiable thumb height.
  • Perhaps rename it from text to something more descriptive, since this is much more than plain text. Perhaps something like markup, since that is more descriptive and self-explanatory?

I personally would have chosen a syntax less like HTML, but that's just personal preference, so you don't have to listen to that.

Anyway, that's just a few quick thoughts I had on the spot.

@SmallJoker SmallJoker added this to Highest priority on top in Formspec PR Priority List [POSSIBLE CONFICTS] Jul 15, 2019
@pyrollo

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

Hi v-rob,

* Does it collapse multiple whitespaces into one?  (I hope it doesn't, since that is so annoying IMO.)

No it does not. It also keeps new lines. Of course in "justify" mode, it adds some extra space to them.

* Are the `width` and `height` parameters in pixels or coordinates?  Since this is scalable, it should probably be in coordinates.

Now they are in irrlicht coordinates (I suppose they are pixels).

* In the `<style>` tag, when setting the font size, is the value the height in pixels or is it relative to the current size?

Same as above.

* I noticed that margins and things like that are in pixels.  Since formspecs are scalable, I think that they should be coordinates

Yes that's a good idea. I don't know if it's possible (element aware of scaling?). Nothing scales but the text is ajusted to fit. Like in other text displaying softwares. If you change window size, text is layed out again to fit but font size is not changed.

* Other things should be relative like text size (if it isn't already) so that the form can fit everybody's screen size and settings.

Same as above, char size is given as is to font APIs.

* Add support for `real_coordinates` in the `text` element, or just drop support for the old system and always use `real_coordinates` (which reduces code size, and the old system doesn't matter much any more since there's no practical use for it anymore.)

Yes, that's something we planed to do. We were waiting for you PR to be accepted (I guess it is done now).
I hope the "old" system will be abandoned. I took textarea as template because it's close to this new element.

* Use `guiScrollBar` instead of `IGUIScrollBar` since the former has a modifiable thumb height.

Ok!

* Perhaps rename it from `text` to something more descriptive, since this is much more than plain text.  Perhaps something like `markup`, since that is more descriptive and self-explanatory?

The name is always a problem, it has to be short and precise. We got through many names* and back to text even it's not satisfying. markup is short but it does not satisfy me neither.

I personally would have chosen a syntax less like HTML, but that's just personal preference, so you don't have to listen to that.

I would also. First I though of something like BBCode but it would have been a pain to escape all brackets not to conflict with formspecs (specially in short texts, long text are likely to use formspec_escape). BBCode would have been fine as it is aimed to be basic and simple.
Then I went to html/xml/svg/... syntax, not wanting to use an exotic syntax, even if it's somehow misleading. HTML is complex and huge, here we are in a much simpler and basic stufff.
But yeah, if we could have a syntax fitting more to our needs, I take it ! (actually if its made of opening/closing tags, it's easy to change)

Thanks for your feed back !

@v-rob

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

Thanks for getting back! Answering a few questions and adding a few more comments:

  • For the size of images, when I mean coordinates, I mean imgsize which is calculated for each formspec's size. So, for instance, line 415 could instead be

    int width = strtol(attrs["width"].c_str(), NULL, 10) * imgsize.X;

    You'd also have to pass imgsize to the constructor of GUIText, of course.

  • Yes, the old coordinate system is pretty much abandoned now. New elements really don't have to work with it anymore since that would add extra code for no real benefit.

  • For guiScrollBar, use setPageSize to set the height of the thumb.

  • I was also thinking BBCode for the syntax since it's easier to understand. I was trying to make something like this PR a while back, but it didn't work very well at all. During that, I came up with a syntax that was short and simple, very similar to BBCode. Here it is (although modified for this PR):

Tags

Note that, although I used [], <> could be used just as well. I prefer [] because it fits in with the formspec syntax better. I also left out the closing tags because it's just [/tagname].

[align=<left/right/center/justify>]: Aligns text

[action=<name>]: A link. I called it action because it doesn't necessarily have to work like a link; it could do a different action

[img=<name>;float=<left/right>,width=<#>,height=<#>]: Image. After semicolon are the optional arguments, separated by commas.

[item=<name>;float=<right/left>,width=<#>,height=<#>,rotate=<true/false>]: Item image

[color=<color>]: Set text color

[font=<mono/normal>]: Set font to monospace or back to normal

[size=<big/bigger/normal/#>: Set font size. Works as <big>, <bigger>, <normal>, and <style size=...> all in one.

[global;op1=<value>,op2=<value>]: Just like global, but with comma separated values.

I'm not very satisfied with img, item, or global since they all take multiple values, so I tried to make them as close to normal formspec syntax as possible. But I think the other tags are alright.

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

I prefer XML tags to bbcode.

Also, I think the element should be called hypertext[], as that's what it is

@pyrollo

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

This PR could be labelled as WIP, there are still many improvements that can be done.

Regarding the tags, I think we will keep xml style syntax.
We adopted the "action" tag name which is much better and clearer.
I think thinks should be kept simple and our PR is already too rich in term of styling.
I'm wondering if having just 3 sizes of chars, 2 styles (like it is yet) and a few automatic styles (actions, "bold" or "strong", and another one) would be sufficient. May be giving the possibility of choosing any font size would lead to a risk of having very heterogeneous formspecs. Anyway the full possibilities will be kept in C++ code for later use.

so tags could be :

  • Align tags : align or center / justify / right / left
  • Size tags : normal / big / bigger ?
  • action, img, item
  • font or mono / proportionnal (two possibilities)
  • b, i with color/style set in global
  • global : very convenient way to configure global stuff : action, action hover, b, i styles.

And that's it, keeping it simple for non programmers.

@SmallJoker SmallJoker added the WIP label Jul 17, 2019
@SmallJoker

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

You implemented an entire HTML-like parser and render (using Irrlicht ofc) in a thousand lines. Wow, that's impressive! It won't only improve the capabilities of wiki mods, but also make formspecs visually more attractive.
I see that this PR yet needs a few touches to complete, like real coordinates and rendering speed tests using large content (clipping content).

@Sokomine

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Please resolve the merge conflict with src/client/hud.cpp. I tried it in my local branch but am not sure if I did that right. Fixing that might attract a lot more testers.

The image of the furnace did not show up in my local client. Other than that, the test command worked fine and showed the formspec as displayed above. Hovering over a link did highlight it, and clicking on "using" displayed the help page as it ought to. Scrolling worked fine as well.

Even adding a classic formspec button to the new formspec is supported.

The font size seems to be independent of the font size set in minetest.conf. Perhaps this is a good idea. It may make formspec design easier as long as the fonts do not get too small.

I did have one big problem though: Opening the inventory formspec turned out to be a bad idea as I found no way to close it again. ESC did not work. I had to kill the client.

Inventory items, items in chests and images in unified_inventory where not shown. Perhaps that is related to the furnace not showing up and caused by my inproper merge.

Copy link
Contributor

left a comment

Great work on this PR! I've suggested some minor changes as follows.

doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.h Outdated Show resolved Hide resolved
src/gui/guiText.h Outdated Show resolved Hide resolved
Copy link
Member

left a comment

A very good feature. A little bit codestyle to fix and many performance issues to fix, this can be memory intensive (especially for android) on many parts

src/gui/guiText.cpp Outdated Show resolved Hide resolved
src/gui/guiText.cpp Outdated Show resolved Hide resolved
src/gui/guiText.cpp Outdated Show resolved Hide resolved
src/gui/guiText.cpp Outdated Show resolved Hide resolved
src/gui/guiText.cpp Outdated Show resolved Hide resolved
src/gui/guiText.cpp Outdated Show resolved Hide resolved
src/gui/guiText.cpp Outdated Show resolved Hide resolved
src/gui/guiText.cpp Outdated Show resolved Hide resolved
src/gui/guiText.cpp Outdated Show resolved Hide resolved
src/gui/guiText.cpp Outdated Show resolved Hide resolved
@Sokomine

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Later on, some limited support of tables would be nice. Sometimes - even in the example in the introduction - some texts require a tabular setup. This may be very difficult to implement and is a wish for further development once this got merged (which I hope will be possible soon!)

@paramat

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

See the output of the clang format build check for many valid codestyle errors.
Of course, many of clang format's suggestions are invalid or bad suggestions and shouldn't be implemented, it's fairly obvious which.

@kilbith kilbith force-pushed the EvidenceBKidscode:backport-html branch 5 times, most recently from 0cb0e31 to 866d9e0 Jul 31, 2019
@kilbith

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

  • text element renamed to hypertext
  • link becomes action
  • Fixed code style, bug with line height, bug with link hovering and item rotation now works
  • Changed tag management and added new tags
  • Demo mod updated accordingly
Copy link
Member

left a comment

Long review. The PR works, so I think the leftover is to make the code style perfect?

doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
src/gui/guiHyperText.cpp Outdated Show resolved Hide resolved
src/gui/guiHyperText.cpp Outdated Show resolved Hide resolved
src/gui/guiHyperText.cpp Outdated Show resolved Hide resolved
src/gui/guiHyperText.cpp Outdated Show resolved Hide resolved
src/gui/guiHyperText.h Outdated Show resolved Hide resolved
@kilbith kilbith force-pushed the EvidenceBKidscode:backport-html branch 4 times, most recently from a04a95f to da1d049 Aug 1, 2019
@Sokomine

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

Any progress here? I'm still hoping for better formspecs and a way to chat with mobs. This PR can do that. Please continue working on it and eventually merging!

@pyrollo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

Hi, some work has been done on our side but we have to prepare it for the PR.

Also, some recent changes of fontengine have to be taken in account.

@v-rob

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

I was thinking something: Is there any way to escape the tags? Compatibility might be broken with labels for user-inputted data since tags won't be escaped. Adding this functionality to minetest.formspec_escape wouldn't be a good idea because it would invalidly escape things for all elements, including those that don't use tags. I think a new function, minetest.hypertext_escape which runs minetest.formspec_escape and then escapes the tags would be a good idea.

@kilbith

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

I was thinking something: Is there any way to escape the tags?

Yes, see the demo mod (2nd formspec).

@pyrollo pyrollo force-pushed the EvidenceBKidscode:backport-html branch 5 times, most recently from 393f4d4 to 9e450d0 Sep 10, 2019
@pyrollo

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Hi all,
PR has been rebased and should be now ready for merging (WIP label can be removed).
No extra feature will be added before it is merged, no code change planned (except needed in order to merge).

@pyrollo pyrollo force-pushed the EvidenceBKidscode:backport-html branch from 9e450d0 to 1d26f88 Sep 11, 2019
Copy link
Member

left a comment

Look good

doc/lua_api.txt Outdated Show resolved Hide resolved
src/gui/guiHyperText.cpp Outdated Show resolved Hide resolved
src/gui/guiHyperText.cpp Outdated Show resolved Hide resolved
src/gui/guiHyperText.cpp Outdated Show resolved Hide resolved
@pyrollo pyrollo force-pushed the EvidenceBKidscode:backport-html branch from 1d26f88 to 9fcfd5a Sep 18, 2019
@pyrollo

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Hi Rubenwardy,

Changes have been made accordingly.

@pyrollo pyrollo force-pushed the EvidenceBKidscode:backport-html branch from 9fcfd5a to 1b5f69d Sep 19, 2019
@rubenwardy rubenwardy removed the WIP label Sep 19, 2019
@pyrollo pyrollo force-pushed the EvidenceBKidscode:backport-html branch from 1b5f69d to da28a15 Sep 30, 2019
@SmallJoker SmallJoker added this to the 5.2.0 milestone Oct 1, 2019
@kilbith kilbith referenced this pull request Oct 6, 2019
0 of 5 tasks complete
@Sokomine

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2019

Thanks for keeping working on this! I'm glad it has been added to 5.2.0. Hopefully it'll come then. I'm eagerly awaiting this vast improvement of the game.

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Oct 16, 2019

The testing code works, although this part is a bit misleading:

<action name="default:cobblestone">cobblestone</action>

It implies that spaces are possible within the name field, but it only accepts a single word.
Apart from that - the demo code works perfectly, and the callbacks as well. There's some space below the formspec without real coordinates but I don't think anyone cares about scaling there. It's an entire mess anyway.

From my POV this PR only needs a comment solved above, and a rebase.

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 16, 2019

Please add the files to the clang format whitelist https://github.com/minetest/minetest/blob/master/util/travis/clang-format-whitelist.txt to silence the build fail. Most of the suggestions made by clang format are (as usual) useless and make the code far less readable. Some of what it reports may be valid so i suggest looking through it first for any valid suggestions.

@kilbith kilbith force-pushed the EvidenceBKidscode:backport-html branch from da28a15 to 1549236 Oct 17, 2019
@SmallJoker

This comment has been minimized.

Copy link
Member

commented Oct 17, 2019

Started Minetest with freetype disabled:

Thread 1 "minetest" received signal SIGSEGV, Segmentation fault.
0x00005555556f1fcc in FontEngine::getFont(unsigned int, FontMode, bool, bool) ()
(gdb) bt
#0  0x00005555556f1fcc in FontEngine::getFont(unsigned int, FontMode, bool, bool) ()
#1  0x00005555556f30af in FontEngine::readSettings() ()
#2  0x00005555556f3d21 in FontEngine::FontEngine(Settings*, irr::gui::IGUIEnvironment*) ()
#3  0x00005555556a7e45 in ClientLauncher::run(GameParams&, Settings const&) ()

EDIT: No need for formatting there. Just re-use the normal and monospace fonts if freetype is not available.

@pyrollo

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2019

Thanks for the fixing! Introducing FontSpec is very clever :)

I cant find a way to mark "change requested" as done (they all seem to be outdated).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.