~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 2

[PATCH] app/internal/window: (X11) Add support for UTF-8 window title.

Details
Message ID
<20191112145250.31762-1-db047h@gmail.com>
DKIM signature
pass
Download raw message
Patch: +32 -3
Signed-off-by: Denis Bernard <db047h@gmail.com>
---
 app/internal/window/os_x11.go | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/app/internal/window/os_x11.go b/app/internal/window/os_x11.go
index d820360..7223199 100644
--- a/app/internal/window/os_x11.go
+++ b/app/internal/window/os_x11.go
@@ -62,6 +62,7 @@ type x11Window struct {
 	x  *C.Display
 	xw C.Window
 
+	atoms       map[string]C.Atom
 	evDelWindow C.Atom
 	stage       system.Stage
 	cfg         config
@@ -198,6 +199,26 @@ func (w *x11Window) destroy() {
 	C.XCloseDisplay(w.x)
 }
 
+// atom is a wrapper around XInternAtom that caches the results in order to
+// limit conversions/allocations from Go strings to C.
+//
+func (w *x11Window) atom(name string, onlyIfExists bool) C.Atom {
+	if a, ok := w.atoms[name]; ok {
+		return a
+	}
+	cname := C.CString(name)
+	flag := C.Bool(C.False)
+	if onlyIfExists {
+		flag = C.True
+	}
+	a := C.XInternAtom(w.x, cname, flag)
+	C.free(unsafe.Pointer(cname))
+	if a != C.None {
+		w.atoms[name] = a
+	}
+	return a
+}
+
 // x11EventHandler wraps static variables for the main event loop.
 // Its sole purpose is to prevent heap allocation and reduce clutter
 // in x11window.loop.
@@ -503,6 +524,7 @@ func newX11Window(gioWin Callbacks, opts *Options) error {
 		w: gioWin, x: dpy, xw: win,
 		width:  cfg.Px(opts.Width),
 		height: cfg.Px(opts.Height),
+		atoms:  make(map[string]C.Atom),
 		cfg:    cfg,
 		xim:    xim,
 		xic:    xic,
@@ -521,12 +543,19 @@ func newX11Window(gioWin Callbacks, opts *Options) error {
 	// set the name
 	ctitle := C.CString(opts.Title)
 	C.XStoreName(dpy, win, ctitle)
+	// set _NET_WM_NAME as well for UTF-8 support in window title.
+	C.XSetTextProperty(dpy, win,
+		&C.XTextProperty{
+			value:    (*C.uchar)(unsafe.Pointer(ctitle)),
+			encoding: w.atom("UTF8_STRING", false),
+			format:   8,
+			nitems:   C.ulong(len(opts.Title)),
+		},
+		w.atom("_NET_WM_NAME", false))
 	C.free(unsafe.Pointer(ctitle))
 
 	// extensions
-	ckey := C.CString("WM_DELETE_WINDOW")
-	w.evDelWindow = C.XInternAtom(dpy, ckey, C.False)
-	C.free(unsafe.Pointer(ckey))
+	w.evDelWindow = w.atom("WM_DELETE_WINDOW", false)
 	C.XSetWMProtocols(dpy, win, &w.evDelWindow, 1)
 
 	go func() {
-- 
2.17.1
Details
Message ID
<BYE18A86GLIB.2870VCKLTAOIL@themachine>
In-Reply-To
<20191112145250.31762-1-db047h@gmail.com> (view parent)
DKIM signature
pass
Download raw message
On Tue Nov 12, 2019 at 3:52 PM Denis Bernard wrote:
> Signed-off-by: Denis Bernard <db047h@gmail.com>
> ---
>  app/internal/window/os_x11.go | 35 ++++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/app/internal/window/os_x11.go b/app/internal/window/os_x11.go
> index d820360..7223199 100644
> --- a/app/internal/window/os_x11.go
> +++ b/app/internal/window/os_x11.go
> @@ -62,6 +62,7 @@ type x11Window struct {
>  	x  *C.Display
>  	xw C.Window
>  
> +	atoms       map[string]C.Atom
>  	evDelWindow C.Atom
>  	stage       system.Stage
>  	cfg         config
> @@ -198,6 +199,26 @@ func (w *x11Window) destroy() {
>  	C.XCloseDisplay(w.x)
>  }
>  
> +// atom is a wrapper around XInternAtom that caches the results in order to
> +// limit conversions/allocations from Go strings to C.
> +//

I don't think caching is worth it to avoid C allocations. However, it might be
worth it if XInternAtom incurs a roundtrip to the server. Does it? If so, does
Xlib cache atoms for us? If not, state that in the documentation.

> +func (w *x11Window) atom(name string, onlyIfExists bool) C.Atom {
> +	if a, ok := w.atoms[name]; ok {
> +		return a
> +	}
> +	cname := C.CString(name)

defer C.free immediately after this.

> +	flag := C.Bool(C.False)
> +	if onlyIfExists {
> +		flag = C.True
> +	}
> +	a := C.XInternAtom(w.x, cname, flag)
> +	C.free(unsafe.Pointer(cname))
> +	if a != C.None {

If caching is worth it, why is a negative cache not?

> @@ -503,6 +524,7 @@ func newX11Window(gioWin Callbacks, opts *Options) error {
>  		w: gioWin, x: dpy, xw: win,
>  		width:  cfg.Px(opts.Width),
>  		height: cfg.Px(opts.Height),
> +		atoms:  make(map[string]C.Atom),
>  		cfg:    cfg,
>  		xim:    xim,
>  		xic:    xic,
> @@ -521,12 +543,19 @@ func newX11Window(gioWin Callbacks, opts *Options) error {
>  	// set the name
>  	ctitle := C.CString(opts.Title)

defer C.free immediately after CString.
Details
Message ID
<CAE+P9PMs24MaaFcaUndG3XA-ZXfJVXX4gAwbpagZw1-wG-96tA@mail.gmail.com>
In-Reply-To
<BYE18A86GLIB.2870VCKLTAOIL@themachine> (view parent)
DKIM signature
pass
Download raw message
On Tue, Nov 12, 2019 at 4:44 PM Elias Naur <mail@eliasnaur.com> wrote:
>
> On Tue Nov 12, 2019 at 3:52 PM Denis Bernard wrote:
> > Signed-off-by: Denis Bernard <db047h@gmail.com>
> > ---
> >  app/internal/window/os_x11.go | 35 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 32 insertions(+), 3 deletions(-)
> >
> > diff --git a/app/internal/window/os_x11.go b/app/internal/window/os_x11.go
> > index d820360..7223199 100644
> > --- a/app/internal/window/os_x11.go
> > +++ b/app/internal/window/os_x11.go
> > @@ -62,6 +62,7 @@ type x11Window struct {
> >       x  *C.Display
> >       xw C.Window
> >
> > +     atoms       map[string]C.Atom
> >       evDelWindow C.Atom
> >       stage       system.Stage
> >       cfg         config
> > @@ -198,6 +199,26 @@ func (w *x11Window) destroy() {
> >       C.XCloseDisplay(w.x)
> >  }
> >
> > +// atom is a wrapper around XInternAtom that caches the results in order to
> > +// limit conversions/allocations from Go strings to C.
> > +//
>
> I don't think caching is worth it to avoid C allocations. However, it might be
> worth it if XInternAtom incurs a roundtrip to the server. Does it? If so, does
> Xlib cache atoms for us? If not, state that in the documentation.

XinternAtom does a round-trip.
https://www.x.org/releases/X11R7.5/doc/man/man3/XInternAtom.3.html
As for caching in xlib, I don't know but from what I've seen,
applications do their own caching (by saving the atoms in variables
with a name similar to the atom name).
I also did it that way because the vsync update requires tons of
atoms. I'll update the function comment.

>
> > +func (w *x11Window) atom(name string, onlyIfExists bool) C.Atom {
> > +     if a, ok := w.atoms[name]; ok {
> > +             return a
> > +     }
> > +     cname := C.CString(name)
>
> defer C.free immediately after this.

I'd rather not do it here. atom() could be called in a hot code path,
so if I can avoid a defer to C code...

>
> > +     flag := C.Bool(C.False)
> > +     if onlyIfExists {
> > +             flag = C.True
> > +     }
> > +     a := C.XInternAtom(w.x, cname, flag)
> > +     C.free(unsafe.Pointer(cname))
> > +     if a != C.None {
>
> If caching is worth it, why is a negative cache not?

Because it may happen that we query for an atom set by some other
application (or even ourselves) at a later point in time.

>
> > @@ -503,6 +524,7 @@ func newX11Window(gioWin Callbacks, opts *Options) error {
> >               w: gioWin, x: dpy, xw: win,
> >               width:  cfg.Px(opts.Width),
> >               height: cfg.Px(opts.Height),
> > +             atoms:  make(map[string]C.Atom),
> >               cfg:    cfg,
> >               xim:    xim,
> >               xic:    xic,
> > @@ -521,12 +543,19 @@ func newX11Window(gioWin Callbacks, opts *Options) error {
> >       // set the name
> >       ctitle := C.CString(opts.Title)
>
> defer C.free immediately after CString.
>

Will do
Details
Message ID
<BYE648P4YL8G.1GURL2P3TOGYN@testmac>
In-Reply-To
<CAE+P9PMs24MaaFcaUndG3XA-ZXfJVXX4gAwbpagZw1-wG-96tA@mail.gmail.com> (view parent)
DKIM signature
pass
Download raw message
> >
> > > +func (w *x11Window) atom(name string, onlyIfExists bool) C.Atom {
> > > +     if a, ok := w.atoms[name]; ok {
> > > +             return a
> > > +     }
> > > +     cname := C.CString(name)
> >
> > defer C.free immediately after this.
> 
> I'd rather not do it here. atom() could be called in a hot code path,
> so if I can avoid a defer to C code...
> 

You're only free'ing if the atom wasn't in the cache, in which case
we're doing a malloc and a roundtrip. A free is a drop in the bucket in
that case.

Besides, with Go 1.14's open coded defers, avoiding defers should almost
never be necessary.
Details
Message ID
<CAE+P9PMXXG6ebfrM7RrYov6GyDA5-or99Zy2tyyZ-50PfCrLwA@mail.gmail.com>
In-Reply-To
<BYE648P4YL8G.1GURL2P3TOGYN@testmac> (view parent)
DKIM signature
pass
Download raw message
On Tue, Nov 12, 2019 at 8:33 PM Elias Naur <mail@eliasnaur.com> wrote:
>
> > >
> > > > +func (w *x11Window) atom(name string, onlyIfExists bool) C.Atom {
> > > > +     if a, ok := w.atoms[name]; ok {
> > > > +             return a
> > > > +     }
> > > > +     cname := C.CString(name)
> > >
> > > defer C.free immediately after this.
> >
> > I'd rather not do it here. atom() could be called in a hot code path,
> > so if I can avoid a defer to C code...
> >
>
> You're only free'ing if the atom wasn't in the cache, in which case
> we're doing a malloc and a roundtrip. A free is a drop in the bucket in
> that case.

Seen from that angle, you're totally right!

>
> Besides, with Go 1.14's open coded defers, avoiding defers should almost
> never be necessary.

I wasn't aware of that, but with CGo's inability to inline C calls, I
don't know if this will really help here. I'll just defer the call
anyway.

This atom cache may seem like a big machinery for a handful of atoms,
but it's a great help right now in implementing frame sync with the
Window Manager. I must however admit that it may not even stay after
we have a working implementation and a first pass of refactoring. If
you prefer, I can use a simpler approach for this UTF8 patch.

-- denis
Details
Message ID
<BYE8GGCMLKG0.2BGL0XW2W2F4D@testmac>
In-Reply-To
<CAE+P9PMXXG6ebfrM7RrYov6GyDA5-or99Zy2tyyZ-50PfCrLwA@mail.gmail.com> (view parent)
DKIM signature
pass
Download raw message
On Tue Nov 12, 2019 at 9:14 PM Denis Bernard wrote:
> On Tue, Nov 12, 2019 at 8:33 PM Elias Naur <mail@eliasnaur.com> wrote:
> >
> > > >
> > > > > +func (w *x11Window) atom(name string, onlyIfExists bool) C.Atom {
> > > > > +     if a, ok := w.atoms[name]; ok {
> > > > > +             return a
> > > > > +     }
> > > > > +     cname := C.CString(name)
> > > >
> > > > defer C.free immediately after this.
> > >
> > > I'd rather not do it here. atom() could be called in a hot code path,
> > > so if I can avoid a defer to C code...
> > >
> >
> > You're only free'ing if the atom wasn't in the cache, in which case
> > we're doing a malloc and a roundtrip. A free is a drop in the bucket in
> > that case.
> 
> Seen from that angle, you're totally right!
> 
> >
> > Besides, with Go 1.14's open coded defers, avoiding defers should almost
> > never be necessary.
> 
> I wasn't aware of that, but with CGo's inability to inline C calls, I
> don't know if this will really help here. I'll just defer the call
> anyway.
> 
> This atom cache may seem like a big machinery for a handful of atoms,
> but it's a great help right now in implementing frame sync with the
> Window Manager. I must however admit that it may not even stay after
> we have a working implementation and a first pass of refactoring. If
> you prefer, I can use a simpler approach for this UTF8 patch.
> 

I leave that decision to you. My only concern was caching to avoid Cgo,
which is overkill. Caching because of X roundtrips is fine.