Breeze currently ships with 24 and 48 px cursors. My 14" WQHD screen, which I understand to be a fairly common size and resolution, needs something in between to look right. For this screen, 40 px is ideal. GNOME's theme Adwaita ships with 24, 32, 48, 64, and 96 px options. It seems reasonable for Breeze to have at least the same options. I have patched the Breeze cursor build script to support building many sizes without hard-coding the xcursorgen configs. You can find the changes on my 'cursor-build-multiple-sizes' branch at: https://github.com/iamjamestl/breeze/tree/cursor-build-multiple-sizes https://github.com/KDE/breeze/compare/master...iamjamestl:cursor-build-multiple-sizes See the commit message for implementation comments. Reproducible: Always
I've rebased my branch at https://github.com/iamjamestl/breeze/tree/cursor-build-multiple-sizes so this is cleanly mergeable into current HEAD.
Hi James, thanks for your work. I did something similar yesterday and didn't see what you did before that. My Diff is here: https://phabricator.kde.org/D4358 It adds a 36px cursor only, which is sufficient in my opinion. But please take a look at the Diff and tell me if I should change something else. For your next patch, try to use our Phabricator directly instead of Github, which we don't use for dev work. Then some of the devs will notice it earlier. Thanks!
36 px isn't good enough and here's why: You have a whole class of laptops with 14" 2560x1440 (WQHD) displays, such as the Lenovo ThinkPad T4*0/X* line of laptops that are very popular in the Linux community. These screens require a scaling factor of 1.67 (that's one and two thirds), which would make a (24 * 1.67 =) 40 px cursor look right. You may ask, "what difference does 4 pixels make?" Well that's 10%, and it's instantly noticeable, especially when highlighting lines relative to normal text height. See the attached screenshots. You might say, "well having 24, 32, 36, 40, and 48 is too many confusing choices." But users are used to making choices like that for fonts, where sizes like 24, 32, 36, 40, and 48 are common. You might say, "well having that many sizes increases the size of the resulting binaries installed on the system." To that I say adding sizes 32 and 40 in addition to 36 increases the total size of both Breeze and Breeze_Snow combined by just 625 KB. You might say, "well, managing all of those sizes is kind of ugly with our build scripts." But if you had actually looked at my patch, you would have seen that I completely refactored the build script to support arbitrarily many sizes, and also support building arbitrarily many cursors (so that you don't have to duplicate the build script across both the Breeze and Breeze_Snow cursors). Please reconsider my patch. Also, I take sincere objection to your suggestion that I didn't submit my contribution properly. I have contributed to a lot of open-source projects before. This is a bug so I submitted a bug report and made it perfectly clear in the bug report title that I included a patch. The patch had a lot of binary data, so rather than upload it to Bugzilla, I chose to link to it on GitHub, where there is an official mirror of the Breeze repository. If you don't want people using GitHub, either don't mirror the repo on GitHub, or include a CONTRIBUTING file in the repository so community members know how to submit contributions. Furthermore, you are the KDE dev, not me. You should have seen this; I mean, it's been eight months! It is your job to look at and respond to Bugzilla bugs, and shepherd community contributions through your SDLC, of which I am unfamiliar, especially because there is no CONTRIBUTING file in the repository! Your apparent dismissal of my well-crafted (and personally time-consuming) contribution is offensive and makes me less likely to contribute to KDE projects. Please reconsider my patch. Again, it's at https://github.com/iamjamestl/breeze/compare/master...iamjamestl:cursor-build-multiple-sizes.patch in standard Git unified diff format, regardless of where it's hosted. If you need me to rebase it again, I can. Thanks, James
Created attachment 103738 [details] Breeze @ 36 px on properly-scaled WQHD display
Created attachment 103739 [details] Breeze @ 40 px on properly-scaled WQHD display
I have rebased the patch on the current master HEAD and added your 36 px size. Again, it's at: https://github.com/iamjamestl/breeze/compare/master...iamjamestl:cursor-build-multiple-sizes.patch You have no excuse not to consider it. Please do the honorable thing. This patch was first, and it's better. Thanks, James
@James: You have to create a patch at phabricator.kde.org (make an account at identity.kde.org first). Then, you patch is much more visible to developers.
Dominik, I tried, but due to the binary data in this patch, it was too large for Phabricator, by an order of magnitude.
*** This bug has been marked as a duplicate of bug 348603 ***
So that's how it's going to be. Guess I won't be contributing to KDE anymore.
(In reply to James Lee from comment #10) > So that's how it's going to be. Guess I won't be contributing to KDE > anymore. That would be sad. FYI: https://github.com/KDE "Official mirror of the KDE project. To contribute code to these repositories, please see https://community.kde.org/Infrastructure/Github_Mirror" Bugzilla is good for bug reports, but that's really it; where was the problem, did you use arcanist for trying to upload your changes?
Created attachment 104804 [details] Phabricator Error on Patch Upload I literally can't upload this patch to Phabricator. Because it contains new sizes of the cursors, it exceeds the 2 MB limit. The patch is 13 MB.
I can see where the web interface would limit you like that, but that shouldn't happen if you had a clone of the repository, worked in a branch and submitted your diff using arcanist. Seems like https://community.kde.org/Infrastructure/Github_Mirror needs an update as well though, as it still mentions reviewboard and rbtools instead of phabricator...
For what it's worth, this page https://community.kde.org/Infrastructure/Github_Mirror, and probably Phabricator, didn't exist when I submitted this bug report and patch. I tried uploading the patch with Arcanist and it was still too large. I took out the build data as suggested at https://secure.phabricator.com/book/phabricator/article/differential_large_changes/ and then it went through. The diff is open at https://phabricator.kde.org/D5250; and I think it is still worth a review since it improves the cursor build script and supports building arbitrarily many cursor sizes.
Oh, I did not notice that the new patch contains more changes. Roman, can you have a look?
*** Bug 383146 has been marked as a duplicate of this bug. ***
Roman? Do you have time to review James's contribution? Especially if the custom cursor size could be integrated in future to System Settings with a slider instead of dropdown selection. I recently setup a user with 1080p display for vision impairment accessibility, similar changes that HiDPI users do to scale up elements. 48px is too small, they still have trouble spotting the cursor, I'm not sure what size it is on macOS that they also use but it was much larger. Bug report(marked as duplicate of this one): https://bugs.kde.org/show_bug.cgi?id=383146
Hi Brennan, from my side the patch worked. It felt though like there were some performance issues when using the many sizes (since it loops through them), but that might be also in my imagination. The thing is Ken had written the original script and I pinged him in the review on Phabricator for his opinion, but he didn't yet respond to it. If you can draw his attention to the review it has a chance to be revived and become accepted. I'm currently without enough time on my hand to organize it (writing on my thesis).
Just adding a comment here as well to draw attention that the merge request on Phabricator is still not reviewed, yet. Also setting to "confirmed" since that seems to be common agreement from what I read here.
*** Bug 477489 has been marked as a duplicate of this bug. ***
477489 asks for arbitrary cursor sizes.
I'd like to add that some non-Qt, non-GTK apps (e.g. kitty, which uses GLFW) don't scale cursor images themselves, so on 300% zoom the largest Breeze cursor they use would be 48px, which is much smaller than 72px in KDE apps. So in addition to the current 24/36/48px, at least 60px (for 250% zoom) and 72px (for 300% zoom) should be added. And if KDE is to allow arbitrary cursor sizes, physical cursor image files of `{cursor size}x{global zoom}` must be generated, too.