Hi!

Thanks for the report.
I am reading this code and see this comment about this macro:

/* The maximum dimension of Xcursor which is fully supported in any
 * environments. This is defined on line 59 of xcursorint.h in
 * libXcursor source code. Make sure this is about dimensions(width
 * and height) not about nominal size despite of it's name.
 */

And indeed finding libxcursor code, 64 still seems to be the value of this macro. Now I have not really tried to understand really the code. I just noted that this macro value was based on some other source and I wonder if that won't break something.
If you have read and understood the code, could you confirm there won't be a consistency bug or something on the way?

Also please, would you mind submit a patch with `git format-patch origin/master` so that we get proper authorship and comment for the commit. While you do this, please also update the comment to explain why this value was originally based on libXcursor external source and why it doesn't matter anymore. Otherwise this is all just contradictory and will byte us later on, when we will try to understand this code.
Thanks.

I was trying to edit the Adwaita cursor theme which is the default cursor theme for GNOME 3.  Simply opening and re-exporting the cursor file I get this warning:

      g_message (_("Your cursor was successfully exported but it contains one or "
                   "more frames whose width or height is more than %ipx.\n"
                   "It will clutter the screen in some environments."),
                   MAX_BITMAP_CURSOR_SIZE);

This is because GNOME is already using icons that are 96px while the limit here is 64px.  I'm guessing this feature was added in case someone used a large logo or something and didn't realize it needs to be smaller.  What results is two icons that are 64px rather than one that is 96px and another that is 64px.  I think the image actually stored in the file was 96px but the header shows 64px.  So you can import the cursor file but not export it correctly.  It might take some extra free time of mine, but I'll try and create a patch for this change in format-patch format and confirm that I can import and export the cursor without issues given the larger size.

Based on my understanding of the xcursor format I don't think there is a fundamental issue creating a cursor of almost any size as long as it's less than 32-bit in dimension or the total size is less than a 32-bit offset.  By using an export layer plugin for GIMP to get each layer as a PNG I can then use xcursorgen to recreate the cursor with the correct sizes.  This change in GIMP would just make that editing process much easier.

ftp://www.x.org/pub/X11R7.7/doc/man/man3/Xcursor.3.xhtml#heading4

Ok. I will play a bit with these cursors from adwaita-icon-theme to understand better this format and get back to you.

So I had a closer look. First, it looks to me like the cursor loaded on my screen is 32px and I have a crappy medium resolution, medium density display. With the avent of QHD and 4K screens (I even see 5K screens are sold nowadays) with very dense pixels, I can easily imagine not only a 96px but easily much bigger cursor size to work well on the very dense machines.

And not even counting accessibility issues (especially visual deficiencies) where one might want bigger cursors.

So yeah, I totally agree on the base idea of raising this value.
I'm only worried on the part where it says that it is based on a max value of libXCursor.

So first libXCursor is apparently the X11 library to load cursor, as one would expect from the name (ftp://www.x.org/pub/X11R7.7/doc/man/man3/Xcursor.3.xhtml).

So I checked out the code from the source repository.
I can confirm that MAX_BITMAP_CURSOR_SIZE still exists and is still 64 even on master. It is used only for XcursorNoticeCreateBitmap() and XcursorNoticePutBitmap() forbiding indeed bitmap with a dimension over 64px as cursor. So at first, it felt bad. Now it seems as if these 2 functions are not used internally, only available in header. But that doesn't mean they are not used somewhere else in X11.

I'm not going to go much deeper in X11 innerworks so I'll just do it empirically:

(1) can you just confirm me that X11 loads properly your 96px cursor? Does it show on screen as expected?
(2) Is this X11 Cursor format only used within X11, as the name implies? Does Wayland for instance use this as well (where I would hope such a low limitation is gone)? Or used by other software?

I think I'll just make a decision depending on your answers to the questions on these 2 points.

Even GIMP ditched bitmap cursors a while ago, and we usually keep
compat stuff for looooong :)

(Unrelated: You make a good point about 4K and larger screens, we should
probably also offer our own cursors in twice a large or larger at some
point).

I have a 28" 4k monitor which is HiDPI (150% scaling) so I have the cursor set to large under the accessibility settings in GNOME, which is a 48px cursor.  I just tested the largest setting which should be 96px and it does work fine.  I have an nvidia card so wayland is not an option with the nvidia driver loaded, but I suppose I can try on my laptop when I get a chance and switch to wayland just to do this same test.

Just to provide a bit of back story I recently installed a Titan V in my desktop so I can do some AI work from home and it causes a bug in X11 with the latest nvidia driver.  When a cursor animates like the waiting cursor it freezes up the xserver when it finishes one complete animation cycle.  You can attach and detach with gdb over SSH to resume surprisingly.  I was using GIMP as one attempt to remove all but one frame from each resolution making these cursors no longer animated.  As I mentioned before I was able to accomplish this by using an export layers plugin to get the PNGs and then using xcursorgen with the hot spots extracted from the original cursor file using a hex editor.  When I loaded it back into GIMP it has the right size again, 96px.  I just couldn't do the mod directly in GIMP due to this export restriction.  The cursor works and no longer animated and the freeze went away.  In the end it looked kind of funny stuck on one frame so I "disabled" animated cursors by just replacing them with the plain arrow.  The lack of busy notification really hasn't been a problem so far.

(In reply to Michael Natterer from comment #5)
> Even GIMP ditched bitmap cursors a while ago, and we usually keep
> compat stuff for looooong :)

Hmmm… So I am not familiar with cursors at all so maybe they use unexpected definitions. Otherwise what are all the png files under cursors/? That looks like bitmap cursors to me!
And I don't see vector cursors (which format would that be?). Or isn't it what you meant when you said we ditched bitmap cursors?
 
> (Unrelated: You make a good point about 4K and larger screens, we should
> probably also offer our own cursors in twice a large or larger at some
> point).

Yep.

(In reply to Eric Work from comment #6)
> I have a 28" 4k monitor which is HiDPI (150% scaling) so I have the cursor
> set to large under the accessibility settings in GNOME, which is a 48px
> cursor.  I just tested the largest setting which should be 96px and it does
> work fine.

Just a bit of a warning. I read that when doing HiDPI scaling, GNOME would double pixels of bitmaps. So when displaying a 96px cursor, I am wondering if it actually uses an available 96px version of a 48px one where it duplicates every pixel (this could be tested by making a completely different 96px, just for the sake of differentiating them during the test).

For info, I also managed to get a 96px cursor in GNOME by playing with /org/gnome/desktop/interface/cursor-size in dconf Editor. This is apparently even the max size I got (any bigger size would just give me a 96px cursor). I have not tested if it actually used a 48px cursor and doubled it or whatnot.

> When a cursor animates like the waiting cursor

I have a question about cursor animation and this X11 cursor format. Are all layers in a X11 cursor the frames of an animated cursor or are they various sized versions of a same cursor?
I made all my tests on a random Adwaita cursor (the first: bd_double_arrow): https://git.gnome.org/browse/adwaita-icon-theme/tree/Adwaita/cursors
And I see each layer has a different size (24 to 96px). So it looks more like a set of various sizes rather than an animation to me, even though each layer shows a duration and our messages also tell about frames.

Anyway here is my attempt to a resolution. I made a few changes, but basically in your case, we won't clamp anymore to this value so your export will be successful, yet will still warn when using higher values (changing the macro value while saying it is synced to libXCursor makes no sense; so now we just use it as a warning test).

Note that I am considering just dropping this annoying warning. But let's see how this goes first.

Is this commit good for you?
I will close this report as fixed, but please reopen if you disagree or think of a better solution.

commit 0484ce83afbc6fca1554e4d133469737e27e47bf (HEAD -> master, origin/master, origin/HEAD)
Author: Jehan <jehan@girinstud.io>
Date:   Sun Jan 7 14:41:44 2018 +0100

    Bug 792266 - Increase maximum size of x11 cursor during export.
    
    We were basing our max export size on a macro value defined in
    libXCursor code: MAX_BITMAP_CURSOR_SIZE. This macro is still defined in
    libXCursor and still has the same value (64), yet it is unsure how far
    or even where this is enforced since it seems we can get at least 96px
    cursors in GNOME/X11.
    
    As a consequence, this commit:
    - still warns when cursor size is over this value, with more explicit
      text, yet does not change the cursor size anymore! So it is now
      possible to export bigger cursors, but you still get a warning.
    - only changes the cursor size for the existing more-than-8-digits test
      and I add a warning when it does so (we should never modify an image
      silently!).
    - adds the size 96 as not triggering the warning about GNOME Settings
      since it definitely looks like this size is valid there (according to
      my own empirical tests). Also since 96 is higher than the libXCursor
      current MAX macro value, this really raises the question to where this
      max is enforced and whether we should not just drop the first warning.
    
    Note that it breaks a bit the string freeze since I modify one string
    and adds one. Sorry for this!

 plug-ins/common/file-xmc.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

I'll apply your patch so I can then modify one of the cursors from the Adwaita theme.  I'll change the 96px image only to some other color and then test that when changing between 48px and 96px cursors in GNOME that it does indeed use the larger one.  I'll repeat the same experiment on Wayland as well.  It may take me a few days to finish this task.  This will confirm that GIMP is now working for the larger size and that GNOME indeed supports 96px cursors.

I performed all the experiments that were suggested.

1. Patch gimp with 0484ce83, rebuild, replace file-xmc.
2. Copy "arrow" Adwaita cursor icon to a tmp location.  Open the file in gimp and color the 96px cursor red instead of black.
3. Overwrite "arrow" with the modified cursor file.
4. Change the cursor from 48px to 96px.  Observe that it changes to red.
5. Change the cursor from 96px back to 48px.  Observe that it changes back to black.
6. Repeat step 2-4 when using Wayland.  Same result.

So in summary the patch allows gimp to save the 96px cursor back as 96px and not 64px. GNOME 3 does use a 96px cursor, not 48px doubled, which can be set from the accessibility settings panel or using "gsettings set org.gnome.desktop.interface cursor-size 96".  I know this bug is already closed, but I wanted to confirm our assumptions.

There is one remaining question which does not necessarily pertain to this size issue.  Using xcursorgen you can set a hotspot for each icon size since in could in theory be different for each image.  In gimp during export it asks for a single hotpot x & y.  I didn't check in a hex editor if this is used for all images or not but it should ideally have an option for each image, maybe encoded in the layer name like the size and timing.  That would be an enhancement of course and a much larger scale change.  Still this change is a nice step forward.

> I know this bug is already closed, but I wanted to confirm our assumptions.

Yes, thanks you for all the tests. This is indeed informative.

I also got some news in between, that "bitmap" pertains to bicolor icons (i.e black & white), hence Mitch's comment 5. I was kind of working on the assumptions that bitmap and pixmap were basically the same.

So this warning is only for a very specific type of X cursors, which are bitmap X cursors, which are indeed probably not much used anymore. So I reworded a bit the warning in commit 22a6e2bb19da4fd2fe6fd347a6ea185abb9b220b.
I would have just removed it (since it doesn't look like our plug-in is even able to export bitmap cursors) but I am told that GDK can turn pixmap cursors into bitmap cursors if a system doesn't support them. So well… I leave it for now until someone who knows better does something about it (old compatibility shit is always a bit scary!).

And this probably explains why the pixmap cursors don't seem to have this limitation even though the macro is still in libXCursor code.

> Using xcursorgen you can set a hotspot for each icon size since in could in theory be different for each image.  In gimp during export it asks for a single hotpot x & y.

Do not hesitate to write a new feature request for this. Otherwise this idea will likely be lost for the eons.
Also if you can provide a patch, that would be even better. ;-)

(In reply to Eric Work from comment #9)
> There is one remaining question which does not necessarily pertain to this
> size issue.  Using xcursorgen you can set a hotspot for each icon size since
> in could in theory be different for each image.  In gimp during export it
> asks for a single hotpot x & y.  I didn't check in a hex editor if this is
> used for all images or not but it should ideally have an option for each
> image, maybe encoded in the layer name like the size and timing.  That would
> be an enhancement of course and a much larger scale change.  Still this
> change is a nice step forward.

For the record, I opened bug 792445 about this.
Not going to work on this personally, I think, but I would review any patches contributed to us.
At least now the feature request is in the system. :-)