~eliasnaur/gio-patches

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
5 4

[PATCH gio v3] text,widget: handle font missing from collection

Details
Message ID
<20220118005459.1918-1-kanobe@gmail.com>
DKIM signature
pass
Download raw message
Patch: +15 -0
This fix prevents a segmentation violation when a text.Cache doesn't
contain a font that is requested during layout. It also makes the
Cache fallback to returning any font from the Cache if a particular
font is not found.

Signed-off-by: Jeff Williams <kanobe@gmail.com>
---
 text/shaper.go   | 12 ++++++++++++
 widget/editor.go |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/text/shaper.go b/text/shaper.go
index 59020e07..d5d7ecdb 100644
--- a/text/shaper.go
+++ b/text/shaper.go
@@ -3,6 +3,7 @@
package text

import (
	"errors"
	"io"
	"strings"

@@ -11,6 +12,8 @@ import (
	"gioui.org/op/clip"
)

var noSuchFont = errors.New("cannot find matching font")

// Shaper implements layout and shaping of text.
type Shaper interface {
	// Layout a text according to a set of options.
@@ -63,6 +66,9 @@ func (c *Cache) faceForStyle(font Font) *faceCache {
	if closest, ok := c.closestFont(font); ok {
		return c.faces[closest]
	}
	for _, v := range c.faces {
		return v
	}
	return nil
}

@@ -110,12 +116,18 @@ func NewCache(collection []FontFace) *Cache {
// Layout implements the Shaper interface.
func (c *Cache) Layout(font Font, size fixed.Int26_6, maxWidth int, txt io.Reader) ([]Line, error) {
	cache := c.lookup(font)
	if cache == nil {
		return nil, noSuchFont
	}
	return cache.face.Layout(size, maxWidth, txt)
}

// LayoutString is a caching implementation of the Shaper interface.
func (c *Cache) LayoutString(font Font, size fixed.Int26_6, maxWidth int, str string) []Line {
	cache := c.lookup(font)
	if cache == nil {
		return nil
	}
	return cache.layout(size, maxWidth, str)
}

diff --git a/widget/editor.go b/widget/editor.go
index a9785575..b3064213 100644
--- a/widget/editor.go
+++ b/widget/editor.go
@@ -1218,6 +1218,9 @@ func (e *Editor) SetCaret(start, end int) {
func (e *Editor) makeValidCaret(positions ...*combinedPos) {
	// Jump through some hoops to order the offsets given to offsetToScreenPos,
	// but still be able to update them correctly with the results thereof.
	if e.lines == nil {
		return
	}
	positions = append(positions, &e.caret.start, &e.caret.end)
	sort.Slice(positions, func(i, j int) bool {
		return positions[i].ofs < positions[j].ofs
-- 
2.11.0

[gio/patches] build failed

builds.sr.ht <builds@sr.ht>
Details
Message ID
<CH8EIB7E7A8O.12HXHAGT9BZ4S@cirno>
In-Reply-To
<20220118005459.1918-1-kanobe@gmail.com> (view parent)
DKIM signature
missing
Download raw message
gio/patches: FAILED in 20m44s

[text,widget: handle font missing from collection][0] v3 from [Jeff Williams][1]

[0]: https://lists.sr.ht/~eliasnaur/gio-patches/patches/28434
[1]: kanobe@gmail.com

✓ #674781 SUCCESS gio/patches/openbsd.yml https://builds.sr.ht/~eliasnaur/job/674781
✗ #674779 FAILED  gio/patches/freebsd.yml https://builds.sr.ht/~eliasnaur/job/674779
✓ #674778 SUCCESS gio/patches/apple.yml   https://builds.sr.ht/~eliasnaur/job/674778
✓ #674780 SUCCESS gio/patches/linux.yml   https://builds.sr.ht/~eliasnaur/job/674780
Details
Message ID
<CH8LUET5D59D.19SQKA809V9JY@zoidberg>
In-Reply-To
<20220118005459.1918-1-kanobe@gmail.com> (view parent)
DKIM signature
fail
Download raw message
DKIM signature: fail
On Tue Jan 18, 2022 at 01:54 CET, Jeff Williams wrote:
> This fix prevents a segmentation violation when a text.Cache doesn't
> contain a font that is requested during layout. It also makes the
> Cache fallback to returning any font from the Cache if a particular
> font is not found.
>
> Signed-off-by: Jeff Williams <kanobe@gmail.com>
> ---
> text/shaper.go | 12 ++++++++++++
> widget/editor.go | 3 +++
> 2 files changed, 15 insertions(+)
>
> diff --git a/text/shaper.go b/text/shaper.go
> index 59020e07..d5d7ecdb 100644
> --- a/text/shaper.go
> +++ b/text/shaper.go
> @@ -3,6 +3,7 @@
> package text
>  
> import (
> + "errors"
> "io"
> "strings"
>  
> @@ -11,6 +12,8 @@ import (
> "gioui.org/op/clip"
> )
>  
> +var noSuchFont = errors.New("cannot find matching font")

to follow Go guidelines, I think this should be named
var errNoSuchFont

or
var errNoMatchingFont

-s
Details
Message ID
<CH8QV2HXARTS.1G198K4SFEVFL@macbog.local>
In-Reply-To
<20220118005459.1918-1-kanobe@gmail.com> (view parent)
DKIM signature
fail
Download raw message
DKIM signature: fail
On Tue Jan 18, 2022 at 01:54 CET, Jeff Williams wrote:
> This fix prevents a segmentation violation when a text.Cache doesn't
> contain a font that is requested during layout. It also makes the
> Cache fallback to returning any font from the Cache if a particular
> font is not found.
>

Thank you. One comment below. Tests for the fixed panics would be nice.

> Signed-off-by: Jeff Williams <kanobe@gmail.com>
> ---
>  text/shaper.go   | 12 ++++++++++++
>  widget/editor.go |  3 +++
>  2 files changed, 15 insertions(+)
>
> diff --git a/text/shaper.go b/text/shaper.go
> index 59020e07..d5d7ecdb 100644
> --- a/text/shaper.go
> +++ b/text/shaper.go
> @@ -11,6 +12,8 @@ import (
>  	"gioui.org/op/clip"
>  )
>  
> +var noSuchFont = errors.New("cannot find matching font")
> +

Same comment as Sebastien's.

>  // Shaper implements layout and shaping of text.
>  type Shaper interface {
>  	// Layout a text according to a set of options.
> @@ -63,6 +66,9 @@ func (c *Cache) faceForStyle(font Font) *faceCache {
>  	if closest, ok := c.closestFont(font); ok {
>  		return c.faces[closest]
>  	}
> +	for _, v := range c.faces {
> +		return v
> +	}
>  	return nil
>  }
>  
> @@ -110,12 +116,18 @@ func NewCache(collection []FontFace) *Cache {
>  // Layout implements the Shaper interface.
>  func (c *Cache) Layout(font Font, size fixed.Int26_6, maxWidth int, txt io.Reader) ([]Line, error) {
>  	cache := c.lookup(font)
> +	if cache == nil {
> +		return nil, noSuchFont
> +	}
>  	return cache.face.Layout(size, maxWidth, txt)
>  }
>  
>  // LayoutString is a caching implementation of the Shaper interface.
>  func (c *Cache) LayoutString(font Font, size fixed.Int26_6, maxWidth int, str string) []Line {
>  	cache := c.lookup(font)
> +	if cache == nil {
> +		return nil
> +	}
>  	return cache.layout(size, maxWidth, str)
>  }
>  
> diff --git a/widget/editor.go b/widget/editor.go
> index a9785575..b3064213 100644
> --- a/widget/editor.go
> +++ b/widget/editor.go
> @@ -1218,6 +1218,9 @@ func (e *Editor) SetCaret(start, end int) {
>  func (e *Editor) makeValidCaret(positions ...*combinedPos) {
>  	// Jump through some hoops to order the offsets given to offsetToScreenPos,
>  	// but still be able to update them correctly with the results thereof.
> +	if e.lines == nil {
> +		return
> +	}

Is this an optimization? If so, I don't think it's worth its weight.

>  	positions = append(positions, &e.caret.start, &e.caret.end)
>  	sort.Slice(positions, func(i, j int) bool {
>  		return positions[i].ofs < positions[j].ofs
> -- 
> 2.11.0
Details
Message ID
<CAHe4cP=242sN3-MP5KFLC0Y18=1xKxSqfweHDJWP_mGMrStfXg@mail.gmail.com>
In-Reply-To
<CH8LUET5D59D.19SQKA809V9JY@zoidberg> (view parent)
DKIM signature
pass
Download raw message
>
> to follow Go guidelines, I think this should be named
> var errNoSuchFont
>
> or
> var errNoMatchingFont
>

Okay, I'll change it to `errNoSuchFont`.
Details
Message ID
<CAHe4cPnr+U-tpg9TzMPV2TB9T=s7vMksV6srf2dgKh=4+XT+7Q@mail.gmail.com>
In-Reply-To
<CH8QV2HXARTS.1G198K4SFEVFL@macbog.local> (view parent)
DKIM signature
pass
Download raw message
>
> Thank you. One comment below. Tests for the fixed panics would be nice.
>

Sure, I'll try and write tests for it.


> > @@ -1218,6 +1218,9 @@ func (e *Editor) SetCaret(start, end int) {
> >  func (e *Editor) makeValidCaret(positions ...*combinedPos) {
> >       // Jump through some hoops to order the offsets given to offsetToScreenPos,
> >       // but still be able to update them correctly with the results thereof.
> > +     if e.lines == nil {
> > +             return
> > +     }
>
> Is this an optimization? If so, I don't think it's worth its weight.
>

Oh, it's another part of the fix. It seems that in an otherwise
functioning program,
as a consequence of not finding the requested font the editor ends up
with lines set to nil,
and later in this method it calls offsetToScreenPos which tries to read e.lines
unconditionally:

    func (e *Editor) offsetToScreenPos(offset int) (combinedPos,
func(int) combinedPos) {
      var col, line, idx int
      var x fixed.Int26_6

      l := e.lines[line]
    ...

It seemed easier to return with that sanity check from SetCaret that
has no return
value than pick a return value that offsetToScreenPos should return on failure.


> >       positions = append(positions, &e.caret.start, &e.caret.end)
> >       sort.Slice(positions, func(i, j int) bool {
> >               return positions[i].ofs < positions[j].ofs
> > --
> > 2.11.0
>
Reply to thread Export thread (mbox)