GUI: Fix footer update after selecting None #4345
Conversation
src/gui/footer/footer_line.cpp
Outdated
| @@ -51,6 +51,8 @@ bool FooterLine::Create(footer::Item item_id, size_t index) { | |||
|
|
|||
| if (result) { | |||
| positionWindows(); | |||
| } else if (item_id == footer::Item::none) { | |||
| Erase(index); | |||
There was a problem hiding this comment.
This doesn't seem to be a correct fix. The result is set to true on any change, which should call positionWindows. That is, even on change to/from none. This thing might possibly work, but it's a bit random, plus it will call positionWindows each time (inside Erase), suboptimally.
By quickly looking at the code, the proper/elegant fix would probably be to add FooterItemMappingRec<std::monostate, Item::none> to FooterItemMappings
There was a problem hiding this comment.
Trying to add FooterItemMappingRec as suggested doesn't work due to compiler error:
error: static assertion failed: T must occur exactly once in alternatives [build] 1115 | static_assert(__detail::__variant::__exactly_once<_Tp, _Types...>,.
As far as I could see, Erase doesn't call positionWindows and it only is erased if result is false, which happens if the item isn't registered (which is only the case for footer::item::none).
There was a problem hiding this comment.
I'm now using unregister directly and also moved it above the FooterItemsMapping function, which should be a more proper way to do it. If you have any ideas how to make the FooterItemMappingRec working with std::monostate, please let me know :)
bkerler
force-pushed
the
Fix_footer_update
branch
from
069bf61 to
4202f5a
Compare
bkerler
force-pushed
the
Fix_footer_update
branch
from
4202f5a to
acade0a
Compare
| @@ -41,6 +41,9 @@ bool FooterLine::Create(footer::Item item_id, size_t index) { | |||
|
|
|||
| if (item_id >= footer::Item::_count) { | |||
| item_id = footer::Item::none; | |||
| } else if (item_id == footer::Item::none) { | |||
| unregister(index); | |||
There was a problem hiding this comment.
This does not call positionWindows, which it probably should in some cases (at least according to the previous code).
| @@ -41,6 +41,9 @@ bool FooterLine::Create(footer::Item item_id, size_t index) { | |||
|
|
|||
| if (item_id >= footer::Item::_count) { | |||
| item_id = footer::Item::none; | |||
| } else if (item_id == footer::Item::none) { | |||
| unregister(index); | |||
| return false; | |||
There was a problem hiding this comment.
It is rather debatable whether this should call true or false. From the previous code, it rather seems like the function should return true if an item is changed (and in that case, and only in that case, it should call reposition). This code does not follow that logic.
But looking around, it seems like the return value is not used anywhere, so it would perhaps be best to just turn it into a procedure.
This fixes issue #4166.
After selecting None in the Footer settings menu, the existing footer item isn't erased, thus stays active although it has been disabled.