Skip to content

Conversation

@colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Oct 13, 2025

What it does

Fixes #16418 by checking whether writing the preference would be necessary.

How to test

  1. Follow steps from Title bar style preference unnecessarily written to settings.json #16418
  2. Observe that the preference is not written to the file on MacOS.

Warning

Arguably, this 'fix' actually reveals a bug. Since the preference should not be included on MacOS, the inspect call should return undefined. At the moment, it doesn't: the preference system in fact has added the preference to its schema, with the appropriate default value of native. We should likely fix that, and then ensure that the preference isn't active at all in that case, and that all consumers of the value correctly handle its absence.

Follow-ups

Breaking changes

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Review checklist

Reminder for reviewers

@github-project-automation github-project-automation bot moved this to Waiting on reviewers in PR Backlog Oct 13, 2025
@colin-grant-work colin-grant-work force-pushed the bugfix/unnecessary-title-bar-setting branch from 083b1d5 to e6a9597 Compare October 13, 2025 13:49
@colin-grant-work colin-grant-work changed the title Only set window.titleBarStyle preference if active value differs Only set window.titleBarStyle preference if different from active value Oct 13, 2025
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like my changes in #11588 broke with the latest preferences refactoring? I agree that the frontend should definitely filter those preferences out.

…tion.ts


Extra space snuck in

Co-authored-by: Mark Sujew <[email protected]>
@colin-grant-work
Copy link
Contributor Author

It seems like my changes in #11588 broke with the latest preferences refactoring?

Certainly seems that way. At a casual glance, it looks like there is some checking of the included property, but I'm not exactly sure where in the schema building lifecycle it's taking place - apparently not everywhere it should.

@msujew
Copy link
Member

msujew commented Oct 14, 2025

FYI my comment was just to confirm that something is broken - I can't actually test the changes, since I don't have a MacOS system available to me right now.

@colin-grant-work
Copy link
Contributor Author

FYI my comment was just to confirm that something is broken - I can't actually test the changes, since I don't have a MacOS system available to me right now.

@msujew The preference is written regardless of platform, so it is possible to test that part on any OS. Just delete it from your settings JSON, and then check that it doesn't reappear. The only difference on MacOS is that if it does get written, then your settings JSON complains that it shouldn't be there.

@ndoschek ndoschek requested a review from msujew November 25, 2025 10:47
@ndoschek
Copy link
Contributor

@msujew would you have time to take another look? TIA!

Copy link
Contributor

@ndoschek ndoschek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this on Ubuntu, and unfortunately it doesn't work for me: removing the line from the user settings file causes it to reappear after a reload.

I assume this is related to the fact that resetting this preference also doesn't work, I think we have here the same problem aswe encountered in #16646.
As noted, this is being tracked further in #16645, which aims to fix other occurrences that rely solely on newValue.

Remove line and reset preference
Screencast.2025-11-28.10.18.33.mp4

@colin-grant-work
Copy link
Contributor Author

I tested this on Ubuntu, and unfortunately it doesn't work for me: removing the line from the user settings file causes it to reappear after a reload.

@ndoschek The behavior you're seeing is incorrect, but not because the setting is being written to your settings.json. On Linux, the default value for the preference is native, so that if custom is the active value, it needs to be written to the settings.json. This PR only aims to avoid writing the preference if it would be redundant. What is strange about what you're seeing is that you're changing the preference value from custom -> undefined, which should cause the default value native to take effect, but it doesn't appear that it is taking effect. Or is it? It's a bit hard to tell, since the menu items aren't visible in the recording.

@ndoschek
Copy link
Contributor

ndoschek commented Dec 2, 2025

Ah, I see, thanks for the explanation! You're absolutely right: testing with native and removing the line, it no longer reappears. 🎉

Yes, it is a bit strange, but I think in the case where the value is custom and I remove the line, it gets restored via window.electronTheiaCore.getTitleBarStyleAtStartup(). We already receive a style there, and I assume it's the last one that was set?

Regarding the reset case: I'll be working on ticket #16645 this month, and I'll make sure to address this case then!

But I noticed one more thing:
With this change, when there is no entry for the title bar in the settings.json and I switch back to custom in the settings UI, the reload dialog is no longer shown immediately. The titleBarStyleChangeFlag remains false in this first case, and only if I switch back to native and then to custom again does the reload prompt appear.

Remove line and switch to custom
Screencast.2025-12-02.17.35.47.mp4

And if I remove the flag entirely, I end up in an infinite reload loop 😅
I can take another look tomorrow and see if I can spot the problem, since it's not really testable on your end.

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

Labels

None yet

Projects

Status: Waiting on reviewers

Development

Successfully merging this pull request may close these issues.

Title bar style preference unnecessarily written to settings.json

4 participants