Skip to content

Commit

Permalink
Lua: Invalidate layout in set_dialog_value()
Browse files Browse the repository at this point in the history 
Needed when the new value is longer than the old one.

Fixes #3572.
  • Loading branch information
@jostephd
jostephd committed Sep 29, 2018
1 parent 8f8ec38 commit 8000a86
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/scripting/lua_gui2.cpp
View file
Expand Up @@ -462,6 +462,8 @@ int show_message_box(lua_State* L) {
int intf_set_dialog_value(lua_State* L)
{
gui2::widget *w = find_widget(L, 2, false);
if (w)
w->get_window()->invalidate_layout();

#ifdef GUI2_EXPERIMENTAL_LISTBOX
if(gui2::list_view* list = dynamic_cast<gui2::list_view*>(w))
Expand Down

8 comments on commit 8000a86

@gfgtdf
Copy link
Contributor

@gfgtdf gfgtdf commented on 8000a86 Sep 29, 2018

Choose a reason for hiding this comment

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

#3572 is specific about labels, that have a text that might be cutoff. But commit calls invalidate_layout() when any widget is changed, like setting the selected listbox item, toggeling a checkbox etc. . invalidate_layout() is slow, So it really only should be called when a text is changed.

@jostephd
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's exactly why I posted the patch as a proof of concept and asked for reviews/suggestions on it. What would be the best way not to call invalidate_layout() on checkboxes (and any other widgets that don't change size when they change value)?

@gfgtdf
Copy link
Contributor

@gfgtdf gfgtdf commented on 8000a86 Sep 29, 2018

Choose a reason for hiding this comment

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

i suggest to move the call down the lines (inside those elseifs) for all widgets that need it (text_box, and the 'else' banch that handles all generic widgets that where set_value sets the label). (currently that'd be line 509 and 579.)

@jostephd
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd considered that but I wasn't sure that none of the other branches would ever need an invalidation. For example, tree_view_node probably also needs an invalidation, and selectable_item is abstract so it also needs an invalidation.

@gfgtdf
Copy link
Contributor

@gfgtdf gfgtdf commented on 8000a86 Sep 30, 2018

Choose a reason for hiding this comment

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

well, the action on tree_view_node (expand node) and selectable_item (checkbox, togglebutton) are basicially the same as if the user clicked on thme where it doesnt need invalidate either, so there it wouldnt really mke sense to call it there. For treeviews specificially i don't remember its layout algorithm exactly but afaik it is also able to add a scrollbar around it on its own (will will then alrady call invalid_layout ? Again not exactly sure) and will never result in wiodgets/text. For toglebuttons, i guess it is in theory possible that someone creates a togglebutton definition that has a differnt size ion the toggeled state, but woudl really be a bug that should be solved in a different way.

@jostephd
Copy link
Member Author

Choose a reason for hiding this comment

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

So maybe invalidate_layout should be called from the set_value member function of each widget that changes size when its value changes?

@gfgtdf
Copy link
Contributor

@gfgtdf gfgtdf commented on 8000a86 Oct 1, 2018

Choose a reason for hiding this comment

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

So maybe invalidate_layout should be called from the set_value member function of each widget that changes size when its value changes?

No, i think this would make some dialogs, in particular the multiplayer lobby, which already suffers a lot from slowness even worse.

invalidate_layout has multipel disadvantages:

  1. It's slow
  2. It makes the dialog look realy bad if its size changes often, like for example when it happens whenever you click on a button. (invalidate layout will not only increase size, but also decrease size so calling on a invalidate_layout might cause it's size to change even when it is not neeced at all)

So most dialog are designed to allocate the correct size when the dialog layout is first created (layouted), meaning avoiding calling set_label while the dialog is shown and instead use a 'stacked_widget' or a 'multipage' widget that uses the maximum posible size for any of its child widgets then instead of setting the label you would have one 'page' with the first text, one page with the second text and then during the dialog switch just switch the pages instead of setting the label.

While this sounds good in theory there are unfortunteley some cases where calculating all sizes befor the widget is shown is not possible, and thus calling set_label cannot be avoided. For example:

  1. the loadgame dialog, not only there are probably a lot of saves, some players have multiple hundred saved games so calculating their informaltion-panel-sizes alone woudl take a lot of time, also the game would probably need to open all of those savefiles to calculate that infromation which is also slow.

In cases like the savegame dialog it would clearly be best to somehow fix the size of left-side-panel and when a new game is selecrted to only relayout the size of that panel instead of of the whole dialog, In the case of the multiplayer create dialog (which also suffers froma sinilar problem )

  1. The multiplayer lobby, look for exampel at the 'turn_number' label, i would totally not be suprised if it'd be cut off when it exeeds 100 while the lobby is open, also it clearly wouldn't make sense to have like a 999-page multipage widget for every possible number.

In cases like that it'd be best if label had preallocaed the maximum possible size (assuming for example lesss than 1000 turns) and allocating these in advance.

  1. Adding items on a listbox or a similar widget can cause an invalidate_layout when it casues the listbox to have a scrollbar, this isn't too bad because it can happen only at most once (per listbox) while the dialog is shown, but it you know in advance that many item will be added later to the list that the scrollbar is amost certainly needed , it doens make sense to set the scrollbar mode to always when the dialog is shown.

@jostephd
Copy link
Member Author

Choose a reason for hiding this comment

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

2. It makes the dialog look realy bad if its size changes often, like for example when it happens whenever you click on a button. (invalidate layout will not only increase size, but also decrease size so calling on a invalidate_layout might cause it's size to change even when it is not neeced at all)

So most dialog are designed to allocate the correct size when the dialog layout is first created (layouted), meaning avoiding calling set_label while the dialog is shown and instead use a 'stacked_widget' or a 'multipage' widget that uses the maximum posible size for any of its child widgets then instead of setting the label you would have one 'page' with the first text, one page with the second text and then during the dialog switch just switch the pages instead of setting the label.

Okay, I understand, but the case in question is that Lua code did change the value of a label. That's part of the API, we can't forbid UMC from changing values of labels, and we don't know if the UMC author knew (or for that matter was able to) size the dialog correctly up front.

I really don't know how to proceed here. I don't want to call invalidate_layout() any more often than necessary but I don't see how to make fewer calls without possibly breaking something. Perhaps we can continue this discussion on the forums or discord or a github issue where more people will see it?

Thank you for the very detailed answer.

Please sign in to comment.