-
Notifications
You must be signed in to change notification settings - Fork 8.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Terminal shouldn't treat read-only settings as an error reading settings #7727
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@amazingant if I'm reading between the lines, what you'd rather have is "stop putting in profiles I don't care about". That we consider a read-only file an error is almost entirely accidental, and we definitely shouldn't hork it when that happens. I'd rather solve your actual issue than the one you've made for yourself, however. 😄 |
@DHowett I actually am after the blowing up when my settings file is read-only - I prefer to hand-edit the settings file and keep it updated/committed in my dotfiles repo 😆 Since there's a setting to specifically stop adding other profiles (yay!), I'll definitely make use of it going forward. 😄 I subscribed myself to this repo's releases so I know when there's new settings, and usually undo the read-only and fiddle with the new settings after I get updated; it's fun seeing the progress y'all are making and getting to try the new features. |
Thanks & understood. @DHowett would you like a new bug report or are we happy the change to reject past working settings.json is intentional? |
@steveroot This one's.. complicated. We chose to move to uniform JSON parsing to make our lives easier in preparation for #1564... but that came at the cost of a couple incidental compatibility tricks we were playing. Having been the person to make that decision, I chose to allow for some small amount of scream testing. If enough people reported trouble with a particular setting, we'll go backsies on it. . . but until there's a big enough mess of complaints the strictness is "by design". Make sense? 😄 |
Good choice, it makes sense and I agree. The error I saw (+the useful error explanation with how to fix it) is small price to pay for progress. |
How would you expect this to work?
|
As an end user, I'd prefer that Terminal not complain at all, and have it still honor my settings as are were on disk. As a developer, if Terminal successfully reads the settings file and deserializes/validates them, but can't write an updated version of them, I'd probably want it to still honor the settings as they are on disk, but to show the user (still me) a warning. While I've done this on purpose on my system, if someone else had done this by accident, or there were other permissions/disk issues they had run into, it would be more useful to say there was an issue writing the settings file rather than saying they made syntax errors. |
We wrap the call to `_WriteSettings` in `CascadiaSettingsSerialization.cpp` in a try/catch block, and if we catch an error we append a warning telling the user to check the permissions on their settings file. Closes #7727
🎉This issue was addressed in #7950, which has now been successfully released as Handy links: |
🎉This issue was addressed in #7950, which has now been successfully released as Handy links: |
Environment
Steps to reproduce
settings.json
to be read-onlyExpected behavior
Terminal launches normally, and honors the current contents of the settings file. The new WSL flavor does not show up in the new tab options, since it is not present in the settings.
Actual behavior
Terminal launches and displays the following error message:
My assumption here is that Terminal is encountering an exception trying to "fix" my settings file for me on launch, and just bubbles up this generic error dialog. This can be worked around by just not marking the settings file as read-only, or specifying the
disabledProfileSources
option with at leastWindows.Terminal.Wsl
in it, but I'm reporting this because it seems a bit heavy-handed to tell me my settings are completely unusable just because Terminal can't replace my settings.The text was updated successfully, but these errors were encountered: