gesture,widget: Text selection in Editor widgets

[theclapp]

• Click-and-drag to select text.
• Cut/Copy selected text via Shortcut-X/C
• Paste text via Shortcut-V
• Paste overwrites selected text
• Typing overwrites selected text
• A "select all" hotkey, Shortcut-A
• Selection via keyboard via Shift-Arrows
• Touch support
• Double-click selects a word
• Triple-click selects a line
• Shift-click selects from the caret to the clicked position, or adjusts the current selection.

[theclapp]

Force-pushed with signed commits. Appreciation to @whereswaldon in #2 (comment) for laying out how to do that.

[eliasnaur]

Haven't looked closer yet, but I'd love a test or two to demonstrate and lock in the new behaviour :)

[theclapp]

@eliasnaur I added a test case and did some other refactoring. PTAL. Thanks.

[theclapp]

Rebased from main & force-pushed.

[eliasnaur]

Sorry for the delay. I intend to review this soon, but given its complexity I'd like to release the new renderer first.

[theclapp]

There's obviously more to do for this functionality:
[ edit: feature checklist moved to PR description ]

If I have time to work on any of that, would you prefer that I just added it to this branch (probably making the change larger and presumably harder to review), or work on that in a different branch (branched from this one)? Happy to do either, but would prefer to optimize in favor of getting it merged sooner.

[eliasnaur]

Please add new work as separate commits on top of the others. The combined change will be larger, but that's ok as long as the commits are logically separate units.

[theclapp]

Rebased to current main (with the compute renderer) and force-pushed.

Also, earlier, moved the feature list from an earlier comment to the PR description as a checklist.

[eliasnaur]

Excellent work, I can't wait to have selections in Gio!

This is the first round of review comments. The comments are mainly about making Editor always have a selection (or, equivalently, adding width to the caret), and making the zero value (zero characters selected) useful in the existing operations (move, delete etc.).

[theclapp]

Pushed some new code. Marked several comments as "resolved". The rest are still outstanding.

[theclapp]

You mentioned "this is the first round of review comments", so maybe you meant to address this later, but just in case: The selection color is hardcoded to color.NRGBA{B: 0xff, A: 0x40} which is suboptimal, to say the least. But I'm not sure where the color should be specified. Any thoughts?

[eliasnaur]

You mentioned "this is the first round of review comments", so maybe you meant to address this later, but just in case: The selection color is hardcoded to color.NRGBA{B: 0xff, A: 0x40} which is suboptimal, to say the least. But I'm not sure where the color should be specified. Any thoughts?

Specifying the color is best left for the theme, which in your case probably means export paintSelection as PaintSelection, calling it from material.Editor. Just like PaintText, PaintSelection would just use op.PaintOp and not specify a color.

[theclapp]

[...] The selection color is hardcoded to color.NRGBA{B: 0xff, A: 0x40} [...]

Specifying the color is best left for the theme

I agree.

which in your case probably means export paintSelection as PaintSelection, calling it from material.Editor. Just like PaintText, PaintSelection would just use op.PaintOp and not specify a color.

I'm not sure I know how to do that. (I'm not saying it's impossible, just that I don't know how.) paintSelection calls paint.FillShape, which takes a color explicitly. And where would the color come from in the first place? Can I just add SelectionColor to EditorStyle, and default it to Theme.Palette.ContrastBg in widget/material.Editor(), and pass it to PaintText, where it's used in the paintSelection color? Also, making paintSelection more stand-alone would appear to require iterating over e.shapes again (unless I'm missing something), which seems wasteful.

[eliasnaur]

which in your case probably means export paintSelection as PaintSelection, calling it from material.Editor. Just like PaintText, PaintSelection would just use op.PaintOp and not specify a color.

I'm not sure I know how to do that. (I'm not saying it's impossible, just that I don't know how.) paintSelection calls paint.FillShape, which takes a color explicitly.

See Editor.PaintText that uses raw clip and PaintOps for painting the text with whatever material is current.

And where would the color come from in the first place? Can I just add SelectionColor to EditorStyle, and default it to Theme.Palette.ContrastBg in widget/material.Editor()

SGTM

and pass it to PaintText, where it's used in the paintSelection color? Also, making paintSelection more stand-alone would appear to require iterating over e.shapes again (unless I'm missing something), which seems wasteful.

I don't mind the waste for now; it's only iterating the visible lines of text once more. We can get clever in a follow-up, for example by storing into e.selectedShapes just the e.shapes that need selection painted (usually none), or by constructing the selection area as one large clip operation.

[theclapp]

Pushed some new code, marked some more conversations as resolved.

Still more work to be done, but getting there.

Pushing the selection-aware code down as far as it could go does in fact make the code cleaner and more streamlined. Thanks for the suggestion.

[theclapp]

I think I've addressed all comments / objections / suggestions so far. PTAL. Thank you for your feedback.

[theclapp]

I'm going to bring up the eventual commit order ...

So, for example, a set of N commits that does refactoring only, setting the stage for the selection code, possibly changing the API (e.g. changing Move to MoveCaret, and adding SetCaret), but adding no actual selection code or api, and then another set of N commits that does add the selection code, but makes no (or as few as possible) other changes.

So the first set would, for example, change func (e *Editor) Move(distance int) to func (e *Editor) MoveCaret(caretDelta int), and then the second set would change it to func (e *Editor) MoveCaret(startDelta, endDelta int).

Yes?

So ... would doing all that in place make the comments in this PR hard to read afterwards, or does GH know how to take care of all that? And if the former, maybe I should open a new PR with a new branch with the reordered / combined / split commits? Any advice? I haven't done anything like that before.

[theclapp]

Just for posterity: I suspect others might benefit from this as much as I did, which has sections on squashing, splitting, and reordering commits: https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History

[theclapp]

One of the things I'm wondering, from you (@eliasnaur) or anyone else that happens to be following along, is: would the reorganization of the commits you're requesting be easier with an interactive rebase, with (I suspect) rather a lot of commit splitting and conflict resolution, or would it be easier if I just diffed the current branch against main, and created brand new commits in a new branch, with (I suspect) way fewer problems with commit splitting and conflict resolution.

I mean, to be sure, the whole diff is 1842 lines, but at least I'd only have to go over them approximately once each, instead of quite possibly several times each. Like I said, looking for advice.

/cc @mvdan @whereswaldon if you have any advice. If not, no worries & thanks anyway. 😄

[mvdan]

Personally, I just rebase and force-push to PRs :) Whatever you do, "see older versions of this PR" is practically impossible on GitHub, so I go for the simpler approach of fewer commits.

[eliasnaur]

One of the things I'm wondering, from you (@eliasnaur) or anyone else that happens to be following along, is: would the reorganization of the commits you're requesting be easier with an interactive rebase, with (I suspect) rather a lot of commit splitting and conflict resolution, or would it be easier if I just diffed the current branch against main, and created brand new commits in a new branch, with (I suspect) way fewer problems with commit splitting and conflict resolution.

I mean, to be sure, the whole diff is 1842 lines, but at least I'd only have to go over them approximately once each, instead of quite possibly several times each. Like I said, looking for advice.

FWIW, I rarely start over from a diff to main, and use git rebase -i main almost exclusively to re-order, squash/fixup, edit commits and commit messages.

[whereswaldon]

I guess my comments are nothing new here, but I also just use git rebase -i for this sort of thing. I have attempted the kind of clean slate history reconstruction that you describe (diffing main), but it's very difficult to retroactively isolate logical changesets with that approach.

[eliasnaur]

I'm going to bring up the eventual commit order ...

So, for example, a set of N commits that does refactoring only, setting the stage for the selection code, possibly changing the API (e.g. changing Move to MoveCaret, and adding SetCaret), but adding no actual selection code or api, and then another set of N commits that does add the selection code, but makes no (or as few as possible) other changes.

Exactly (I'm shamelessly stealing the "setting the stage" metaphor). The goal is to minimize and isolate the exciting changes, to reduce review fatigue and for easier debugging in case a future bisect blames one of your commits.

So the first set would, for example, change func (e *Editor) Move(distance int) to func (e *Editor) MoveCaret(caretDelta int), and then the second set would change it to func (e *Editor) MoveCaret(startDelta, endDelta int).

Yes?

Yes.

So ... would doing all that in place make the comments in this PR hard to read afterwards, or does GH know how to take care of all that? And if the former, maybe I should open a new PR with a new branch with the reordered / combined / split commits? Any advice? I haven't done anything like that before.

Like Daniel, I don't have much hope for retaining the comment history. Even if we did retain history, there's little linking commits to the GitHub PRs. If you feel generous, please do summarize relevant history in the commit messages that end up merged.

[theclapp]

Rebased from main before big commit reorganization.

[theclapp]

I've reorganized the commits into just two, one with just refactoring, and one with just selection code, as requested. PTAL.

[eliasnaur]

Thank you for rebasing the commits ,much appreciated.

I have a single concern about SetCaret left, otherwise LGTM.

[lrewega]

Trying out the latest commit 6502c73 I noticed that using arrow keys with highlighted text does not clear the highlight, as I would expect, but instead slides the highlight, which is a behaviour I've never seen before!

[eliasnaur]

Trying out the latest commit 6502c73 I noticed that using arrow keys with highlighted text does not clear the highlight, as I would expect, but instead slides the highlight, which is a behaviour I've never seen before!

I'm seeing this too. It's an interesting effect, for sure :)

[theclapp]

using arrow keys with highlighted text does not clear the highlight, as I would expect, but instead slides the highlight

Yep. Good catch! This is only for the left & right arrows; up & down clear the selection as expected.

It was like this before the commit reorganization, too, apparently.

This is a direct result of the MoveCaret(startDelta, endDelta int) api. There's no value of endDelta you can send to extend the selection if you've pressed Shift and to clear it if you haven't. A 0 will shrink/extend the selection, a +/-1 will slide/shrink/enlarge it, depending on the sign of startDelta. (Not noticing this bug was a direct result of me always either clicking the mouse or using the up/down arrows during testing, I guess. And insufficient test cases.)

@eliasnaur, what are your thoughts? As we've discussed, we want the api to be selection-agnostic as far down as possible. The options as I see them:

1. Change MoveCaret to accept another flag that says clear or extend the selection, like the internal moveWord/moveLines/etc, and ignore endDelta if it's selectionClear. MoveCaret(startDelta, endDelta int, selAct SelectionAction).
2. Call ClearSelection before MoveCaret if Shift is pressed
3. Have two functions: MoveCaret(startDelta) which calls ClearSelection internally and then moves the caret, and MoveCaretShifted(startDelta, endDelta) (or possibly ChangeSelection or something else?) which does what MoveCaret does now
4. Pass an out-of-band value in endDelta (e.g. math.MaxInt32) that's special-cased to clear the selection.

const NoSelection = math.MaxInt32

func (e *Editor) MoveCaret(startDelta, endDelta int) {
  if endDelta == NoSelection { e.ClearSelection() }
  // ...

2-4 still require selection-specific code in the left/right arrow code, though, and also user code would have to do the same thing in a similar context.

I think I prefer option 2, with the option of changing to option 1 when/if we ever export the other movement functions (moveWord/moveLine/etc).

[eliasnaur]

using arrow keys with highlighted text does not clear the highlight, as I would expect, but instead slides the highlight

Yep. Good catch! This is only for the left & right arrows; up & down clear the selection as expected.

This is a direct result of the MoveCaret(startDelta, endDelta int) api. There's no value of endDelta you can send to extend the selection if you've pressed Shift and to clear it if you haven't. A 0 will shrink/extend the selection, a +/-1 will slide/shrink/enlarge it, depending on the sign of startDelta. (Not noticing this bug was a direct result of me always either clicking the mouse or using the up/down arrows during testing, I guess. And insufficient test cases.)

@eliasnaur, what are your thoughts? As we've discussed, we want the api to be selection-agnostic as far down as possible. The options as I see them:

2. Call ClearSelection before MoveCaret if Shift is pressed

I believe you mean when Shift is not pressed?

I think I prefer option 2

I prefer option 2 as well. I don't see a need for MoveCaret to precisely map to keyboard combinations, so a ClearSelection when moving without shift is fine.

with the option of changing to option 1 when/if we ever export the other movement functions (moveWord/moveLine/etc).

I think we can fold moveWord/Line into a generalization of MoveCaret. See my #3 (comment) sketch.

[theclapp]

I believe you mean when Shift is not pressed?

Right. Oops.

option 2

Awesome.

I think we can fold moveWord/Line into a generalization of MoveCaret

For a later PR.

[theclapp]

using arrow keys with highlighted text does not clear the highlight

Fixed. Test case added, too.

[eliasnaur]

Great work, all merged! Thank you for your patience.