Properly selects means the test between cursor location and click location are selected.

Obviously it's text not test.

presumably broken by commit e580c29f07f42fb171264a97389a18a2a7cdb3cd changing from button-press events to gestures

Also not working is the "Truncate current selection, but keep it as big as possible" behaviour.


I suspect a comparison is inverted somewhere, but that's just based on a quick glance and is probably totally wrong

perhaps relevant: GTK+ 4 not only doesn't work here, but also emits a display beep when you try to select this way

...which is due to the IM context thinking that the release of the Shift key represents the end of a key sequence, which failed.

The beep occurs on current GTK+ 3 too. My guess is that it might be related to the recent commit adding support for entry of emoji sequences, though I haven't bisected this yet.

Created attachment 357036 [details] [review]
Entry: Fix Shift-click → extend/truncate selection

Since the move from button-press to gesture events, Shift-clicking did
not work to start a selection (from none) or truncate an existing one.

This was due to the code being copy-pasted around and some logic being
broken in the process. This makes both of those work as they should, by
shuffling it again so the end result is the same as before. Highlights:

(1) ::button-press if extending due to a single press would call
set_positions(tmp_pos, tmp_pos), which is what made the Shift+click to
create a selection work. That was lost. Add it back to make that work.

(2) ::button-press in the “Truncate current selection” branch would not
execute all the stuff around “extend_to_left”, as that was the else
case. So, set extend_selection = FALSE so we skip over that later on.

(3) BUT! This Truncate case never fired because it was in the else
branch of if (in_selection())! Of course, it must be in the true branch.

(4) The IM context was not reset if the Shift-click occurred within an
existing selection, only if it did not. In ::button-press this was the
first thing done if extending a selection, regardless. Make it so again.

Hi Carlos, any thoughts? :)

Carlos had an alternative patch but said on IRC the way I do it made sense (plus, mine ensures the IM context is always reset, which his didn't, iirc). So, I'll probably go for it, so we can get this important feature fixed in 3.22.20.

It may be possible to shuffle this all around again to read more nicely, so it can always be refactored, but it's more important just to get the feature working again first, and afaict I exactly restore the old behaviour with minimal changes.

Review of attachment 357036 [details] [review]:

Yes, my patch didn't try to address the IM context issue. The code shuffling in your patch also makes sense, so let's go with your changes instead.

One minor nit with the patch, AFAIK there's no specific codestyle for separating cases on a switch statement, so I'd maybe leave the extra newlines out, as it's not like the current code is breaking any style.

Thanks for the review!

(In reply to Carlos Garnacho from comment #11)
> One minor nit with the patch, AFAIK there's no specific codestyle for
> separating cases on a switch statement, so I'd maybe leave the extra
> newlines out, as it's not like the current code is breaking any style.

docs/CODING_STYLE says:

> It is preferable, though not mandatory, to separate the various cases with
> a newline

At the time, I wasn't consciously aware of that, and based my decision on how (A) I was in the area and (B) several other large switch statements in the source file do have blank lines between their break/case labels already, and it seemed nice to follow this and gain readability while I was there.

Attachment 357036 [details] pushed as 6e414d4 - Entry: Fix Shift-click → extend/truncate selection

Thanks for the report!