Skip to content

libs-gui: only create icon/miniwindow when GSSuppressAppIcon is explicitly NO - #930

Open
probonopd wants to merge 1 commit into
gnustep:masterfrom
probonopd:gssuppressappicon-only-no
Open

libs-gui: only create icon/miniwindow when GSSuppressAppIcon is explicitly NO#930
probonopd wants to merge 1 commit into
gnustep:masterfrom
probonopd:gssuppressappicon-only-no

Conversation

@probonopd

@probonopd probonopd commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

As discussed in the last monthly meeting, creating miniwindows should only happen when requested (opt-in).

Previously, the app icon and miniwindow were created unconditionally and only the display was suppressed. This led to unnecessary allocations and potential issues when GSSuppressAppIcon was not set.

With this change, _appIconInit returns early if GSSuppressAppIcon is not set to NO, and miniwindow creation is skipped unless GSSuppressAppIcon is NO. Also adds a nil check before GSRemoveIcon to prevent potential crashes.

This change is based on gershwin-desktop/gershwin-developer@e8f2110

cc @gcasa @pkgdemon

@probonopd
probonopd requested a review from fredkiefer as a code owner August 31, 2026 14:16
@probonopd
probonopd force-pushed the gssuppressappicon-only-no branch from 3d3d4cb to 94a16b9 Compare August 31, 2026 14:23
@probonopd

probonopd commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

I suspect the test failures are not caused by this PR (which doesn't touch fonts), what do you think?

gui/NSFont/named.m:
Failed file:  named.m aborted without running all tests!
Failed test:     (2026-08-31 14:26:57.787 UTC)   named.m:37 ... fontWithName:size: returns a font
Failed test:     (2026-08-31 14:26:57.787 UTC)   named.m:39 ... the font keeps the requested name
Failed test:     (2026-08-31 14:26:57.787 UTC)   named.m:40 ... the font has the requested point size

@gcasa

gcasa commented Aug 31, 2026

Copy link
Copy Markdown
Member

I suspect the test failures are not caused by this PR (which doesn't touch fonts), what do you think?

gui/NSFont/named.m:
Failed file:  named.m aborted without running all tests!
Failed test:     (2026-08-31 14:26:57.787 UTC)   named.m:37 ... fontWithName:size: returns a font
Failed test:     (2026-08-31 14:26:57.787 UTC)   named.m:39 ... the font keeps the requested name
Failed test:     (2026-08-31 14:26:57.787 UTC)   named.m:40 ... the font has the requested point size

I just re-ran this. It fails in the same way. I am not sure what could have changed to cause this issue.

@gcasa

gcasa commented Aug 31, 2026

Copy link
Copy Markdown
Member

The failure in named.m is because there doesn't always have to be a "Helvetica" font. In the art backend, this was a safe assumption since it copied in a "Helvetica.nfont" (nfonts are something created by Deek / Jeff Teuneissen) to fulfill this. I am not sure why we didn't see this failure until now, but it's my guess that that is the cause here.

@gcasa

gcasa commented Aug 31, 2026

Copy link
Copy Markdown
Member

Please try this version of named.m... I have zipped it here. It retrieves the names of all fonts and then grabs one by name. This ensures were are using a font that is actually installed.

named.m.zip

@probonopd

Copy link
Copy Markdown
Contributor Author

Thanks @gcasa.
Wouldn't you want to get that applied independently from my PR, and then rebase my PR on top of it?

@gcasa

gcasa commented Aug 31, 2026

Copy link
Copy Markdown
Member

Thanks @gcasa. Wouldn't you want to get that applied independently from my PR, and then rebase my PR on top of it?

I had thought about that. I will try it.

@fredkiefer

Copy link
Copy Markdown
Member

Not sure whether that change will help. What should happen with the name "Helvetica" is that it is hard coded into NSFont and if no font with that name can be found, we try the default font, whatever the backend uses for that. What could be happening there is that none of the well known fonts is present.
And as you correctly asked, why is this happening now and didn't happen for the last merged PR? One possible option is that a new base image is being used.

I'll have a look at what the frame buffer version of cairo is using here, but I would expect it to have all the normal fontconfig fonts.

@gcasa

gcasa commented Aug 31, 2026

Copy link
Copy Markdown
Member

I have pushed this change on master. Please pull that change.

@gcasa

gcasa commented Aug 31, 2026

Copy link
Copy Markdown
Member

FYI, I have been frustrated with the current behavior. In my view, GSSuppressAppIcon should act as you suggest.

@gcasa

gcasa commented Sep 1, 2026

Copy link
Copy Markdown
Member

Not sure whether that change will help. What should happen with the name "Helvetica" is that it is hard coded into NSFont and if no font with that name can be found, we try the default font, whatever the backend uses for that. What could be happening there is that none of the well known fonts is present. And as you correctly asked, why is this happening now and didn't happen for the last merged PR? One possible option is that a new base image is being used.

I'll have a look at what the frame buffer version of cairo is using here, but I would expect it to have all the normal fontconfig fonts.

My approach here was to avoid the hardcoding and find all fonts and get the name of the first one. This way it preserves the intention of the test. I think you're saying we should hardcode "Helvetica" so that referencing that (if Helvetica itself isn't available) it will get the default font. Is that correct?

@probonopd
probonopd force-pushed the gssuppressappicon-only-no branch from 94a16b9 to e8d53c8 Compare September 2, 2026 17:37
@gcasa

gcasa commented Sep 4, 2026

Copy link
Copy Markdown
Member

So now the font tests are failing in a different place?

@pkgdemon

pkgdemon commented Sep 4, 2026

Copy link
Copy Markdown

So now the font tests are failing in a different place?

From the test log:

Testing convert.m...
Running gui/NSFontManager/convert.m...
Start set:       convert.m:22 ... convertFont:toSize:
2026-09-02 17:42:19.885 convert[28134:28134] Failed to get _NET_WORKAREA
2026-09-02 17:42:19.894 convert[28134:28134] Failed to get _NET_WORKAREA
Failed test:     (2026-09-02 17:42:19.904 UTC)   convert.m:40 ... the converted font has the requested size
Failed test:     (2026-09-02 17:42:19.904 UTC)   convert.m:42 ... the converted font keeps the original name
Passed test:     (2026-09-02 17:42:19.904 UTC)   convert.m:44 ... converting to the current size returns the original font
End set:         convert.m:50 ... convertFont:toSize:

@probonopd
probonopd force-pushed the gssuppressappicon-only-no branch from e8d53c8 to 4d3e06e Compare September 5, 2026 09:40
@probonopd

probonopd commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Looks like the Cairo backend needs a window for the screen to be initialized, which is used for font size conversions. Since we no longer have the miniicon windows, we probably need to initialize the display connection manually in the test.

Does this sound correct @gcasa?

…citly NO

Previously, the app icon and miniwindow were created unconditionally
and only the display was suppressed. This led to unnecessary allocations
and potential issues when GSSuppressAppIcon was not set.

Now _appIconInit returns early if GSSuppressAppIcon is not set to NO,
and miniwindow creation is skipped unless GSSuppressAppIcon is NO.
Also adds a nil check before GSRemoveIcon to prevent potential crashes.
@probonopd
probonopd force-pushed the gssuppressappicon-only-no branch from 4d3e06e to 4b8a923 Compare September 5, 2026 09:58
@fredkiefer
fredkiefer removed their request for review September 6, 2026 15:36
@gcasa

gcasa commented Sep 7, 2026

Copy link
Copy Markdown
Member

Looks like the Cairo backend needs a window for the screen to be initialized, which is used for font size conversions. Since we no longer have the miniicon windows, we probably need to initialize the display connection manually in the test.

Does this sound correct @gcasa?

I don't think so. From my experience... (granted I have limited experience with the backend, thankfully it is very stable)... initializeContext for CairoContext currently only registers the default graphics context and font classes. It doesn't create or inspect a window. It just creates the display server. What leads you to the conclusion that a window is needed?

@probonopd

probonopd commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

the test failures were caused by the default graphics context lacking proper screen/drawable initialization when the app icon window is no longer created.

Two options are proposed to fix this.

In NSApplication._init, the sequence is:

  1. initialize_gnustep_backend() - registers context classes
  2. [GSDisplayServer serverWithAttributes:nil] - connects to display
  3. [[NSScreen mainScreen] deviceDescription] at line 899 - retrieves screen info
  4. [NSGraphicsContext graphicsContextWithAttributes:attributes] - creates default context
  5. [self _appIconInit] at line 934 - previously created the icon window (which initialized screen-related state)

The problem: the default context created in step 4 is not associated with any window. In the xlib backend's GSContext.m, the association between context and window device only happens when a window is passed as the destination attribute (lines 225-226):

[server setWindowdevice: [(NSWindow*)dest windowNumber] 
                forContext: self];

With the old code, _appIconInit created an icon window, which triggered this setWindowdevice:forContext: call and initialized the gstate's context and drawMechanism fields. With the new code, this initialization no longer occurs.

Note: gcasa mentioned initializeContext for CairoContext, but this method doesn't exist in the codebase.

Two options to fix this:

Option A: Fix in the test's main() function in Tests/gui/NSFontManager/convert.m, before font operations:

// Force screen context initialization before font operations
(void)[GSCurrentServer() screenRContext];

// Now safe to do font operations
NSFontManager *fm = [NSFontManager sharedFontManager];

Option B: Fix in NSApplication._init in Source/NSApplication.m, after creating the default context:

_default_context = [NSGraphicsContext graphicsContextWithAttributes: attributes];
RETAIN(_default_context);
[NSGraphicsContext setCurrentContext: _default_context];

// Ensure screen context is initialized before any font operations
(void)[GSCurrentServer() screenRContext];

Option A is less risky because it only affects this specific test and doesn't change behavior for other apps. Option B is a more comprehensive fix but changes core initialization behavior that could have wider unintended consequences.

Which option do you prefer, or would you like to pursue both?

@fredkiefer

Copy link
Copy Markdown
Member

Sorry both options are wrong. Did you come up with this yourself or did you use AI for that? If the later is the case, please switch to a better version.
If this was your own idea, please have a look at where screenRContext is defined and used. It only exists in back and there in XGServer, which means, we can only use it in the x11 based backends and no others. So if you really want to use this method, do so in the backend.Perhaps call it at the end of _initXContext? But I am really not sure.

As this PR already caused some unnecessary changes in gui, I am very reluctant here. Could we please postpone all this until after the release?

@probonopd

Copy link
Copy Markdown
Contributor Author

Agree that if this is seen as a risky change it should be done after the release.

Please note that already without further changes, the tests pass now, by the test creating a window. My theory is that the issue did not exist before this PR because a window was always created (the miniwindow) that also initialized a context, and now that the miniwindow is not always created anymore, we need a window (or at least a context) to be created by some other means (e.g., the test). While I did use AI to help me research (after all, this codebase is still unfamiliar to me), i came to the conclusions myself, so I am to blame if the logic is wrong. Thanks for the hints, will investigate more.

@pkgdemon

pkgdemon commented Sep 9, 2026

Copy link
Copy Markdown

If I am following correctly. Does the test itself just need another app to open to create a window? It sounds like just a broken test that could be fixed by simply opening an app as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants