Skip to content

Commit

Permalink
fix ~ prefix in file link detection (#1208)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mintty
mintty committed Apr 16, 2023
1 parent fe93a69 commit 7b60f9b
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/termmouse.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ static char scheme = 0;
}
else if (strchr("_#%~+-", c))
ret_p = p;
else if (strchr(".$@/\\", c)) {
else if (strchr(".~$@/\\", c)) {
if (!forward)
ret_p = p;
}
Expand Down

8 comments on commit 7b60f9b

@clayne
Copy link

@clayne clayne commented on 7b60f9b Apr 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last time these lines were touched was a very long time ago. Why is this suddenly needed to fix the URL parsing issues? What's magical about this line somehow being involved when I thought we already fixed the state issue?

image

4a031d6c4 termmouse.c     (andy.koppe 2010-08-15 20:00:28 +0000  71)     if (iswalnum(c))
4a031d6c4 termmouse.c     (andy.koppe 2010-08-15 20:00:28 +0000  72)       ret_p = p;
4a031d6c4 termmouse.c     (andy.koppe 2010-08-15 20:00:28 +0000  73)     else if (term.mouse_state != MS_OPENING && *cfg.word_chars) {
4a031d6c4 termmouse.c     (andy.koppe 2010-08-15 20:00:28 +0000  74)       if (!strchr(cfg.word_chars, c))
4a031d6c4 termmouse.c     (andy.koppe 2010-08-15 20:00:28 +0000  75)         break;
4a031d6c4 termmouse.c     (andy.koppe 2010-08-15 20:00:28 +0000  76)       ret_p = p;
4a031d6c4 termmouse.c     (andy.koppe 2010-08-15 20:00:28 +0000  77)     }
84d6e17f3 termmouse.c     (andy.koppe 2013-02-14 12:06:08 +0000  78)     else if (strchr("_#%~+-", c))
261012491 termmouse.c     (andy.koppe 2010-04-15 19:38:24 +0000  79)       ret_p = p;
84d6e17f3 termmouse.c     (andy.koppe 2013-02-14 12:06:08 +0000  80)     else if (strchr(".$@/\\", c)) {
261012491 termmouse.c     (andy.koppe 2010-04-15 19:38:24 +0000  81)       if (!forward)
261012491 termmouse.c     (andy.koppe 2010-04-15 19:38:24 +0000  82)         ret_p = p;
261012491 termmouse.c     (andy.koppe 2010-04-15 19:38:24 +0000  83)     }

@mintty
Copy link
Owner Author

@mintty mintty commented on 7b60f9b Apr 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was another failure reported in the issue and this change fixes it.

@clayne
Copy link

@clayne clayne commented on 7b60f9b Apr 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I saw the comment about ~. My concern here is that this is going to spiral out into whack-a-mole and having to touch lines that haven't been modified in over a decade is symptomatic of something else going on. If we rewind to before the URL changes were made, I don't believe there was any issue with something like ~/file being selected appropriately. Is that not the case? (I don't have an older build to check immediately). If it is the case that there was never a problem previously, then why would this line have to be changed after the URL changes were made, particularly when the URL changes had a problem with the select breaking before the final state and that bug was fixed. I mean . has always been in this list of chars as well, but . was failing just the same.

@mintty
Copy link
Owner Author

@mintty mintty commented on 7b60f9b Apr 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a justified enhancement request, the support of parentheses in URLs. Also there was some inconsistency between hover and double-click selection.
In the course of the implementation, previous behaviour (./file) was broken and then fixed.
Admittedly, fixing in this somewhat hacky area is a bit heuristic but a lengthy test list finally confirmed a good state was achieved, just missing the likewise broken ~/file case which was now also fixed.
You are welcome to more extensive testing. If you see further problems in behaviour please describe. As long as a list of test links passes for me, I do not see further changes justified however.

@clayne
Copy link

@clayne clayne commented on 7b60f9b Apr 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there's still a misunderstanding here or I'm missing something. With a build of fe93a698 (the commit before this commit) I have no issue selecting something like ~/foo/bar both with the defaults and OpeningMod=off. Additionally, with a breakpoint on the changed line and selecting (double-clicking) bar of ~/foo/bar I never even saw ~ come up as a candidate character in general:

Thread 1 "mintty" hit Breakpoint 1, sel_spread_word (p=..., forward=false) at termmouse.c:80
80     	    else if (strchr(".$@/\\", c)) {
1: c = 47 L'/'
2: p = {y = 1, x = 5, piy = 0, pix = 0, r = false}
3: ret_p = {y = 1, x = 6, piy = 0, pix = 0, r = false}
4: forward = false
5: scheme = 47 '/'
(gdb) c
Continuing.

Thread 1 "mintty" hit Breakpoint 1, sel_spread_word (p=..., forward=false) at termmouse.c:80
80     	    else if (strchr(".$@/\\", c)) {
1: c = 47 L'/'
2: p = {y = 1, x = 1, piy = 0, pix = 0, r = false}
3: ret_p = {y = 1, x = 2, piy = 0, pix = 0, r = false}
4: forward = false
5: scheme = 47 '/'
(gdb) 
Continuing.

Thread 1 "mintty" hit Breakpoint 1, sel_spread_word (p=..., forward=true) at termmouse.c:80
80     	    else if (strchr(".$@/\\", c)) {
1: c = 32 L' '
2: p = {y = 1, x = 9, piy = 0, pix = 0, r = false}
3: ret_p = {y = 1, x = 8, piy = 0, pix = 0, r = false}
4: forward = true
5: scheme = 0 '\000'
(gdb) 
Continuing.

When selecting ~ at the beginning of the line:

Thread 1 "mintty" hit Breakpoint 1, sel_spread_word (p=..., forward=true) at termmouse.c:80
80     	    else if (strchr(".$@/\\", c)) {
1: c = 47 L'/'
2: p = {y = 3, x = 1, piy = 0, pix = 0, r = false}
3: ret_p = {y = 3, x = 0, piy = 0, pix = 0, r = false}
4: forward = true
5: scheme = 0 '\000'
(gdb) c
Continuing.

Thread 1 "mintty" hit Breakpoint 1, sel_spread_word (p=..., forward=true) at termmouse.c:80
80     	    else if (strchr(".$@/\\", c)) {
1: c = 47 L'/'
2: p = {y = 3, x = 5, piy = 0, pix = 0, r = false}
3: ret_p = {y = 3, x = 4, piy = 0, pix = 0, r = false}
4: forward = true
5: scheme = 0 '\000'
(gdb) 
Continuing.

Thread 1 "mintty" hit Breakpoint 1, sel_spread_word (p=..., forward=true) at termmouse.c:80
80     	    else if (strchr(".$@/\\", c)) {
1: c = 32 L' '
2: p = {y = 3, x = 9, piy = 0, pix = 0, r = false}
3: ret_p = {y = 3, x = 8, piy = 0, pix = 0, r = false}
4: forward = true
5: scheme = 0 '\000'
(gdb) 
Continuing.

In fact, how is this change even going to do anything given that line 78 already has ~ in its set of characters to match? This is also the exact behavior I'd see it result in while debugging. This is what I meant with my earlier comments, that we're changing things here that seem to have no connection to the issue at hand, haven't needed to be changed in over a decade, and upon further examination the changed lines won't even result in any effective difference because an existing conditional already handles the same character.

@mintty
Copy link
Owner Author

@mintty mintty commented on 7b60f9b Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for insisting, I confirm your observation about build fe93a69.
But then there was comment #1208 (comment) which I had initially confirmed too.
Maybe both @lazka and me had some dust in a working copy...
Also when I assumedly fixed this and checked for existing handling of ~, I apparently overlooked line 80.
So I'll patch this back.

@lazka
Copy link

@lazka lazka commented on 7b60f9b Apr 20, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was testing with/using the last release. I can try master though. (also it's not that big of an issue, just a minor annoyance for my muscle memory)

@mintty
Copy link
Owner Author

@mintty mintty commented on 7b60f9b Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lazka, so I misunderstood your comment, sorry.

Please sign in to comment.