awesomeWM / awesome

awesome window manager
https://awesomewm.org/
GNU General Public License v2.0
6.41k stars 598 forks source link

[Feature request] Use `left_ptr_watch` for a startup notification #3737

Open akinokonomi opened 2 years ago

akinokonomi commented 2 years ago

Output of awesome --version: awesome v4.3-1551-g5077c8381-dirty (Too long)

Hello!

The reason for this is that I use Fuchsia cursor theme, and here is what watch vs. left_ptr_watch look like:

image

The latter one looks much more seamless to me.

I tried to patch it myself to submit a PR:

Click here ```diff diff --git a/common/xcursor.c b/common/xcursor.c index c4c691df5..356d3c68e 100644 --- a/common/xcursor.c +++ b/common/xcursor.c @@ -101,6 +101,7 @@ static char const * const xcursor_font[] = [XC_ul_angle] = "ul_angle", [XC_umbrella] = "umbrella", [XC_ur_angle] = "ur_angle", + [XC_left_ptr] = "left_ptr_watch", [XC_watch] = "watch", [XC_xterm] = "xterm", }; 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 ```

But nothing came out of it, because XC_left_ptr is a duplicate entry, and my knowledge is not enough to go any deeper.

Just for reference, Openbox uses left_ptr_watch as well.

Thanks!

Elv13 commented 2 years ago

Use "left_ptr" without the C patch? XC_left_ptr is already handled by xcursor.c. There is no such thing as a left watch in the X11 enum.

akinokonomi commented 2 years ago

Use "left_ptr" without the C patch?

That's what I did at first, but it does not work. It would still show just the watch cursor (and would never remove it even after app was started).

It actually shows left_ptr_watch after I patch the C file, but messes up everything else.

Elv13 commented 2 years ago

But "left_ptr_watch" in your patch is XC_left_ptr. It has nothing to do with the watch. There is no such thing as a left watch as far as I know.

akinokonomi commented 2 years ago

But "left_ptr_watch" in your patch is XC_left_ptr. It has nothing to do with the watch.

I used Openbox code as a reference, here both of them are XC_left_ptr:

$ rg -p "left_ptr" -uuu *
openbox.c
203:    cursors[OB_CURSOR_POINTER] = load_cursor("left_ptr", XC_left_ptr);
204:    cursors[OB_CURSOR_BUSYPOINTER] = load_cursor("left_ptr_watch",XC_left_ptr);

There is no such thing as a left watch as far as I know.

No, there is not.

Elv13 commented 2 years ago

So "left_ptr" == "left_ptr_watch". You don't need a C patch, just use "left_ptr" and be done with it?

akinokonomi commented 2 years ago

just use "left_ptr" and be done with it?

I'm not sure I understand. If I place left_ptr_watch in startup_notification.lua, it does not wok.

EDIT: Oh, if you mean replace "watch" with just "left_ptr", well, it's not that "watch" cursor annoys me that much, so I rather just leave it alone.

Elv13 commented 2 years ago

If I place left_ptr_watch in startup_notification.lua, it does not wok.

left_ptr_watch doesn't exist, use left_ptr. left_ptr_watch seems like an OpenBox invention and/or a legacy compatibility hack for a typo. We wont add an alias for a cursor name to awesome because OpenBox decided to do so. The cursor is called "left_ptr", nor "left_ptr_watch".

akinokonomi commented 2 years ago

left_ptr_watch seems like an OpenBox invention

We wont add an alias for a cursor name to awesome because OpenBox decided to do so.

Then why does so many (all?) cursor themes have the file named left_ptr_watch?

If it's just an alias, why does Openbox show the cursor on right, not the one on the left, as a startup notification (see screenshot)? EDIT: Neither does it show plain left_ptr, which looks like this:

image

$ pacman -Ql oxygen | grep left_ptr_watch      
oxygen /usr/share/icons/KDE_Classic/cursors/left_ptr_watch
oxygen /usr/share/icons/Oxygen_Black/cursors/left_ptr_watch
oxygen /usr/share/icons/Oxygen_Blue/cursors/left_ptr_watch
oxygen /usr/share/icons/Oxygen_White/cursors/left_ptr_watch
oxygen /usr/share/icons/Oxygen_Yellow/cursors/left_ptr_watch
oxygen /usr/share/icons/Oxygen_Zion/cursors/left_ptr_watch

$ pacman -Ql breeze | grep left_ptr_watch      
breeze /usr/share/icons/Breeze_Snow/cursors/left_ptr_watch
breeze /usr/share/icons/breeze_cursors/cursors/left_ptr_watch

$ pacman -Ql xcursor-themes | grep left_ptr_watch 
xcursor-themes /usr/share/icons/handhelds/cursors/left_ptr_watch
xcursor-themes /usr/share/icons/redglass/cursors/left_ptr_watch
xcursor-themes /usr/share/icons/whiteglass/cursors/left_ptr_watch

$ pacman -Ql capitaine-cursors | grep left_ptr_watch 
capitaine-cursors /usr/share/icons/capitaine-cursors-light/cursors/left_ptr_watch
capitaine-cursors /usr/share/icons/capitaine-cursors/cursors/left_ptr_watch

$ pacman -Ql fuchsia-cursor | grep left_ptr_watch
fuchsia-cursor-theme-bin /usr/share/icons/Fuchsia-Pop/cursors/left_ptr_watch
fuchsia-cursor-theme-bin /usr/share/icons/Fuchsia-Red/cursors/left_ptr_watch
fuchsia-cursor-theme-bin /usr/share/icons/Fuchsia/cursors/left_ptr_watch

left_ptr_watch doesn't exist, use left_ptr.

I'm clearly missing something. How do you suggest to use it?

psychon commented 2 years ago

But nothing came out of it, because XC_left_ptr is a duplicate entry, and my knowledge is not enough to go any deeper.

What exactly do you mean? What does it duplicate? I looked at /usr/include/X11/cursorfont.h and found #define XC_left_ptr 68. Nothing else in there has value 68, so I would actually expect your patch to work just fine...

Looking at the code in AwesomeWM, "everything" basically only ever does xcursor_new(bla, xcursor_font_fromstr(something)). xcursor_font_tostr is completely unused (except one internal call). So... this intermediate layer that assigns numbers should be unnecessary and dropable. I guess this is just a left-over from before the port to xcb-util-cursor.

The only complication would be that some other caching mechanism than the current array would be needed, but that seems doable. Some data structure that maps a string to xcb_cursor_t can surely be implemented with the existing DO_BARRAY mechanism.

akinokonomi commented 2 years ago

What exactly do you mean? What does it duplicate? I looked at /usr/include/X11/cursorfont.h and found #define XC_left_ptr 68.

@psychon The XC_left_ptr in static char const * const xcursor_font[] = {} is a duplicate, so its value gets overwritten.

The

    [XC_left_ptr] = "left_ptr",

already exists, so it left_ptr gets overwritten with left_ptr_watch.

    [XC_left_ptr] = "left_ptr_watch",

So, in theory, instead of plain left_ptr, awesome should show left_ptr_watch, but in practice no, it still shows plain left_ptr sometimes too. And does not hide left_ptr_watch after app window appears. Maybe because I have xsetroot -cursor_name left_ptr & on autostart. No idea.

EDIT: It also breaks some other things, like hovering panel gets you a diagonal cross - [XC_X_cursor] = "X_cursor".

I think to fix this the table should be rewritten (if it's a table?), so that some other custom names would be assigned to both XC_left_ptr + left_ptr_watch and both XC_left_ptr + left_ptr. But how to do that is more than I can say.

You can see that it's exactly what they did in openbox https://github.com/awesomeWM/awesome/issues/3737#issuecomment-1312877596: assigned custom name OB_CURSOR_BUSYPOINTER and OB_CURSOR_POINTER to those values.


Here's the warning at a build time:

...
[ 15%] Building C object CMakeFiles/awesome.dir/common/version.c.o
[ 15%] Generating raw_images/AUTOGEN_wibox_widget_piechart_border_width.svg
[ 15%] Building C object CMakeFiles/awesome.dir/common/xcursor.c.o
/home/user/awesome/src/build/common/xcursor.c:104:21: warning: initialized field overwritten [-Woverride-init]
  104 |     [XC_left_ptr] = "left_ptr_watch",
      |                     ^~~~~~~~~~~~~~~~
/home/user/awesome/src/build/common/xcursor.c:104:21: note: (near initialization for ‘xcursor_font[68]’)
...
psychon commented 2 years ago

Ah, thanks & sorry. Somehow I was thinking in quite a wrong direction here...

akinokonomi commented 2 years ago

Sure, no problem.