~eliasnaur/gio-patches

3 2

[PATCH] widget: make editor skip words with key modifier

Details
Message ID
<EA941F9F-77D0-4946-974C-5C7D22E4A7FD@getmailspring.com>
DKIM signature
pass
Download raw message
From ee7af9688385dcc66133c3b2a1aa17361bdb7cc5 Mon Sep 17 00:00:00 2001
From: Jack Mordaunt <jackmordaunt@gmail.com>
Date: Wed, 16 Sep 2020 14:05:26 +0800
Subject: [PATCH] widget: make editor skip words with key modifier

---
 widget/editor.go      | 60 +++++++++++++++++++++++++++++++++++++++++--
 widget/editor_test.go | 49 +++++++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+), 2 deletions(-)

Make editor skip words with key modifier (default ModCtrl, darwin ModAlt).
Added test harness for the `moveWord` method specifically, as well as
adding a call in the consistency test.
Behaviour modelled on common text editors: eats initial whitespace,
then jumps to the end of the word.

diff --git a/widget/editor.go b/widget/editor.go
index 7e8fddb..7abf553 100644
--- a/widget/editor.go
+++ b/widget/editor.go
@@ -7,8 +7,10 @@ import (
 	"image"
 	"io"
 	"math"
+	"runtime"
 	"strings"
 	"time"
+	"unicode"
 	"unicode/utf8"
 
 	"gioui.org/f32"
@@ -266,6 +268,12 @@ func (e *Editor) moveLines(distance int) {
 }
 
 func (e *Editor) command(k key.Event) bool {
+	var modSkip = func() key.Modifiers {
+		if runtime.GOOS == "darwin" {
+			return key.ModAlt
+		}
+		return key.ModCtrl
+	}()
 	switch k.Name {
 	case key.NameReturn, key.NameEnter:
 		e.append("\n")
@@ -278,9 +286,17 @@ func (e *Editor) command(k key.Event) bool {
 	case key.NameDownArrow:
 		e.moveLines(+1)
 	case key.NameLeftArrow:
-		e.Move(-1)
+		if k.Modifiers.Contain(modSkip) {
+			e.moveWord(-1)
+		} else {
+			e.Move(-1)
+		}
 	case key.NameRightArrow:
-		e.Move(1)
+		if k.Modifiers.Contain(modSkip) {
+			e.moveWord(1)
+		} else {
+			e.Move(1)
+		}
 	case key.NamePageUp:
 		e.movePages(-1)
 	case key.NamePageDown:
@@ -775,6 +791,46 @@ func (e *Editor) moveEnd() {
 	e.caret.xoff = l.Width + a - e.caret.x
 }
 
+// moveWord moves the caret to the next word in the specified direction.
+// Positive is forward, negative is backward.
+// Absolute values greater than one will skip that many words.
+func (e *Editor) moveWord(distance int) {
+	e.makeValid()
+	var (
+		// split the distance information into constituent parts to be
+		// used independently.
+		words, direction = func(n int) (int, int) {
+			if n < 0 {
+				return n * -1, -1
+			} else {
+				return n, 1
+			}
+		}(distance)
+		// atEnd if caret is at either side of the buffer.
+		atEnd = func() bool {
+			return e.rr.caret == 0 || e.rr.caret == e.rr.len()
+		}
+		// next returns the appropriate rune given the direction.
+		next = func() (r rune) {
+			if direction < 0 {
+				r, _ = e.rr.runeBefore(e.rr.caret)
+			} else {
+				r, _ = e.rr.runeAt(e.rr.caret)
+			}
+			return r
+		}
+	)
+	for ii := 0; ii < words; ii++ {
+		for r := next(); unicode.IsSpace(r) && !atEnd(); r = next() {
+			e.Move(direction)
+		}
+		e.Move(direction)
+		for r := next(); !unicode.IsSpace(r) && !atEnd(); r = next() {
+			e.Move(direction)
+		}
+	}
+}
+
 func (e *Editor) scrollToCaret() {
 	e.makeValid()
 	l := e.lines[e.caret.line]
diff --git a/widget/editor_test.go b/widget/editor_test.go
index 7f51297..71b4590 100644
--- a/widget/editor_test.go
+++ b/widget/editor_test.go
@@ -91,6 +91,7 @@ const (
 	moveStart
 	moveEnd
 	moveCoord
+	moveWord
 	moveLast // Mark end; never generated.
 )
 
@@ -140,6 +141,8 @@ func TestEditorCaretConsistency(t *testing.T) {
 				e.moveEnd()
 			case moveCoord:
 				e.moveCoord(image.Pt(int(x), int(y)))
+			case moveWord:
+				e.moveWord(int(distance))
 			default:
 				return false
 			}
@@ -155,6 +158,52 @@ func TestEditorCaretConsistency(t *testing.T) {
 	}
 }
 
+func TestEditorMoveWord(t *testing.T) {
+	type Test struct {
+		Text  string
+		Start int
+		Skip  int
+		Want  int
+	}
+	tests := []Test{
+		{"", 0, 0, 0},
+		{"", 0, -1, 0},
+		{"", 0, 1, 0},
+		{"hello", 0, -1, 0},
+		{"hello", 0, 1, 5},
+		{"hello world", 3, 1, 5},
+		{"hello world", 3, -1, 0},
+		{"hello world", 8, -1, 6},
+		{"hello world", 8, 1, 11},
+		{"hello    world", 3, 1, 5},
+		{"hello    world", 3, 2, 14},
+		{"hello    world", 8, 1, 14},
+		{"hello    world", 8, -1, 0},
+		{"hello brave new world", 0, 3, 15},
+	}
+	setup := func(t string) *Editor {
+		e := new(Editor)
+		gtx := layout.Context{
+			Ops:         new(op.Ops),
+			Constraints: layout.Exact(image.Pt(100, 100)),
+		}
+		cache := text.NewCache(gofont.Collection())
+		fontSize := unit.Px(10)
+		font := text.Font{}
+		e.SetText(t)
+		e.Layout(gtx, cache, font, fontSize)
+		return e
+	}
+	for ii, tt := range tests {
+		e := setup(tt.Text)
+		e.Move(tt.Start)
+		e.moveWord(tt.Skip)
+		if e.rr.caret != tt.Want {
+			t.Fatalf("[%d] moveWord: bad caret position: got %d, want %d", ii,
e.rr.caret, tt.Want)
+		}
+	}
+}
+
 func TestEditorNoLayout(t *testing.T) {
 	var e Editor
 	e.SetText("hi!\n")
-- 
2.28.0.windows.1
Details
Message ID
<C5OMNW3ARMGO.3U8MK0GWI4GGO@testmac>
In-Reply-To
<EA941F9F-77D0-4946-974C-5C7D22E4A7FD@getmailspring.com> (view parent)
DKIM signature
pass
Download raw message
`git am` complains that the patch is corrupted. Are you sending the mail
through `git send-email`? If not, you can use the "Prepare a patchset"
web-flow from the frontpage of a personal git.sr.ht fork of gio.

A few comments below.

On Wed Sep 16, 2020 at 4:09 PM CEST, Jack Mordaunt wrote:
> >From ee7af9688385dcc66133c3b2a1aa17361bdb7cc5 Mon Sep 17 00:00:00 2001
> From: Jack Mordaunt <jackmordaunt@gmail.com>
> Date: Wed, 16 Sep 2020 14:05:26 +0800
> Subject: [PATCH] widget: make editor skip words with key modifier
>
> ---
>  widget/editor.go      | 60 +++++++++++++++++++++++++++++++++++++++++--
>  widget/editor_test.go | 49 +++++++++++++++++++++++++++++++++++
>  2 files changed, 107 insertions(+), 2 deletions(-)
>
> Make editor skip words with key modifier (default ModCtrl, darwin ModAlt).
> Added test harness for the `moveWord` method specifically, as well as
> adding a call in the consistency test.
> Behaviour modelled on common text editors: eats initial whitespace,
> then jumps to the end of the word.
>
> diff --git a/widget/editor.go b/widget/editor.go
> index 7e8fddb..7abf553 100644
> --- a/widget/editor.go
> +++ b/widget/editor.go
> @@ -7,8 +7,10 @@ import (
>  	"image"
>  	"io"
>  	"math"
> +	"runtime"
>  	"strings"
>  	"time"
> +	"unicode"
>  	"unicode/utf8"
>  
>  	"gioui.org/f32"
> @@ -266,6 +268,12 @@ func (e *Editor) moveLines(distance int) {
>  }
>  
>  func (e *Editor) command(k key.Event) bool {
> +	var modSkip = func() key.Modifiers {
> +		if runtime.GOOS == "darwin" {
> +			return key.ModAlt
> +		}
> +		return key.ModCtrl
> +	}()

A closure seems overkill. Why not

	modSkip := key.ModCtrl
	if runtime.GOOS == "darwin" {
		modSkip = key.ModAlt
	}

?

>  	switch k.Name {
>  	case key.NameReturn, key.NameEnter:
>  		e.append("\n")
> @@ -278,9 +286,17 @@ func (e *Editor) command(k key.Event) bool {
>  	case key.NameDownArrow:
>  		e.moveLines(+1)
>  	case key.NameLeftArrow:
> -		e.Move(-1)
> +		if k.Modifiers.Contain(modSkip) {

I'd expect

	k.Modifiers == modSkip

because the presence of other modifiers may mean other
things. For example, Alt+Ctrl+Arrow gives a "beep" from
Firefox on my macOS, and no other action.

> +			e.moveWord(-1)
> +		} else {
> +			e.Move(-1)
> +		}
>  	case key.NameRightArrow:
> -		e.Move(1)
> +		if k.Modifiers.Contain(modSkip) {

Same.

> +			e.moveWord(1)
> +		} else {
> +			e.Move(1)
> +		}
>  	case key.NamePageUp:
>  		e.movePages(-1)
>  	case key.NamePageDown:
> @@ -775,6 +791,46 @@ func (e *Editor) moveEnd() {
>  	e.caret.xoff = l.Width + a - e.caret.x
>  }
>  
> +// moveWord moves the caret to the next word in the specified direction.
> +// Positive is forward, negative is backward.
> +// Absolute values greater than one will skip that many words.
> +func (e *Editor) moveWord(distance int) {
> +	e.makeValid()
> +	var (
> +		// split the distance information into constituent parts to be
> +		// used independently.
> +		words, direction = func(n int) (int, int) {
> +			if n < 0 {
> +				return n * -1, -1
> +			} else {
> +				return n, 1
> +			}
> +		}(distance)

Again, the closure seems overkill. I suggest just

	words, direction := n, 1
	if distance < 0 {
		words, direction = n * -1, -1
	}

> +		// atEnd if caret is at either side of the buffer.
> +		atEnd = func() bool {
> +			return e.rr.caret == 0 || e.rr.caret == e.rr.len()
> +		}
> +		// next returns the appropriate rune given the direction.
> +		next = func() (r rune) {
> +			if direction < 0 {
> +				r, _ = e.rr.runeBefore(e.rr.caret)
> +			} else {
> +				r, _ = e.rr.runeAt(e.rr.caret)
> +			}
> +			return r
> +		}
> +	)
> +	for ii := 0; ii < words; ii++ {
> +		for r := next(); unicode.IsSpace(r) && !atEnd(); r = next() {
> +			e.Move(direction)
> +		}
> +		e.Move(direction)
> +		for r := next(); !unicode.IsSpace(r) && !atEnd(); r = next() {
> +			e.Move(direction)
> +		}
> +	}
> +}
> +
>  func (e *Editor) scrollToCaret() {
>  	e.makeValid()
>  	l := e.lines[e.caret.line]
> diff --git a/widget/editor_test.go b/widget/editor_test.go
> index 7f51297..71b4590 100644
> --- a/widget/editor_test.go
> +++ b/widget/editor_test.go
> @@ -155,6 +158,52 @@ func TestEditorCaretConsistency(t *testing.T) {
>  	}
>  }
>  
> +func TestEditorMoveWord(t *testing.T) {

Thank you.
Details
Message ID
<8E0221B3-7AFD-4D37-86B1-BAF979BA967E@getmailspring.com>
In-Reply-To
<C5OMNW3ARMGO.3U8MK0GWI4GGO@testmac> (view parent)
DKIM signature
pass
Download raw message
I'm happy to make the changes.

The "corruption" may be due to pasting the .patch into my email client
(which is in plaintext mode, so idk why it's not happy). I'll use `git
send-email` next time. 

In regards to your comments, the closure initialisation is a stylistic
choice, as far as I know Go inlines the closures so it's moot with
regards to performance. To me it's cleaner but I understand if that's
not a shared opinion. I'll defer to you. 

> I'd expect
> 
> k.Modifiers == modSkip
> 
> because the presence of other modifiers may mean other
> things. For example, Alt+Ctrl+Arrow gives a "beep" from
> Firefox on my macOS, and no other action.

That was just my ignorance about the code, equality does seem like a
better choice!
Details
Message ID
<C5ORWAJ1XWIW.17PRQTBOJ9KK4@testmac>
In-Reply-To
<8E0221B3-7AFD-4D37-86B1-BAF979BA967E@getmailspring.com> (view parent)
DKIM signature
pass
Download raw message
On Wed Sep 16, 2020 at 8:34 PM CEST, Jack Mordaunt wrote:
> I'm happy to make the changes.
>
> The "corruption" may be due to pasting the .patch into my email client
> (which is in plaintext mode, so idk why it's not happy). I'll use `git
> send-email` next time. 
>

Great, thanks. Sorry for the trouble.

> In regards to your comments, the closure initialisation is a stylistic
> choice, as far as I know Go inlines the closures so it's moot with
> regards to performance. To me it's cleaner but I understand if that's
> not a shared opinion. I'll defer to you. 
>

If it's all the same to you, I prefer the shorter versions without
closures.  Thanks.

Elias
Export thread (mbox)