search

elementary / wingpanel

Stylish top panel that holds indicators and spawns an application launcher
https://elementary.io
GNU General Public License v3.0
137 stars 46 forks source link

Suppress tooltips when popover is open #393

Closed vjr closed 3 years ago

vjr commented 3 years ago

Fixes #392

@pongloongyeat I am unable to assign reviewers - would you mind reviewing or requesting appropriate developer to look at this?

vjr commented 3 years ago

I guess I should use the unowned keyword for the display widget vars. Will update later in the day.

vjr commented 3 years ago

At least one other issue with this PR is it assumes the tooltip is set in the markup property and does not save/restore the plain text tooltip property - what if there are third party indicators that do this?

Is there any way to distinguish which is the active property set by an indicator?

vjr commented 3 years ago

How about connecting to the query-tooltip signal and return false to suppress the tooltip? Then it would not matter what kind of tooltip it is.

Oh I see, thank you, let me look into that (might take me a few days to revisit this PR), it would simplify things a lot I guess!

vjr commented 3 years ago

Hi @jeremypw looks like you already have a branch with the simplified approach here -> https://github.com/elementary/wingpanel/tree/suppress-tooltips which looks good to me so I guess you can go ahead and have it merged 👍🏻

vjr commented 3 years ago

@jeremypw I guess around line 30 in your branch the _previous_indicator declaration can be removed.

jeremypw commented 3 years ago

@vjr Huh - I had completely forgotten about that branch! Not sure why I did not submit it. I'll have a look at it again and submit. Thanks.

jeremypw commented 3 years ago

@vjr It looks like I did push that branch as a PR to this one (https://github.com/vjr/wingpanel/pull/1). So I have now removed the unused code. Its up to you to merge it into this one.

vjr commented 3 years ago

@vjr It looks like I did push that branch as a PR to this one (vjr#1). So I have now removed the unused code. Its up to you to merge it into this one.

Done!

marbetschar commented 3 years ago

Not tested this, but instead of introducing an additional variable (is_popover_open) isn't there already an instance variable of the Gtk.Popover itself which does reflect the open/close state? Maybe Gtk.Widget.visible?

vjr commented 3 years ago

Not tested this, but instead of introducing an additional variable (is_popover_open) isn't there already an instance variable of the Gtk.Popover itself which does reflect the open/close state? Maybe Gtk.Widget.visible?

Thanks for the comment! Yes, let me take a look and see if it works and I'll update this PR if it does...

vjr commented 3 years ago

@marbetschar looks like the visible property works!

@jeremypw i had to make some minor changes in addition to using the visible property because the tooltip was not being restored in some cases (likely when i was switching between adjacent popovers while they were open) - and i noticed you used .indicator_widget in some places and .display_widget in others; was there a reason for that? i've just used .display_widget everywhere...

in any case, the PR is whittled down to just a 3-line diff now 👍🏻

jeremypw commented 3 years ago

Other than a minor quibble about the suppress_tooltip () function this is looking good. Thanks.

vjr commented 3 years ago

Other than a minor quibble about the suppress_tooltip () function this is looking good. Thanks.

There is already a similar existing private function called make_modal () in this file with a boolean flag that inverts the behaviour, but do you have a suggestion for a better name than suppress_tooltip () ? Maybe update_has_tooltip (xxx, bool enable = true) ?

jeremypw commented 3 years ago

If the display widget can validly be null then it ought to be defined as nullable in the code and it isn't. I cannot see a valid case for having an indicator without a display widget tbh. I suppose 3rd party indicators might do something weird but I am not sure we still support those. However, I guess it does no harm to be on the safe side and keep this check. In which case update_has_tooltip () is a reasonable name.

vjr commented 3 years ago

If the display widget can validly be null then it ought to be defined as nullable in the code and it isn't. I cannot see a valid case for having an indicator without a display widget tbh. I suppose 3rd party indicators might do something weird but I am not sure we still support those. However, I guess it does no harm to be on the safe side and keep this check. In which case update_has_tooltip () is a reasonable name.

@jeremypw done!