GNOME Bugzilla – Bug 786814
hyperlink tooltip doesn't update if invalid utf8
Last modified: 2017-08-31 19:48:33 UTC
Have two hyperlinks right below each other, one of them having an invalid UTF-8 target. E.g. with the patched "ls" command and an invalid UTF-8 filename. Mouseover the valid UTF-8 one, wait until the tooltip is shown. Then move the mouse over the invalid one. Actual: Tooltip text remains the same (the previous filename). Expected: Show U+FFFD replacement characters, or at the very least an empty popup (as done now if you mouseover here from a non-hyperlinked cell) or perhaps nothing at all, but definitely not some other cell's target.
You can get the U+FFFD thing by just calling g_utf8_make_valid() after the g_uri_unescape_string(). (BTW I noticed that terminal-window.c:screen_hyperlink_hover_uri_changed_cb() leaks @label.)
(In reply to Christian Persch from comment #1) > You can get the U+FFFD thing by just calling g_utf8_make_valid() That's what I did in a pretty early version, but refactored later. It'd require a brand new glib-2.52, I don't want to bump the dependency that much. I thought the rest (the tooltip displaying code inside Gtk+) would be clever enough to automatically do so... I was wrong.
The code of g_utf8_make_valid is not too long, so you could just copypaste it into terminal-util.
(In reply to Christian Persch from comment #1) > (BTW I noticed that > terminal-window.c:screen_hyperlink_hover_uri_changed_cb() leaks @label.) What do you mean? It has gs_free.
Created attachment 358452 [details] [review] fix I never understood the purpose of this reverse-chronological-deprecation thingy with GLIB_MAX_ALLOWED. Is it okay to bump it? At least it gets rid of a deprecation warning. Is there a word for this at all? Like depostcation maybe? :P (No Google match for this word yet :))
I think I managed to understand the purpose of MAX_ALLOWED. Unlike MIN_REQUIRED, which is for the user who compiles the package to bail out early on if glib isn't recent enough, MAX_ALLOWED is for us, developers. This guarantees that we are warned if we erroneously use a newer API without incrementing MIN_REQUIRED (which in turn would cause the user to see problems). I think my current fix is technically correct, however, this approach defeats the purpose of MAX_ALLOWED since we're no longer protected against making such a mistake somewhere else in the code. Ideally MAX_ALLOWED would only be modified to 2.52 instead of the global 2.42 for this particular method that we're adding now. It's unclear to me whether this can be done somehow, probably not.
... Or, of course, I could go for unconditionally using our copy-pasted implementation and never glib's :P
(In reply to Egmont Koblinger from comment #7) > ... Or, of course, I could go for unconditionally using our copy-pasted > implementation and never glib's :P Yes, that'd be fine with me.
Created attachment 358840 [details] [review] fix v2 Avoid branching on glib version, use copied implementation for the time being.
Submitted.