Use API call to save instead of direct call #427

taooceros · 2021-05-11T12:27:29Z

close #388

TODO:

Implement a auto call to Save all plugin's json storage that has registered
Implement a better (safer) store to replace Dictionary<Type, object>
Finish up some settings

taooceros · 2021-05-11T12:29:19Z

Flow.Launcher/PublicAPIInstance.cs

        public void LogException(string className, string message, Exception e, [CallerMemberName] string methodName = "") => Log.Exception(className, message, e, methodName);

-        private readonly Dictionary<Type, dynamic> PluginJsonStorages = new Dictionary<Type, dynamic>();
+        private readonly Dictionary<Type, object> PluginJsonStorages = new Dictionary<Type, object>();


dynamic seems not working as intended, so I move back to direct cast

…parate Save

jjw24 · 2021-05-12T08:59:02Z

Did you want me to review and merge, or you still working on the other two todos?

taooceros · 2021-05-12T09:04:07Z

Did you want me to review and merge, or you still working on the other two todos?

still working on it. There are a few things that need to be cleaned up.
I am struggling with the second one, but it would be fine to not do it.

taooceros · 2021-05-12T11:48:58Z

I didn't find a better way to replace the Dictionary<Type, object> and reflection for registration. So just review the current one.

jjw24 · 2021-05-12T12:00:44Z

Version bumps for plugins please :)

taooceros · 2021-05-12T12:01:49Z

Version bumps for plugins please :)

Sure

jjw24 · 2021-05-12T22:19:08Z

Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs

-        public void Save()
-        {
-            _storage.Save();
-        }
-


why is this removed? settings still needs to be saved right?

I add a auto save for all registered jsonstorage, so that we don't need to configure isavable for setting, unless they are going to save something else.

Flow.Launcher/Flow.Launcher/PublicAPIInstance.cs

Lines 139 to 146 in b37f9f3

private void SaveAllJsonStorage()

{

foreach (var value in _pluginJsonStorages.Values)

{

var method = value.GetType().GetMethod("Save");

method?.Invoke(value, null);

}

}

would this not be a doubling up then, with PluginManager.Save() call?

also you need to move this SaveAllJsonStorage() call to here right so when Flow's settings window closed you want to save all plugin settings:

Flow.Launcher/Flow.Launcher/ViewModel/SettingWindowViewModel.cs

Lines 85 to 97 in 9d4af83

public void Save()

{

foreach (var vm in PluginViewModels)

{

var id = vm.PluginPair.Metadata.ID;

Settings.PluginSettings.Plugins[id].Disabled = vm.PluginPair.Metadata.Disabled;

Settings.PluginSettings.Plugins[id].Priority = vm.Priority;

}

PluginManager.Save();

_storage.Save();

}

do we want to remove ISavable?

if it's auto saved, might want to document it somewhere right? for those that develop plugins

I think this method is called in SaveAllJsonStorage, so it is impossible to directly call that method here, or else will become infinity recurse.
The call for

PluginManager.Save(); _storage.Save();

do save all plugin setting.

yeah i see. hmm how do save plugin settings when user closes settings window? PluginManager.Save() only saves plugin settings where the interface ISavable is applied and _storage.Save() saves Flow's own settings.

instead of passing the api to the view, which is not even used, we can and should pass it to the viewmodel, this then allows you to call save plugin settings seperately from the view

Flow.Launcher/Flow.Launcher/SettingWindow.xaml.cs

Line 28 in 1e687a2

public SettingWindow(IPublicAPI api, SettingWindowViewModel viewModel)

PluginManager.Save() only saves plugin settings where the interface ISavable is applied and _storage.Save() saves Flow's own settings.

I have modified PluginManager.Save() to include saving plugins settings.

public static void Save() { foreach (var plugin in AllPlugins) { var savable = plugin.Plugin as ISavable; savable?.Save(); } API.SavePluginSettings(); }

jjw24 · 2021-05-12T22:32:08Z

Not related to this change but I think we should update the comment to give an example scenario where you will need to override the original class instance, i.e. use the override save method and when you just need to use save the original instance. Since you are in this space maybe you could just give a good explanation in the comment that would be awesome.

Flow.Launcher/Flow.Launcher.Plugin/IPublicAPI.cs

Lines 174 to 188 in b37f9f3

    
           /// <summary> 
        
           /// Save JsonStorage for current plugin. This is the method used to save settings to json in Flow.Launcher 
        
           /// This method will save the original instance loaded with LoadJsonStorage. 
        
           /// </summary> 
        
           /// <typeparam name="T">Type for Serialization</typeparam> 
        
           /// <returns></returns> 
        
           void SaveJsonStorage<T>() where T : new(); 
        
           /// <summary> 
        
           /// Save JsonStorage for current plugin. This is the method used to save settings to json in Flow.Launcher 
        
           /// This method will override the original class instance loaded from LoadJsonStorage 
        
           /// </summary> 
        
           /// <typeparam name="T">Type for Serialization</typeparam> 
        
           /// <returns></returns> 
        
           void SaveJsonStorage<T>(T settings) where T : new();

taooceros · 2021-05-13T05:36:49Z

I have updated the comment. In fact, I don't believe we may actually need the one to overwrite it, because the supposed process is to load and use the loaded file (which will be a new when none exist), and save the original one. Should we remove it?

Also, do you think we should modify the name specifically to setting because the file will be stored inside the setting folder.

By the way, would you please give me the access for OAuth from Jetbrains IDE integration? Sometimes I may use rider instead of Visual Studio or vscode. Thank you!

jjw24 · 2021-05-13T08:20:55Z

By the way, would you please give me the access for OAuth from Jetbrains IDE integration? Sometimes I may use rider instead of Visual Studio or vscode. Thank you!

Done

jjw24 · 2021-05-13T11:19:11Z

I have updated the comment. In fact, I don't believe we may actually need the one to overwrite it, because the supposed process is to load and use the loaded file (which will be a new when none exist), and save the original one. Should we remove it?

i am unsure when there will be a circumstance where we needed to save a new instance to the original, so i would say yes

Also, do you think we should modify the name specifically to setting because the file will be stored inside the setting folder.

yes

taooceros · 2021-05-13T11:24:49Z

By the way, would you please give me the access for OAuth from Jetbrains IDE integration? Sometimes I may use rider instead of Visual Studio or vscode. Thank you!

Done

Thank you!

jjw24 · 2021-05-16T07:03:54Z

Flow.Launcher.Plugin/IPublicAPI.cs

+        /// <summary>
+        /// Save Flow's plugins settings
+        /// </summary>
+        void SavePluginSettings();


what's the difference between this method and SaveSettingJsonStorage()?

SaveSettingJsonStorage is for a specific Type (or T as generic), but this is for all plugin.

jjw24 · 2021-05-16T07:04:48Z

Flow.Launcher.Plugin/IPublicAPI.cs

-        /// Save JsonStorage for current plugin. This is the method used to save settings to json in Flow.Launcher
+        /// Save JsonStorage for current plugin's setting. This is the method used to save settings to json in Flow.Launcher
        /// This method will save the original instance loaded with LoadJsonStorage.
+        /// This API call is for manually Save. Flow will automatically save all setting that has registered. 


i think it will be better to also explain how setting gets registered in the comment so plugin devs do not need to dig into code to find out.

…cher/Flow.Launcher into MigrateDirectPluginJsonStorage

Flow.Launcher.Plugin/IPublicAPI.cs

jjw24 · 2021-05-17T21:52:55Z

Plugins/Flow.Launcher.Plugin.Explorer/ViewModels/SettingsViewModel.cs

        public void Save()
        {
-            storage.Save();
+            Context.API.SaveSettingJsonStorage<Settings>();
        }


one more question, why does this have to call the save directly?

Explorer plugin contains a call for save every time a new item in its setting is edited. I didn't change it.
It is not ISavable, so I believe it won't be called twice. However, do you think we should remove that?

oh right, it's not ISavable, just calls infra's save directly. Nah don't worry about it for this PR, using the API is at least better. Let's get this merged in.

Move most direct call of PluginJsonStorage to API call

33accbd

taooceros commented May 11, 2021

View reviewed changes

taooceros added 2 commits May 12, 2021 00:37

add auto save for all setting that has been registered, and remove Se…

25f6e66

…parate Save

fix some typo

a9695e3

Move storage to api call for websearch and calculator plugins

358460c

taooceros requested a review from jjw24 May 12, 2021 11:49

Version Bumps

b37f9f3

jjw24 assigned taooceros May 12, 2021

jjw24 added the bug Something isn't working label May 12, 2021

jjw24 reviewed May 12, 2021

View reviewed changes

Update Comment

dc36633

Update Comment

dea26c6

taooceros added 2 commits May 13, 2021 19:29

Remove unneccessary api, rename storage api, and update comment

d84eff7

Add SavePluginSettings API, and call that api in PluginManager.Save();

20ba2b2

jjw24 added the review in progress Indicates that a review is in progress for this PR label May 14, 2021

jjw24 reviewed May 16, 2021

View reviewed changes

jjw24 and others added 3 commits May 16, 2021 17:11

Update IPublicAPI.cs

97b2c2a

Update Comment

baceb8c

Merge branch 'MigrateDirectPluginJsonStorage' of github.com:Flow-Laun…

e565022

…cher/Flow.Launcher into MigrateDirectPluginJsonStorage

taooceros commented May 16, 2021

View reviewed changes

Flow.Launcher.Plugin/IPublicAPI.cs Outdated Show resolved Hide resolved

jjw24 added 6 commits May 16, 2021 17:14

update comments

65eb4e8

Merge branch 'dev' into MigrateDirectPluginJsonStorage

909fe5a

update ISavable comments

a25f836

version bump PluginsManager

efee45b

version bump Shell

5980528

revert

1e687a2

jjw24 reviewed May 17, 2021

View reviewed changes

jjw24 enabled auto-merge May 18, 2021 22:04

jjw24 added enhancement New feature or request and removed bug Something isn't working review in progress Indicates that a review is in progress for this PR labels May 18, 2021

jjw24 approved these changes May 18, 2021

View reviewed changes

jjw24 merged commit be8bb8b into dev May 18, 2021

jjw24 deleted the MigrateDirectPluginJsonStorage branch May 18, 2021 22:05

jjw24 mentioned this pull request May 18, 2021

Release 1.8.0 | Plugin 2.0.0 #363

Merged

taooceros mentioned this pull request Jun 21, 2021

Add backwards compatibility for Everything plugin v1.4.9 #481

Merged

	private void SaveAllJsonStorage()
	{
	foreach (var value in _pluginJsonStorages.Values)
	{
	var method = value.GetType().GetMethod("Save");
	method?.Invoke(value, null);
	}
	}

	public void Save()
	{
	foreach (var vm in PluginViewModels)
	{
	var id = vm.PluginPair.Metadata.ID;

	Settings.PluginSettings.Plugins[id].Disabled = vm.PluginPair.Metadata.Disabled;
	Settings.PluginSettings.Plugins[id].Priority = vm.Priority;
	}

	PluginManager.Save();
	_storage.Save();
	}

Uh oh!

Use API call to save instead of direct call #427

Use API call to save instead of direct call #427

Uh oh!

Conversation

taooceros commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjw24 commented May 12, 2021

Uh oh!

taooceros commented May 12, 2021

Uh oh!

taooceros commented May 12, 2021

Uh oh!

jjw24 commented May 12, 2021

Uh oh!

taooceros commented May 12, 2021

Uh oh!

jjw24 May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjw24 commented May 12, 2021

Uh oh!

taooceros commented May 13, 2021

Uh oh!

jjw24 commented May 13, 2021

Uh oh!

jjw24 commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taooceros commented May 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjw24 May 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taooceros May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

taooceros commented May 11, 2021 •

edited

Loading

jjw24 May 12, 2021 •

edited

Loading

jjw24 commented May 13, 2021 •

edited

Loading

jjw24 May 16, 2021 •

edited

Loading

taooceros May 18, 2021 •

edited

Loading