cursors: Make build scripts more generic; build more sizes
Needs ReviewPublic

Authored by jamesl on Mar 29 2017, 9:31 PM.

Details

Reviewers
romangg
Group Reviewers
Plasma
Summary

Breeze ships with 24 and 48 px cursors, and recently added 36 px in
D4358. However, with modern screen resolutions coming in all over the
place, there need to be more options. For example, my 14" WQHD display
with a scaling factor of 1.67 looks best with a (24 * 1.67 =) 40 px
cursor. Beyond that, 6k and 8k displays are not far off. KDE already
supports a 3X scaling mode, but doesn't come with a cursor size suitable
for it. GNOME's Adwaita theme already supports 24, 32, 48, 64, and 96
px cursor options. Breeze should have at least the same options.

This change turns the xcursorgen config files into something that can
be processed to generate arbitrary size cursors. The build script has
been modified to iterate over a list of sizes that can be changed as
desired without having to change any hardcoded values. Pointer x-y
hit values are interpolated, and where they fall between pixels, they
are rounded to the nearest pixel. This does not seem to affect
usability, especially on the high-dpi displays for which the cursors
are intended.

Please see Bug #363147 for additional commentary.

https://bugs.kde.org/show_bug.cgi?id=363147

NOTE: This change causes all of the cursor bitmaps and binaries to be regenerated. That was too large for Phabricator, so I have elimitated them from this diff. If accepted, you must run the build.sh script in the Breeze cursor directory to regenerate the binary cursors before committing.

Diff Detail

Repository
R31 Breeze
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
jamesl created this revision.Mar 29 2017, 9:31 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 29 2017, 9:31 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

This is what you should see when you run the build script:

> ./build.sh 
Checking requirements... DONE
Preprocessing cursor configs... DONE

Generating Breeze...
Making folders... DONE
Generating simple cursor pixmaps... DONE
Generating animated cursor pixmaps... DONE
Generating cursor theme... DONE
Generating shortcuts... DONE
Copying theme index... DONE
Generating Breeze... DONE

Generating Breeze_Snow...
Making folders... DONE
Generating simple cursor pixmaps... DONE
Generating animated cursor pixmaps... DONE
Generating cursor theme... DONE
Generating shortcuts... DONE
Copying theme index... DONE
Generating Breeze_Snow... DONE

COMPLETE!

Script looks very nice! I will test run it tomorrow. One thing we have to keep in mind when adding additional cursor sizes is the automatic (resolution dependent) cursor size mode. Adding many sizes with small differences could make the chosen size ambiguous or the algorithm to select the size become inefficient. But I would start from the premise that this is not case. Let's make sure though by looking into it shortly as well.

Regarding your comments in your bug report: After my initial comment I hadn't looked into it again till today when I noticed your Diff here on Phab, so that's why I haven't responded. The Breeze repo is also currently without maintainer, so that's why there were not many other reactions. Thanks for the later responses to Dominik and Andreas therefore!

cursors/Breeze/build.sh
1

Leave this file at mode 644.

14

What about the 36px size? It is scaling factor 1.5, therefore quite common.

jamesl updated this revision to Diff 12997.Mar 29 2017, 11:02 PM

cursor build.sh: Remove execute bit; add 36 px size

romangg added a comment.EditedMar 30 2017, 2:02 PM

I get lots of error messages, which seem to come from failed or missing integer conversion:

roman@workstation ..ce/kde/workspace/breeze/cursors/Breeze (git)-[master] % ./build.sh
Checking requirements... DONE
./build.sh: line 44: printf: 4.00000000000000000000: invalid number
[...]
./build.sh: line 45: printf: 64.00000000000000000000: invalid number
Preprocessing cursor configs... DONE

Generating Breeze...
Making folders... DONE
Generating simple cursor pixmaps... DONE
Generating animated cursor pixmaps... DONE
Generating cursor theme... DONE
Generating shortcuts... DONE
Copying theme index... DONE
Generating Breeze... DONE

Generating Breeze_Snow...
Making folders... DONE
Generating simple cursor pixmaps... DONE
Generating animated cursor pixmaps... DONE
Generating cursor theme... DONE
Generating shortcuts... DONE
Copying theme index... DONE
Generating Breeze_Snow... DONE

COMPLETE!
./build.sh  293,43s user 17,97s system 117% cpu 4:24,57 total

I assume because of that, the generated *.cursor files have their additional values all set to 0. For example alias.cursor:

24 0 0 24/alias.png 
32 0 0 32/alias.png 
36 0 0 36/alias.png 
40 0 0 40/alias.png 
48 0 0 48/alias.png 
64 0 0 64/alias.png 
96 0 0 96/alias.png
jamesl marked 2 inline comments as done.Mar 30 2017, 3:17 PM

@subdiff, can you try running:

LC_ALL=C ./build.sh

If that works I'll update the script. Otherwise, do you have a better suggestion on how to perform the calculations around line 44:

newxhot=$(printf '%.0f' $(echo "$xhot * $i / $size" | bc -l))
newyhot=$(printf '%.0f' $(echo "$yhot * $i / $size" | bc -l))

Of course shell built-ins would be preferred/safer, but I think it's important for this number to be rounded rather than truncated.

Yea, this fixed the problem. Great!

I built it and installed the cursors now. They are also all correctly listed in the cursortheme KCM (kcmshell5 cursortheme) and I was able to select them all individually. I've seen only two problems :

  • On auto resolution it selects size 32px for my 1.5 scaled display, although it should be 36px. But this is probably in KWin, still we shouldn't forget about it (if you are interested in how it works grep "themeSize" in KWin's source and "cursorSize" in Plasma Desktop's source).
  • When using auto resolution the mouse movement seemed to me in fact somewhat more sluggish when before with only 3 sizes, but this might be an illusion. Did you try your patch on a slower machine?
jamesl updated this revision to Diff 13213.Apr 8 2017, 5:13 AM

build.sh: Use C locale when rounding new cursor size xhot and yhot
values with printf.

Can somebody else (with better shell scripting skills than myself) take a look as well?

I came across this from bugzilla. Apparently unfortunately there has not been anybody reviewing this until now. Is this still relevant? At least I hope it can get sorted out, soon!

Ping @jamesl @ngraham @broulik @romangg