Rewrite the xcursor caching code #3745

psychon · 2022-12-04T08:02:50Z

Previously, the cache for xcursors consisted of a static array. We had a hardcoded list of supported cursor names and each one got assigned a position in this array. This hardcoded list means that one cannot simply use whatever cursor a cursor theme happens to provide, since the name needs to be in our list.

In this commit, the code is rewritten. Instead of a hardcoded list, a sorted array is now used as the cache, as provided by the BARRAY code from common/util.h. The array is sorted by the name of the cursor.

This change implies an API change for the xcursor code. Previously, one had to first translate a cursor name into a cache index (xcursor_font_fromstr()) and could then use this to actually get the cursor (xcursor_new()). With this commit, xcursor_new() is the only function provided by the cursor code and it directly gets a string as argument.

Signed-off-by: Uli Schlachter psychon@znc.in

This PR is related to #3737. I am 80% sure that this PR implements (half of) that feature request by making it possible to use arbitrary cursors, as long as xcb-util-cursor can load a cursor by that name.

@akinokonomi Are you in a position to test this? (=Can you build awesome from a git branch and verify whether this does what you want?)

Personally, I only started awesome via tests/run.sh /dev/null and started xterm from the menu. The "waiting for startup" cursor briefly appeared. Nothing more was tested.

Previously, the cache for xcursors consisted of a static array. We had a hardcoded list of supported cursor names and each one got assigned a position in this array. This hardcoded list means that one cannot simply use whatever cursor a cursor theme happens to provide, since the name needs to be in our list. In this commit, the code is rewritten. Instead of a hardcoded list, a sorted array is now used as the cache, as provided by the BARRAY code from common/util.h. The array is sorted by the name of the cursor. This change implies an API change for the xcursor code. Previously, one had to first translate a cursor name into a cache index (xcursor_font_fromstr()) and could then use this to actually get the cursor (xcursor_new()). With this commit, xcursor_new() is the only function provided by the cursor code and it directly gets a string as argument. Signed-off-by: Uli Schlachter <psychon@znc.in>

codecov · 2022-12-04T08:12:57Z

Codecov Report

Merging #3745 (ec3901a) into master (e281fa3) will increase coverage by 0.00%.
The diff coverage is 90.00%.

@@           Coverage Diff           @@
##           master    #3745   +/-   ##
=======================================
  Coverage   90.99%   90.99%           
=======================================
  Files         900      900           
  Lines       57499    57498    -1     
=======================================
  Hits        52319    52319           
+ Misses       5180     5179    -1

Flag	Coverage Δ
gcov	`90.99% <90.00%> (+<0.01%)`	⬆️
luacov	`93.70% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
globalconf.h	`100.00% <ø> (ø)`
objects/drawin.c	`88.27% <33.33%> (+0.32%)`	⬆️
common/xcursor.c	`100.00% <100.00%> (ø)`
mousegrabber.c	`80.64% <100.00%> (-0.61%)`	⬇️
root.c	`87.84% <100.00%> (ø)`
lib/wibox/widget/imagebox.lua	`95.53% <0.00%> (-0.90%)`	⬇️
lib/awful/widget/layoutlist.lua	`89.28% <0.00%> (-0.60%)`	⬇️
tests/test-screenshot.lua	`97.21% <0.00%> (-0.35%)`	⬇️
spawn.c	`86.16% <0.00%> (+0.06%)`	⬆️
tests/test-input-binding.lua	`99.50% <0.00%> (+2.00%)`	⬆️

psychon · 2022-12-04T08:40:18Z

Nothing more was tested.

I had an idea!

diff --git a/awesomerc.lua b/awesomerc.lua
index c598a3e08..a5cad2eed 100644
--- a/awesomerc.lua
+++ b/awesomerc.lua
@@ -191,6 +191,7 @@ screen.connect_signal("request::desktop_decoration", function(s)
         position = "top",
         screen   = s,
         -- @DOC_SETUP_WIDGETS@
+        cursor = "foobar",
         widget   = {
             layout = wibox.layout.align.horizontal,
             { -- Left widgets

results in

2022-12-04 09:38:34 W: awesome: xerror:1058: X error: request=CreateGlyphCursor (major 94, minor 0), error=BadValue (2)
2022-12-04 09:38:34 W: awesome: xerror:1058: X error: request=ChangeWindowAttributes (major 2, minor 0), error=BadCursor (6)
2022-12-04 09:38:34 W: awesome: xerror:1058: X error: request=ChangeWindowAttributes (major 2, minor 0), error=BadCursor (6)

Checking ls /usr/share/icons/*/cursors, I found dot_box_mask. That one works (and IMO looks confusing, but whatever...).

Will investigate the above X11 errors and report back.

Edit: Excerpt from xtrace output:

000:<:0011: 20: Request(45): OpenFont fid=0x00600001 name='cursor'
000:<:0139: 32: Request(94): CreateGlyphCursor cid=0x00600011 source-font=0x00600001 mask-font=0x00600001 source-char=0xffff mask-char=0x0000 fore-red=0x0000 fore-green=0x0000 fore-blue=0x0000 back-red=0xffff back-green=0xffff back-blue=0xffff
000:>:0139:Error 2=Value: major=94, minor=0, bad=0x0000ffff, seq=0139

Looks a bit more like a problem in xcb-util-cursor...

Edit: Now this looks like a mis-compilation?! We are running this code: https://gitlab.freedesktop.org/xorg/lib/libxcb-cursor/-/blob/3d7e713e85af18d7e52cafdc9d20a2715048dee7/cursor/load_cursor.c#L209-211
gdb says:

(gdb) finish
Run till exit from #0  cursor_shape_to_id (name=name@entry=0x5555555f5ad8 "foobar") at shape_to_id.gperf:84
0x00007ffff7a030e5 in xcb_cursor_load_cursor (c=c@entry=0x5555555c6610, name=name@entry=0x5555555f5ad8 "foobar") at load_cursor.c:209
209	            core_char = cursor_shape_to_id(name);
1: fd = <optimized out>
2: core_char = -1
Value returned is $8 = -1
(gdb) next
211	        cid = xcb_generate_id(c->conn);
1: fd = <optimized out>
2: core_char = -1

You see that cursor_shape_to_id() returned -1 and that core_char was set to -1. Yet, if (core_char == -1) return XCB_NONE; is not executed and instead a cursor from the "cursor" font is created. The rest is -1 converted to uint16_t...

Edit: And if I ask gdb to disassemble the code, I get this:

   0x00007ffff7a030e0 <+64>:	call   0x7ffff7a02650 <cursor_shape_to_id>
=> 0x00007ffff7a030e5 <+69>:	mov    %eax,-0x48(%rbp)
   0x00007ffff7a030e8 <+72>:	mov    (%rbx),%rdi
   0x00007ffff7a030eb <+75>:	call   0x7ffff7a01990 <xcb_generate_id@plt>

The check against -1 seems to be quite gone.

psychon · 2022-12-04T09:13:32Z

Ahhh, debian is using an old (ancient?) version of libxcb-cursor. The bug I found above was fixed 7 years ago: https://gitlab.freedesktop.org/xorg/lib/libxcb-cursor/-/commit/cf26479ece9ab9e04616bc10ba674d88a284e5b0

Edit: Reported as https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1025409

akinokonomi · 2022-12-04T19:27:28Z

Are you in a position to test this? (=Can you build awesome from a git branch and verify whether this does what you want?)

Certainly.

Compiled against LuaJIT 2.1 beta3, xcb-util-cursor 0.1.4.

Everything looks good. Did not notice any issue so far.

And after changing the startup cursor like this:

diff --git a/lib/awful/startup_notification.lua b/lib/awful/startup_notification.lua
index 9bc95236c..45ad9c639 100644
--- a/lib/awful/startup_notification.lua
+++ b/lib/awful/startup_notification.lua
@@ -18,7 +18,7 @@ local beautiful = require("beautiful")
 
 local app_starting = {}
 
-local cursor_waiting = "watch"
+local cursor_waiting = "left_ptr_watch"
 
 --- Show busy mouse cursor during spawn.
 -- @beautiful beautiful.enable_spawn_cursor

It does exactly what I want. Thank you!

Please let me know if I have to test something else.

psychon · 2022-12-04T20:37:42Z

Thanks a lot for testing this!

Aire-One

It was a mildly challenging but interesting review since I haven't had read common/array.h before. Thank you, @psychon, for this PR.

psychon · 2022-12-09T18:39:27Z

since I haven't had read common/array.h before.

Don't go there! It is dark magic that wasn't touched in ages! ;-)

But yeah, that file is actually used a lot in the C code to manage lists, but I guess one seldomly has to actually interact with this file.

(Latest commit is from 2016 by yours truly and it removes a feature. Before that, there were one commit in 2015, 2014, 2011, 2010. I guess this must mean that this is good code that doesn't need any touching...)

actionless · 2023-08-26T01:04:35Z

this would require manual merging because of codecov

actionless approved these changes Dec 4, 2022

View reviewed changes

Aire-One approved these changes Dec 9, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite the xcursor caching code #3745

Rewrite the xcursor caching code #3745

psychon commented Dec 4, 2022

codecov bot commented Dec 4, 2022

psychon commented Dec 4, 2022 •

edited

psychon commented Dec 4, 2022 •

edited

akinokonomi commented Dec 4, 2022

psychon commented Dec 4, 2022

Aire-One left a comment

psychon commented Dec 9, 2022

actionless commented Aug 26, 2023

Rewrite the xcursor caching code #3745

Are you sure you want to change the base?

Rewrite the xcursor caching code #3745

Conversation

psychon commented Dec 4, 2022

codecov bot commented Dec 4, 2022

Codecov Report

psychon commented Dec 4, 2022 • edited

psychon commented Dec 4, 2022 • edited

akinokonomi commented Dec 4, 2022

psychon commented Dec 4, 2022

Aire-One left a comment

Choose a reason for hiding this comment

psychon commented Dec 9, 2022

actionless commented Aug 26, 2023

psychon commented Dec 4, 2022 •

edited

psychon commented Dec 4, 2022 •

edited