search

projecthamster / hamster-shell-extension

Shell extension for hamster
http://projecthamster.org
GNU General Public License v3.0
217 stars 92 forks source link

panelWidget: Do not override show() method #336

Closed matthijskooijman closed 3 years ago

matthijskooijman commented 4 years ago

This widget defined a show() method, which would show the menu. However, show() is already a method defined by some class higher up in the hierarchy, which makes the widget in the status area visible. By defining a show() method here, the behavior of the show() method changes, causing issues.

One such issue showed when combining this extension with the status-area-horizontal-spacing extension, which calls hide() and show() on each widget in the status area. With the overridden show() method in the hamster extension, this would hide the widget and then show the menu, breaking the extension because the widget would stay hidden.

This commit renames show() to show_menu() to prevent to conflict. It seems that the method was not actually used, so no references were updated. For consistency, toggle() is also renamed to toggle_menu(), with one reference updated.

rhertzog commented 3 years ago

Looks good to me. Though I wonder why you keep show_menu() since it's not used.

matthijskooijman commented 3 years ago

Hm, good point. I guess I assumed it served a purpose, but if it's not used I could also remove it. Any thoughts from others?

Even if show() / show_menu() is removed, I guess renaming toggle() to toggle_menu() might be good, to emphasize that it toggles the menu, not the panel widget itself.

hedayat commented 3 years ago

I also agree with removing show() (assuming it is not really necessary) and renaming toggle_menu().

matthijskooijman commented 3 years ago

Thanks for the feedback, I've updated the PR to remove show() instead, and moved the toggle_menu() change into its own commit.

Note that since v2.6, the status-area-horizontal-spacing extension no longer calls hide() and show(), so that conflict is no longer present, but it is still good to fix this at the hamster end.