Skip to content
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

Transitioned to Json Settings #6113

Merged
merged 34 commits into from
Oct 7, 2021
Merged

Conversation

d2dyno1
Copy link
Member

@d2dyno1 d2dyno1 commented Sep 13, 2021

Resolved / Related Issues

Details of Changes
This PR transitions registry based SettingsViewModel to json settings. Now importing and exporting your settings is possible!
This PR also introduces BaseJsonSettingsModel v3 with refactored codebase and ISettingsSharingContext for sharing one settings file across multiple BaseJsonSettingsModel instances. This allows for settings organization while keeping everything in one place.

Benefits

  • More performant settings access thanks to caching
  • Flexible settings store allows serialization and deserialization of various object types
  • All-new BaseJsonSettingsModel v3 with reworked codebase and support of sharing settings instances
  • Flexible settings access stores all settings in one file which allows easy importing/exporting of your configuration.

Regressions

  • Potential regressions and bugs may occur

Note for Contributors

  • This PR also includes IServiceProvider for injecting dependencies into classes (currently only used for injecting setting services). The IoC container was added in mind of further expansion.
  • SettingsViewModel is deprecated and should be no longer used for storing settings. It is only used for merging the settings database.
  • With these changes also comes a new way to access settings:

BEFORE:

bool val = App.AppSettings.ShowBundlesWidget;

AFTER:

IUserSettingsService userSettingsService = Ioc.Default.GetService<IUserSettingsService>();
bool val = userSettingsService.WidgetsSettingsService.ShowBundlesWidget;

Validation

  • Built the app

For #4180, #5195

@d2dyno1 d2dyno1 marked this pull request as ready for review September 15, 2021 18:40
@d2dyno1
Copy link
Member Author

d2dyno1 commented Sep 15, 2021

@yaichenbaum Ready for review!

@winston-de
Copy link
Contributor

I'm getting a null reference exception at startup on line 338 of MainPageViewModel.cs

image

System.NullReferenceException: 'Object reference not set to an instance of an object.'

InterfaceDispatchCell00ADAFE8_$0_Files.Services.IUserSettingsService_slot_9(...) returned null.

@gave92
Copy link
Member

gave92 commented Sep 19, 2021

@d2dyno1 I've pushed a few changes. Feel free to revert them if you disagree. Basically:

  • Added registration for StartupSettingsService in App.xaml.cs, should fix issue reported by @winston-de
  • Removed old settings definitions from SettingsViewModel (they were only used for porting)
  • Track settings changes through AppCenter

@d2dyno1
Copy link
Member Author

d2dyno1 commented Sep 19, 2021

I've just noticed them, thank you ❤️ From the initial look, these look fine so I'm going to keep them in. I'll merge some other settings soon and then it's finished!

@d2dyno1
Copy link
Member Author

d2dyno1 commented Sep 19, 2021

@d2dyno1 you probably know this already but since the new settings are not "observable" some things do not work, e.g. toggling the preview pane or hiding/showing hidden files.

We did hook up PropertyChanged event (found in SettingsViewModel) to get notified when a setting is updated, I've changed it since and now we hook up the OnSettingChangedEvent. Is that not working? Could you please test for me if this line is executed? (since VS is preventing me from running the app) @gave92

@gave92
Copy link
Member

gave92 commented Sep 19, 2021

The problem is that settings do not implement INotifyPropertyChanged so this kind of things don't work : IsChecked="{x:Bind UserSettingsService.PreviewPaneSettingsService.PreviewPaneEnabled, Mode=TwoWay}".

@d2dyno1
Copy link
Member Author

d2dyno1 commented Oct 6, 2021

@gave92 I've fixed the issue. The problem actually had nothing to do with INotifyPropertyChanged. It was simply due to events firing in different classes -> now they're redirected to one event BaseJsonSettingsModel.OnSettingChangedEvent (in IUserSettings) through ISettingsSharingContext

@d2dyno1 d2dyno1 requested a review from gave92 October 6, 2021 17:06
@gave92
Copy link
Member

gave92 commented Oct 6, 2021

Looks mostly fixed 👍 This setting does not work without restart: IsVerticalTabFlyoutEnabled (seems the only one that relied on INotifyPropertyChanged, all the rest are handled in code behind through settings changed event)

@BanCrash
Copy link
Contributor

BanCrash commented Oct 6, 2021

@d2dyno1 could you check if a fresh install is working?

If I make a fresh install of this branch, it will give me a lot of errors and won't work. If I install first last main and then install this version so it gets updated, it works as expected.

@d2dyno1
Copy link
Member Author

d2dyno1 commented Oct 6, 2021

@d2dyno1 could you check if a fresh install is working?

If I make a fresh install of this branch, it will give me a lot of errors and won't work. If I install first last main and then install this version so it gets updated, it works as expected.

I've just ran a fresh build of iusersettings. Didn't have any problems with it

@BanCrash
Copy link
Contributor

BanCrash commented Oct 6, 2021

@d2dyno1 could you check if a fresh install is working?
If I make a fresh install of this branch, it will give me a lot of errors and won't work. If I install first last main and then install this version so it gets updated, it works as expected.

I've just ran a fresh build of iusersettings. Didn't have any problems with it

That's odd, I just tried again uninstalling Files-Dev and then debugging as debug and it throws a lot of exceptions about "Value cannot be null.". Are you debugging on debug or on release?

@d2dyno1
Copy link
Member Author

d2dyno1 commented Oct 6, 2021

Debug x64

Copy link
Member

@gave92 gave92 left a comment

Choose a reason for hiding this comment

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

LRGTM (R is for "really")

@d2dyno1
Copy link
Member Author

d2dyno1 commented Oct 6, 2021

Lereallygimate lol

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Oct 7, 2021
@yaira2 yaira2 merged commit a7fb38f into files-community:main Oct 7, 2021
@d2dyno1 d2dyno1 deleted the iusersettings branch October 7, 2021 05:24
@BanCrash BanCrash removed their request for review October 27, 2021 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transition settings to json format
5 participants