Overload set_visibility for Panel2D and Combobox2D #768
Conversation
There was a problem hiding this comment.
Hi @dwijrajhari,
Thank you for the description. Can you add tests for this additional function. Thanks !
|
@skoudoro Added test. Kindly review |
|
@dwijrajhari can you rebase this PR so that we can review it? @skoudoro |
…r combobox dropdown_menu visibility
dwijrajhari
force-pushed
the
duplicate-callbacks
branch
from
9b44ddb
to
a451043
Compare
|
@skoudoro Please have a look at this PR! |
|
Hi @ganimtron-10 and @dwijrajhari, I think the fix can be even simpler than that. Actually, I do not succeed to reproduce the issue on the main branch. I agree that the arrow do not start on the right position but after 2 clicks, all is good. it look like an initialization issue and not So I recommend to test #731 using master branch. |
|
Ok, I looked deeper, I see what you are doing. |
There was a problem hiding this comment.
Ok, we might need to review the actor visibility design choice. It should be part of the UI future redesign.
For now, this solution looks good to me and thank you for doing this @dwijrajhari.
Thanks for the review @ganimtron-10.
Merging
Related issue: #731 (Clicking the tab of a ComboBox2D opens dropdown without changing icon)
Currently, the way set_visibility is implemented, it collects a list of all the actors down the tree and sets visibility of all of them. Thus the child elements of, say a Panel2D, don't get to choose which actors of themselves they want to make visible.
Thus when a TabPanel2D containing a comboBox2D is selected, it sets the visibility of the comboBox's drop_down_menu to true, even though its _menu_visibility is set to false.
Possible fix: We overload the set_visibility method of Panel2D to call set_visibility on all of its elements. We then overload the set_visibility of comboBox2D to only make the dropdown visible if _menu_visibility is true
This change fixes #731 and also passes existing tests in
fury\ui\testsHowever, since I am not fully aware of the codebase, I don't know if there was a particular reason, set_visiblility was implemented that way. @skoudoro Could you give me some hints?