GNOME Bugzilla – Bug 785672
Entry: Setting icon tooltip to empty disables tooltip on whole widget
Last modified: 2017-08-01 17:32:50 UTC
Created attachment 356696 [details] test case - toggle icon and check its vs entry's tooltips The Entry::set_icon_tooltip_(text|markup) methods set the tooltip text on the IconInfo and then call ensure_has_tooltip() to determine what to set :has-tooltip to. The problem is that when deciding whether to show a tooltip, they only take into consideration whether the present icons have any non-empty tooltips - not the Entry as a whole. So, if we have a tooltip on the entire widget but then set an empty tooltip on an (or both) icon - e.g. because it changed from a warning icon with an explanatory tooltip to an icon that doesn't need one - then ensure_has_tooltip() disables :has-tooltip for the Entry, and its overall tooltip is lost.
Created attachment 356700 [details] [review] Entry: Fix unset icon tooltip hiding Entry tooltip Our ::query-tooltip handler first checks whether the pointer is over any of the icons, returning their tooltip if so, and if not chains up to Widget::query-tooltip in order to show the text for the widget overall. But ensure_has_tooltip(), which exists to update :has-tooltip based on whether ::query-tooltip is needed, only set :has-tooltip to TRUE if any icon had a tooltip, without caring whether the widget as a whole does. That is asymmetrical and meant that if the Entry had a tooltip, but subsequently all icons had their tooltips unset, :has-tooltip would be set to FALSE, and hence the tooltip for the widget would become lost. The fix is to set :has-tooltip to TRUE if the widget has a tooltip of its own, and we only need to check the icons if that is not the case.
Now that I think about it, is ensure_has_tooltip() even needed? Surely the ::query-tooltip handler can just always run, but only return TRUE if any parts of the Entry have tooltip text. That would obviate having to have a custom :tooltip-text setter which would call ensure_has_tooltip(), which would seem to be required in addition to the above if we keep the current way of doing this.
Created attachment 356706 [details] [review] Entry: Fix unset icon tooltip hiding Entry tooltip This seems better. Btw, this problem affects GTK+ 2, 3, and 4; the patch is identical for 3 and 4 and only trivially different for 2. Our ::query-tooltip handler first checks whether the pointer is over any of the icons, returning their tooltip if so, and if not chains up to Widget::query-tooltip in order to show the text for the widget overall. ::query-tooltip only runs if :has-tooltip is TRUE. :has-tooltip was updated by ensure_has_tooltip() whenever any icon tooltip was updated. But this function ignored whether the widget as a whole had a tooltip. That is asymmetrical and meant that if the Entry had a tooltip set, but subsequently all icons had their tooltips unset, :has-tooltip would be set to FALSE, and hence the tooltip for the widget would become lost. The fix is simply to get rid of ensure_has_tooltip() and always set :has-tooltip to TRUE in init(). :has-tooltip is documented as indicating the widget /might/ show a tooltip from ::query-tooltip, and our handler already does the right thing to show/not show/chain up as appropriate.
(In reply to Daniel Boles from comment #2) > Now that I think about it, is ensure_has_tooltip() even needed? Surely the > ::query-tooltip handler can just always run, but only return TRUE if any > parts of the Entry have tooltip text. > > That would obviate having to have a custom :tooltip-text setter which would > call ensure_has_tooltip(), which would seem to be required in addition to > the above if we keep the current way of doing this. In fact, both patches will suffer from this: if we call set_tooltip_text(entry, NULL), then :has-tooltip will be set to FALSE, and so the icon tooltips will not be shown even if they are non-NULL. Still, it seems easy and sensible to fix this, so that unsetting an icon tooltip doesn't hide the overall widget tooltip, even if we can't guarantee the converse. We could document that tooltip text should be set on the entry first and then any icons, and with this fix at least an icon tooltip can be unset without breaking the widget one. Then the 1st patch is the proper one, as it ensures that if the Entry tooltip was unset, but an icon one is newly added, then :has-tooltip is set back to TRUE; the 2nd patch would not do that as it only sets :has-tooltip once. The most comprehensive solution would be to define overriden Entry:tooltip-(text|markup) properties and have them call ensure_tooltip()... but that seems like too much work. Any other thoughts?
Created attachment 356718 [details] [review] Entry: Warn about corner case hiding icon tooltips Here's a suggestion for the docs, to warn about the pitfall mentioned in my last comment and how to work around it. Wasn't sure of the best place to put it, but this seems as good a location as any.
Created attachment 356722 [details] [review] Entry: Warn about corner case hiding icon tooltips