Nope; it was actually commit 3e74409 (bug 786441) breaking it.

Created attachment 362378 [details] [review]
Fix

Fix – to be thorougly verified along with the fix to bug 789390.

Regex matches seem to properly stop being underlined when the mouse disappears; however, explicit hyperlinks don't.

To be con't...

Regexes stop being underlined because of that match_contents_clear() just above the line this patch modifies.

We can't just similarly clear the hyperlink because then we'd need to emit a signal (for the tooltip text), and we shouldn't emit spurious back-n-forth ones. So it's a tricky one.

Two more related issues:

- When the mouse is moved but remains within the same cell, the pointer appears but the regex underline doesn't.

- Regexes/hyperlinks often remain underlined when the mouse leaves the window.

Looks like it's quite hopeless to solve these one by one. A generic cleanup is desirable which solves all these at once, and handles regexes and hyperlinks the same way.

I can foresee a dependency loop issue. I'm somewhat tempted to say that apply_mouse_cursor() should stop/start underlining. (We want underlining to appear/disappear when the pointer does, and even fire up a hyperlink-hover-uri-changed event in these cases.) However, a new or a ceased regex/hyperlink match also causes the mouse cursor shape to change.

We could argue that even though technically that'd be a loop in the function call chain, it couldn't go infinite because apply_mouse_cursor() could only trigger a regex/hyperlink change if it shows/hides the cursor, whereas the latter could only cause a change in the particular mouse cursor shape if it's actually shown. But it's still damn ugly.

Right now I'm pondering about separating "show/hide mouse pointer" logic from "what the mouse pointer looks like if it's shown" logic. I'll be thinking about other possible approaches too.

What frustrates me the most:

I have aliased "ls" to "ls --hyperlink=auto". (coreutils >= 8.28)

If I open a new terminal which happens not to open under the mouse, and then execute "ls", the filename shown in the top left becomes underlined.

Trying to clean up the desired behavior in my mind... I think there are 3 different cases:

  1) The pointer is over VTE and is not auto-hidden. VTE should look for regex/hyperlink matches, and set the pointer shape according to these, as well as according to mouse tracking mode.

  [1b) A special sub-case could be when the pointer is over the padding. We could save CPU by not looking for regex/hyperlink, but the mouse pointer would still need to be set according to mouse tracking mode. I totally don't think this case is worth optimizing for and complicating our codebase with. Plus, just as we confine mouse clicks sent towards apps into the actual non-padding area, probably confining the coordinate for regex/hyperlink matches improves usability, especially with fullscreen terminals. So let's forget this case.]

  2) The pointer is over VTE but is auto-hidden. VTE should not look for regex/hyperlink matches (to save CPU) and should install the invisible pointer.

  3) The pointer is outside of VTE. VTE should not look for regex/hyperlink matches (to save CPU), but needs to install an actual pointer according to mouse tracking mode (to avoid issues with popovers, as per the previous links).

The case of fully obscured VTE (e.g. non-selected tab in a g-t window) should be covered by the 3rd case. (We should double check though, once we have a candidate implementation.)

I do hope I got it right, and will try to fix the issues along these lines, around these 3 well-defined cases.

Yet another issue:

When the mouse enters the VTE widget, and enters an actual character cell (not just the padding), the regex or explicit hyperlink underline doesn't update immediately. It only updates after the mouse is moved to another character cell.

Also reproducible with larger paddings and slow mouse movement, so it's not that the "over the padding" state is "skipped". Reproducible from any of the four directions.

Another interesting corner case:

Have some URL. Mouseover it, it's underlined as expected. Highlight a part of it. During highlight, underlining is intentionally removed. When the mouse is released, underlining is not restored. You have to leave the URL and re-enter with the mouse to underline it. (Moreover, if you release the mouse outside of this URL, the pointer shape isn't updated.)

And yet yet yet another twist to it:

Have some blinking content somewhere else. At the next blink cycle the screen is fully repainted, and as such, the URL becomes underlined.

It's sooooooooo complicated. I'm working on a patch that will hopefully significantly improve the situation, but I don't promise I'll address these all :)

The m_show_match variable suggests to me that probably the code isn't designed to work as I thought.

I thought VTE was supposed to / used to not waste CPU cycles when the cursor was invisible. I'm no longer sure about it. This variable suggests that regex matching always takes place, and on top of that there's the additional possibility of not underlining the result under some circumstances.

I would turn it upside down. I'd say that if we don't want to underline for whatever reason (e.g. cursor autohidden, or selecting in progress), we should invalidate the current match and make sure that we don't waste CPU time on looking for new matches. But whenever there's a match, it should be underlined.

This could probably noticeably speed up some cases, e.g. when lots of contents are produced while the mouse is hidden. But this approach has a drawback too: if the match is invalidated (e.g. keypress autohides the pointer) and then the mouse is moved within the same cell, the regex match is run again, although we could cache the previous match. To me this sounds like a much less significant optimization that I'd say is probably not worth going for, for the sake of a simpler and cleaner code.

Confine or Ignore?

What shall be the behavior when the mouse is over the padding?

(In reply to Egmont Koblinger from comment #9)
> The m_show_match variable suggests to me that probably the code isn't
> designed to work as I thought.
> 
> I thought VTE was supposed to / used to not waste CPU cycles when the cursor
> was invisible. I'm no longer sure about it. This variable suggests that
> regex matching always takes place, and on top of that there's the additional
> possibility of not underlining the result under some circumstances.
> 
> I would turn it upside down. I'd say that if we don't want to underline for
> whatever reason (e.g. cursor autohidden, or selecting in progress), we
> should invalidate the current match and make sure that we don't waste CPU
> time on looking for new matches. But whenever there's a match, it should be
> underlined.

Yes, we shouldn't waste any time computing the match if we're not going to show it anyway.

> This could probably noticeably speed up some cases, e.g. when lots of
> contents are produced while the mouse is hidden. But this approach has a
> drawback too: if the match is invalidated (e.g. keypress autohides the
> pointer) and then the mouse is moved within the same cell, the regex match
> is run again, although we could cache the previous match. To me this sounds
> like a much less significant optimization that I'd say is probably not worth
> going for, for the sake of a simpler and cleaner code.

Agreed.

(In reply to Egmont Koblinger from comment #10)
> Confine or Ignore?
> 
> What shall be the behavior when the mouse is over the padding?

When the mouse is not over a cell, there should be no highlighting, IMHO.

> > Confine or Ignore?
> 
> When the mouse is not over a cell, there should be no highlighting, IMHO.

Mouse clicks are confined when sent to apps. So, confining would be more consistent with it, and a tiny bit easier to use if the window is fullscreen and the URL appears at its side, you can push the mouse all the way to the edge and then click.

On the other hand, this behavior feels somewhat weird to me and probably would look weird at the extra padding at the right/bottom of fullscreen windows. Plus, ignoring padding clicks looks a bit easier to implement.

I really don't have a preference here, so I'll go for your choice and ignore them.

*** Bug 780975 has been marked as a duplicate of this bug. ***

*** Bug 732185 has been marked as a duplicate of this bug. ***

Created attachment 369006 [details] [review]
Fix initial underlining when the widget appears

While working on the big cleanup, I'm factoring out a couple of small standalone and ready fixes that almost certainly won't be affected by the remaining changes.

This tiny one fixes the initial underlining, as per bug 732185.

Created attachment 369010 [details] [review]
Reset hyperlink_hover_uri

Reset m_hyperlink_hover_uri to NULL when moving the mouse to a non-hyperlinked cell.

Created attachment 369011 [details] [review]
Fix initial underlining when the widget appears, part 2

This one is also required for properly underlining (or not underlining) links at startup. Here the initialization of m_mouse_cursor_over_widget is fixed.

The second previous patch fixes the URL to be properly underlined (or not) if the widget appears below the mouse. This one fixes it not to be underlined when the widget appears somewhere else.

Comment on attachment 369010 [details] [review]
Reset hyperlink_hover_uri

But use nullptr instead of NULL, please.

Comment on attachment 369011 [details] [review]
Fix initial underlining when the widget appears, part 2

Thanks!

> But use nullptr instead of NULL, please.

Shall I also change inside _vte_ring_get_hyperlink_at_position() ?

Created attachment 369059 [details] [review]
Cleanup for initial underlining

This is probably not necessary after the previous changes, but IMO would be nice to have.

m_mouse_last_position is implicitly initialized to (0,0), that is, the upper-left cell. I think it's more robust to initialize it to the padding.

On the other hand, reset() moved it to (-1,-1) which I don't see a reason for, a reset operation doesn't/shouldn't change the last seen coordinate.

(In reply to Egmont Koblinger from comment #20)
> > But use nullptr instead of NULL, please.
> 
> Shall I also change inside _vte_ring_get_hyperlink_at_position() ?

Only for new/changed code; existing code should get changed when touched.

(In reply to Egmont Koblinger from comment #21)
> Created attachment 369059 [details] [review] [review]
> Cleanup for initial underlining
> 
> This is probably not necessary after the previous changes, but IMO would be
> nice to have.
> 
> m_mouse_last_position is implicitly initialized to (0,0), that is, the
> upper-left cell. I think it's more robust to initialize it to the padding.
> 
> On the other hand, reset() moved it to (-1,-1) which I don't see a reason
> for, a reset operation doesn't/shouldn't change the last seen coordinate.

Yes you're right; last seen coord is a property of the view, reset shouldn't change view properties.

Created attachment 369069 [details] [review]
wip0

(draft, checkpoint of where I am right now, don't review)

This one hopefully fixes all the behavior around explicit hyperlinks. The former hyperlink_hilite() method does a few optimizations it shouldn't do or does incorrectly: it bails out when the mouse is moved to the padding (underlining should be removed), and compares a non-confined coordinate to a confined one.

Since hyperlink_hilite() detects early on if the hyperlink remains the same, and it's a cheap operation, there's no point in a wrapper in front of it.

Essentially the same should be repeated for regexes too. It's probably going to be more complicated, plus I'm less familiar with that code...

After thinking about it a lot, here's the biggest pain point I see which makes this code hard to touch, or the fix to something often breaking something else...

IMO the biggest pain point is the m_mouse_last_position variable. I just think it uses a wrong paradigm, and has a wrong name. Does "last" even intend to mean "latest seen"; if so, is that a synonym "current" (if not then why)? Why include this word then? Or does it intend to mean "previous"? It's actually a weird mixture of the two. Most of the time, incl. when VTE is idle, this variable contains the current position. While updating the state, you can never be sure in the code if it's the current or the previous (just before getting updated). E.g. sometimes regex underlining happens at the brand new mouse position (passed as a parameter), while m_mouse_last_position contains the previous position. At other places, m_mouse_last_position is passed as the parameter that specifies the current position, that is, the mouse didn't move (why do we call these methods at all then, shouldn't they be a no-op?).

When it's used for e.g. underlining regexes/hyperlinks, comparing the new value (received as a parameter) against the old (m_mouse_last_position) on its own cannot always tell if there's action to be taken. Maybe the mouse pointer just appeared/disappeared due to external circumstances but the mouse didn't move. Following this pattern, we'd need at least a boolean "m_mouse_pointer_last_visible" that we could compare to the current (new) value which is passed as a new parameter. Complicated.

The previous mouse position is potentially used in multiple subcomponents:
- highlighting
- sending mouse events to apps
- regexes
- hyperlinks
maybe even more. Having one common variable storing the previous position means that all these subcomponents have to update their stuff at once, before we update m_mouse_last_position. I find this approach pretty fragile.

If we'd like to avoid the introduction of the aforementioned "m_mouse_pointer_last_visible", the regex and hyperlink subcomponents would be happy with just storing (-1, -1) as the last position when the pointer is hidden. This would not work for other subcomponents though, their requirements are different.

---

So I just think that a "global" (I mean: spanning across multiple subcomponents) variable holding the old value is not a good approach. Instead, the member variable should contain the current (i.e. new) value. That is, upon mouse movement and such, we should update this first and then do the regex/etc. stuff. The variable should accordingly be named m_mouse_position.

If any of the subcomponents need to track the previous value, it should be done individually by each of them, whenever that subcomponent is called, without having to care about the other ones. E.g. hyperlink doesn't need to know it, it just tracks the index of the hyperlink the mouse is over (as per the draft patch). Regex: I'm not sure if it'll need to track it; if yes then it could store (-1,-1) if the pointer is invisible, but probably m_selection_{start,end} will make it unnecessary to keep the previous position. Highlighting and reporting to apps sure need to track it.

{match,hyperlink}_hilite_update shouldn't receive a position parameter, they should work from m_mouse_position containing the current (new) position, plus of course m_mouse_cursor_autohidden and whatever else they need.

---

I'll try to refactor the code (and fix the issues) along these lines.

Created attachment 369273 [details] [review]
v1

This is mostly what had in my mind. (I didn't clean up m_mouse_last_pointer for the rest of the code, and not planning to do that now.)

I think it's mostly a win in code size, readability (I hope so), and fixes the underlining issues. Plus, we no longer do regex matching if it wouldn't be underlined.

Christian, please take a look and let me know if you like the overall direction.

There's one tiny regression: Double clicking removes the underline for a fraction of a second, and then restores. This flickering wasn't there previously. I'll try to fix this later tonight.

The direction is good, and the patch itself looks ok too :-)

Created attachment 369276 [details] [review]
v2

Oops, I actually attached the wrong patch in the afternoon, it didn't contain my last bunch of modifications. I wanted to attach this one instead :-)

> There's one tiny regression: Double clicking removes the underline for a
> fraction of a second, and then restores. This flickering wasn't there
> previously.

Well, this is not strictly speaking technically a bug/regression in the patch, but rather the unfortunate logical behavior.

The assignment of do_check_highlight contains a !m_selecting condition, because as per VTE's current behavior, we'd prefer to stop underlining when the user selects with the mouse. (I assume underlining would be distracting.)

A single click-and-hold doesn't immediately enter selection mode, we wait for the mouse to move by a certain threshold.

For double-click-and-hold (click, release, click) we immediately enter selection mode (we _have to_, since the word is already highlighted), via the GDK_2BUTTON_PRESS branch that calls start_selection() that sets this variable. And since we're selecting, it's no longer underlined.

So here retaining the previous, more user-friendly behavior would require some additional hacks. It's not clear to me yet how to proceed (track in a new variable whether the cursor has been moved by a tiny bit?), and not clear if it's worth it. The patch improves so many things that even with this regression I'd say it's a win :-D

Comment on attachment 369276 [details] [review]
v2

(In reply to Egmont Koblinger from comment #28)
> So here retaining the previous, more user-friendly behavior would require
> some additional hacks. It's not clear to me yet how to proceed (track in a
> new variable whether the cursor has been moved by a tiny bit?), and not
> clear if it's worth it. 

Not worth it, I'd say.

> Not worth it, I'd say.

Okay. (Maybe later, if people complain.)

Submitted.