Skip to content

Conversation

@Unarelith
Copy link
Contributor

@Unarelith Unarelith commented Dec 13, 2018

I really like #5395 but it seems dead, so I decided to rebase it and to take it further.

@rubenwardy suggested changes, I made them, however these changes makes the styling "global" instead of per-inventory list.

listoptions[<spacing factor width>,<spacing factor height>;<img factor size>;<border_size>]

  • Set inventory list style

  • spacing factor width and spacing factor height are spacing factor sizes (relative to the slot size) of a slot
    example 1 : (0.0,0.0) for spacing means that there are no space between slots
    example 2 : (1.0,1.0) for spacing means that default space between slots is used
    example 3 : (2.0,2.0) for spacing means that default space between slots x2 is used

  • img factor size is the size factor (relative to the screen size) of a slot : a slot is always a square
    the default value for the img factor size is 1.0 (42 pixels with border included for a 800x600 screen size)
    example 1 : 1.0 for img factor size means that you use the default size for a slot
    example 2 : 2.0 for img factor size means that you use the default size x2 for a slot

  • border size is the size of the border of a slot (wasn't in the original PR)


Example 1

size[8,7.5;]
image[1,0.6;1,2;player.png]
listoptions[0,0;1;0]
list[current_player;main;0,3.5;8,4;]
list[current_player;craft;3,0;3,3;]
list[current_player;craftpreview;7,1;1,1;]

screenshot-20181214030139


Example 2

size[8,7.5;]
image[1,0.6;1,2;player.png]
listoptions[1,1;0.5;1]
list[current_player;main;0,3.5;8,4;]
list[current_player;craft;3,0;3,3;]
list[current_player;craftpreview;7,1;1,1;]

screenshot-20181214092130


Example 3

size[8,7.5;]
image[1,0.6;1,2;player.png]
listoptions[0,0;1;0]
list[current_player;main;0,3.5;8,4;]
listoptions[1,1;0.5;1]
list[current_player;craft;3,0;3,3;]
listoptions[1,1;1.2;1]
list[current_player;craftpreview;7,1;1,1;]

screenshot-20181214185411


@SmallJoker SmallJoker added @ Client / Audiovisuals Feature ✨ PRs that add or enhance a feature labels Dec 13, 2018
@Unarelith Unarelith force-pushed the formspec_list_style branch 2 times, most recently from bd5c13a to d2479a4 Compare December 13, 2018 21:17
@Unarelith Unarelith force-pushed the formspec_list_style branch 4 times, most recently from c3dfc10 to 70b850c Compare December 14, 2018 02:09
@Unarelith
Copy link
Contributor Author

Unarelith commented Dec 14, 2018

Ok this PR works properly now. I added an example and a screenshot.
Damn, I was so wrong, when I change imgsize the highlighting is still correct, but the selection with the mouse is definitely fucked up.
Finally I got it working, a small fix was missing from the original PR.

Though I really don't know how to set the options per-element without adding parameters to list[].

@rubenwardy
Copy link
Contributor

Actually, this is fine if it only affects lists registered after it. I forgot how table options worked

@Unarelith
Copy link
Contributor Author

Unarelith commented Dec 14, 2018

It doesn't work like that, these options are set on parsing and used at rendering, so the last listoptions[] wins for the entire formspec. I sincerely think that adding additional parameters to list[] isn't a big deal though.

@Unarelith
Copy link
Contributor Author

Unarelith commented Dec 14, 2018

@rubenwardy this PR has been tested and works now, though I agree that per-list definition would be great.

So I looked to tableoptions to see how it's implemented, unfortunately it's very different to list since tableoptions is used directly at parsing time to create GUITable instance. So I came up with two options:

  • A simple way would be to add listoptions data to parserData and then to store a copy in ItemSpec and ListDrawSpec when they're created.

  • Another option would be to add a new struct like ListOptionsSpec which would be stored together with ListDrawSpec and ItemSpec the same way FieldSpec is stored with GUITable* for table[] (with a std::pair)

Tell me which one you prefer and I'll implement it.

@Unarelith
Copy link
Contributor Author

Btw this PR is ready since last commit, I decided to implement the first option since I had no answer.

@rubenwardy
Copy link
Contributor

Nice, the PR looks correct. I'll need to test it and look into parserData a little more before I can be confident in this PR

@nerzhul nerzhul added this to the 5.1.0 milestone Dec 22, 2018
@paramat paramat removed this from the 5.1.0 milestone May 8, 2019
@v-rob
Copy link
Contributor

v-rob commented Jun 4, 2019

I suggest that this element follows the style of tableoptions in that it uses the option name followed by the value. An example: tableoptions[color=#00ff00;background=#ff0000]. So, I suggest that this one follows a similar path like so: listoptions[spacing=1,1;size=2;borderwidth=0]. This will make it easier to add new options if needed without rendering the whole tag invalid on older clients as well as being able to leave some options to their defaults.

BTW, how is the border size calculated? Is it in pixels, or a fraction of imgsize?

@rubenwardy
Copy link
Contributor

This should migrate across to use #8383

@v-rob
Copy link
Contributor

v-rob commented Jun 5, 2019

This should migrate across to use #8383

Due to the way that lists work and therefore the way this PR has to work with them, I don't know how feasible this might be. But for that to happen in the first place, #8383 has to be finished, which seems quite a ways off.

@rubenwardy
Copy link
Contributor

That pr is ready for review

@Unarelith
Copy link
Contributor Author

@rubenwardy I'm still around so ping me when you want to move this forward.

@SmallJoker SmallJoker added the Rebase needed The PR needs to be rebased by its author label Nov 10, 2019
@rubenwardy
Copy link
Contributor

@Unarelith :)

@Unarelith
Copy link
Contributor Author

I will try to rebase this PR soon, probably next week.

@Unarelith
Copy link
Contributor Author

New PR (#9240) created to rebase this one. It was way easier to add the modifications one by one rather than trying to rebase and fix all conflicts. Anyway, it's almost completely working now, I just have one small issue I didn't have before.

@rubenwardy
Copy link
Contributor

#9240

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

Labels

@ Client / Audiovisuals Feature ✨ PRs that add or enhance a feature Formspec Rebase needed The PR needs to be rebased by its author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants