fix: Restrict settings to "global" when not in a repository #11955
Conversation
|
Do we really need to pass this flag to all settings pages? It does not apply to more than half of them. |
It can't because we don't have access to I discovered that we can get it through I can continue the refactoring to require @RussKie I gave it a quick try in the last commit. Look if you find it better...
The constructors of all the setting pages has to be updated due to how they are instantiated.
I don't think so (and it seems to work without problem) |
|
I'm planning to review this, hopefully before the EOW. |
| { | ||
| _canSaveInsideRepo = pageHost.CheckSettingsLogic.CommonLogic.Module.IsValidGitWorkingDir(); |
There was a problem hiding this comment.
What's the concrete type of serviceProvider? It's an instance of GitUICommands, which implements IGitUICommands and, thus, inherits from IServiceProvider.
Which makes this possible:
if (serviceProvider is IGitUICommands uiCommands)
{
_canSaveInsideRepo = uiCommands.Module.IsValidGitWorkingDir();
}It's a little hacky, but we some point I expect we'll have something like the following:
IGitUICommands uiCommands = serviceProvider.GetREquiredService<IGitUICommands>();There was a problem hiding this comment.
Which makes this possible:
I saw that but it was (too) hacky for me that's why I have not decided to go that way.
It's you to decide...
There was a problem hiding this comment.
In the end, ISettingsPageHost, that still need to be passed is now passed in the constructor to have it earlier than in a later call to do some initialization. It's indeed a lot of code changes but nothing silly...
There was a problem hiding this comment.
The current change introduces a hard coupling, and this I'd like to avoid.
Let's resolve from the service provider
and so when a setting could only be saved globally. Fixes gitextensions#11907
2579834 to
698ef8c
Compare
and so that settings file can't be saved in a "not existing" local repository.
Check that repo is valid and provide the information when creating pages
Fixes #11907 (at least prevent one way to reproduce it)
Screenshots
Before
After
Test methodology
Test environment(s)
Merge strategy
I agree that the maintainer squash merge this PR (if the commit message is clear).
(Will be squashed before merging)
✒️ I contribute this code under The Developer Certificate of Origin.