Loading user plugins from app data folder. #6551

maraf · 2019-05-12T08:57:25Z

Solves #6415.

In addition to standard plugin location, this change adds:

In portable mode: GE loads plugins from <GE>\UserPlugins.
In installed mode: GE loads plugins from <GEAppData>\UserPlugins.

Questions:

The implementation places user plugins in Roaming profile, where most of GE stuff is placed, but I have found that Local is also used by GE - window positions are saved there. Should user plugins be Roaming or Local?
Method ManagedExtensibility.SetUserPluginsPath should be internal, but needs to be called from PluginRegistry. Can I add AssemblyInternalsVisibleToAttribute?

codecov · 2019-05-12T09:07:46Z

Codecov Report

Merging #6551 into master will increase coverage by 0.12%.
The diff coverage is 97.61%.

@@            Coverage Diff             @@
##           master    #6551      +/-   ##
==========================================
+ Coverage   47.17%   47.29%   +0.12%     
==========================================
  Files         698      701       +3     
  Lines       52668    52700      +32     
  Branches     6944     6943       -1     
==========================================
+ Hits        24844    24924      +80     
+ Misses      26497    26441      -56     
- Partials     1327     1335       +8

Flag	Coverage Δ
#production	`36.6% <97.14%> (+0.14%)`	⬆️
#tests	`97.63% <100%> (+0.03%)`	⬆️

mast-eu

The implementation places user plugins in Roaming profile, where most of GE stuff is placed, but I have found that Local is also used by GE - window positions are saved there. Should user plugins be Roaming or Local?

Please make Local the default for user plugins.

Method ManagedExtensibility.SetUserPluginsPath should be internal, but needs to be called from PluginRegistry. Can I add AssemblyInternalsVisibleToAttribute?

IMHO it's ok. But I would like to hear @RussKie on this point, since I'm not sure of what he thinks.

Plugins/GitUIPluginInterfaces/ManagedExtensibility.cs

GitUI/Plugin/PluginRegistry.cs

Plugins/GitUIPluginInterfaces/ManagedExtensibility.cs

mstv · 2019-05-14T19:47:24Z

Please make Local the default for user plugins.

The intention of Roaming is that when I log on to another PC, my settings including my selected plugins do roam to this PC. So why not Roaming as the default?
(Window positions depend on the local monitor.)

mast-eu · 2019-05-14T21:29:29Z

So why not Roaming as the default?

Suppose you have 2 PCs with some GE version (say 3.2) and some PluginA.
Now you update 1 PC to GE version 4.0, which brings incompatible changes in the plugin API.
The PluginManager finds a new version of PluginA, already compatible with the new API, and installs it in Roaming.
Now you log into PC 2, which did not yet get GE update to version 4.0 (I think it is checking every 7 days for new updates). But the new version of PluginA has already been synced from PC 1 and you have a problem.

API changes will be frequent, at least while we're getting everything up and running.
We can save a lot of trouble by avoiding to sync plugins through roaming. The downside of having to install plugins on multiple PCs shouldn't be a big problem for the majority of our users.

maraf · 2019-05-15T16:44:50Z

Applied changes based on codereview.

Refactored Local profile path from WindowPositionList to AppSettings.
UserPlugins are loaded from Local profile.
Plugins are loaded recursively under plugins folders.

RussKie · 2019-05-16T12:20:24Z

GitCommands/Settings/AppSettings.cs

+                {
+                    return GetGitExtensionsDirectory();
+                }
+                else


this else is redundant and should be removed to reduce nesting

GitUI/WindowPositionList.cs

RussKie · 2019-05-16T12:28:17Z

Plugins/GitUIPluginInterfaces/ManagedExtensibility.cs

+            }
+
+            return result;
+        }


Tests?

Do we need to load all *.dlls or only those matching a specific patten (e.g. GitExtensions.*.dll)?

Is there a perf risk of searching for a looooooong time? 🤔

Yeah, tests. I'm sorry, I'm going to add them.

I had a discussion with @mast-eu that GE org could reserve prefix GitExtensions.* on nuget.org, so that official/core plugins could be in this form and community plugins will have any other pattern.

The idea is that there will only be 1 level of folder nesting.

The current plugins have no common naming scheme. So, at least for the moment, we need to load all DLLs from defaultPluginPath. A decision about enforcing a naming scheme or not can be taken later.

No, I don't expect a performance impact. I didn't perform any tests yet though.

GE org could reserve prefix GitExtensions.* on nuget.org

👍 Any hints how to do that?

3. The idea is that there will only be 1 level of folder nesting.

I'm afraid we can't assume that. We need to be strict in our implementation and don't go deeper than one level.

GE org could reserve prefix GitExtensions.* on nuget.org

👍 Any hints how to do that?

Send email to account@nuget.org, documentation https://docs.microsoft.com/en-us/nuget/reference/id-prefix-reservation#id-prefix-reservation-application-process.

Unfortunately I have already pushed some packages matching GitExtensions.*, so I would need to unlist them and tranfer to GE org (which I'm ok with). Just let me know when GE org nuget account is ready.

The idea is that there will only be 1 level of folder nesting.

I'm afraid we can't assume that. We need to be strict in our implementation and don't go deeper than one level.

Ok than.

mast-eu

2 minor comments.

GitUI/Plugin/PluginRegistry.cs

GitUI/WindowPositionList.cs

maraf · 2019-05-16T17:09:55Z

@RussKie What do you think about this?

Method ManagedExtensibility.SetUserPluginsPath should be internal, but needs to be called from PluginRegistry. Can I add AssemblyInternalsVisibleToAttribute?

RussKie · 2019-05-17T05:24:34Z

@RussKie What do you think about this?

Method ManagedExtensibility.SetUserPluginsPath should be internal, but needs to be called from PluginRegistry. Can I add AssemblyInternalsVisibleToAttribute?

Do you see any problems with ManagedExtensibility.SetUserPluginsPath remaining public?

maraf · 2019-05-17T07:25:23Z

@RussKie What do you think about this?

Method ManagedExtensibility.SetUserPluginsPath should be internal, but needs to be called from PluginRegistry. Can I add AssemblyInternalsVisibleToAttribute?

Do you see any problems with ManagedExtensibility.SetUserPluginsPath remaining public?

Only that any plugin can call it, but if it's ok for you, I'm ok with it.

maraf · 2019-05-17T08:48:10Z

Resolved notes from code review vol.2.
Not sure about the unit test, is it ok this way?
Should I squash the commits?

RussKie · 2019-05-17T09:04:30Z

Only that any plugin can call it..

Can we make so that once it is initialized it can't be changed?

…

On Fri, May 17, 2019, 5:25 PM Marek Fišera ***@***.***> wrote: @RussKie <https://github.com/RussKie> What do you think about this? Method ManagedExtensibility.SetUserPluginsPath should be internal, but needs to be called from PluginRegistry. Can I add AssemblyInternalsVisibleToAttribute? Do you see any problems with ManagedExtensibility.SetUserPluginsPath remaining public? Only that any plugin can call it, but if it's ok for you, I'm ok with it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6551?email_source=notifications&email_token=ABBTEXWN2SUOQ24H5H4PHRTPVZMWTA5CNFSM4HMJ4XVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVT7CRQ#issuecomment-493351238>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBTEXUCT4VXPU5RMIYA3HTPVZMWTANCNFSM4HMJ4XVA> .

maraf · 2019-05-17T16:30:25Z

Can we make so that once it is initialized it can't be changed?

Sure.

mast-eu · 2019-05-17T22:27:02Z

Just browsed through the unit tests. They look ok.

mast-eu · 2019-05-17T22:28:53Z

I think commits should be squashed before merging.

RussKie · 2019-05-18T00:11:00Z

I'm currently away, so if you happy with it - merge it.

…

On Sat, May 18, 2019, 8:29 AM Martin Steinisch ***@***.***> wrote: I think commits should be squashed before merging. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6551?email_source=notifications&email_token=ABBTEXWJPYOF2ATSJ4VRIODPV4WSXA5CNFSM4HMJ4XVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVV77UA#issuecomment-493617104>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBTEXWWTS5N6UQEECHHYG3PV4WSXANCNFSM4HMJ4XVA> .

RussKie · 2019-05-18T06:34:21Z

UnitTests/Plugins/GitUIPluginInterfacesTests/PluginsPathScannerTests.cs

+    [TestFixture]
+    public class PluginsPathScannerTests
+    {
+        [TestCase(@"..\..\PathScanningData", "PluginInRootDir.dll", "PluginInOwnDir.dll")]


Could you clarify it for me (I'm a bit slow, sorry).

Plugins are stored in c:\Users\<user>\Local\GitExtensions\GitExtensions\UserPlugins folder:

UserPlugins \Plugin1 \Plugin1.dll \Plugin1.config \Plugin2 \Plugin2.dll \Plugin2.config

In this scenario this looks like:

PathScanningData \PluginInRootDir \PluginInRootDir.dll \NotFoundPlugin \SubDir \NotFoundPlugin.dll1.dll \PluginInRootDir.dll

Why are we expecting to load PluginInRootDir.dll?

Backward compatibility. I'm using the method for both core and user plugins. Is it ok? Also PM now extracts plugins in a single directory.

Ah, makes sense, thank you.
all good

maraf · 2019-05-18T07:48:52Z

I'm this weekend away, so I'm going to squash them on Monday.

RussKie · 2019-05-18T12:34:50Z

Plugins/GitUIPluginInterfaces/PluginsPathScanner.cs

+                {
+                    DirectoryInfo directory = new DirectoryInfo(pluginsPath);
+
+                    result = Enumerable.Concat(result, directory.GetFiles("*.dll"));


I would add a paragraph explaining why we are loading two levels of assemblies.
E.g. load plugins located in the folder to provide back compat, and load new style plugin...
Maybe even add a structure to help visualise

UserPlugins \OldPlugin1.dll \OldPlugin2.dll \OldPlugin2.config \NewPlugin1 \Plugin1.dll \Plugin1.config \NewPlugin2 \Plugin2.dll \Plugin2.config

mast-eu · 2019-05-18T13:13:10Z

Will merge after squash.

In portable mode it loads plugins from <GE>\UserPlugins. In installed mode it loads plugins from <GEAppData>\UserPlugins.

maraf · 2019-05-20T19:06:12Z

Squashed commits and added detailed comment to the PluginsPathScanner.cs.

maraf · 2019-05-20T19:32:38Z

Any hint on what is wrong with the build?
All GitUITests failed..

mast-eu · 2019-05-20T21:10:59Z

Any hint on what is wrong with the build?

Like with most software, sometimes AppVeyor builds just need to be restarted ... 😉

mast-eu · 2019-05-20T21:12:35Z

Thank you.

ghost added the 👓 status: needs review label May 12, 2019

mast-eu added this to In progress in Move plugins via automation May 13, 2019

mast-eu self-assigned this May 13, 2019

mast-eu added area: plugins 🖊️ status: cla signed labels May 13, 2019

Move plugins automation moved this from In progress to Review in progress May 14, 2019

mast-eu requested changes May 14, 2019

View reviewed changes

Plugins/GitUIPluginInterfaces/ManagedExtensibility.cs Outdated Show resolved Hide resolved

GitUI/Plugin/PluginRegistry.cs Outdated Show resolved Hide resolved

Plugins/GitUIPluginInterfaces/ManagedExtensibility.cs Show resolved Hide resolved

RussKie reviewed May 16, 2019

View reviewed changes

mast-eu reviewed May 16, 2019

View reviewed changes

GitUI/Plugin/PluginRegistry.cs Outdated Show resolved Hide resolved

GitUI/WindowPositionList.cs Outdated Show resolved Hide resolved

ghost assigned maraf May 17, 2019

ghost added status: ready and removed 👓 status: needs review labels May 17, 2019

Move plugins automation moved this from Review in progress to Reviewer approved May 17, 2019

mast-eu approved these changes May 17, 2019

View reviewed changes

RussKie reviewed May 18, 2019

View reviewed changes

RussKie approved these changes May 18, 2019

View reviewed changes

mast-eu mentioned this pull request May 20, 2019

Roadmap: move plugins #6542

Open

37 tasks

Loading user plugins from app data folder.

b58670d

In portable mode it loads plugins from <GE>\UserPlugins. In installed mode it loads plugins from <GEAppData>\UserPlugins.

maraf force-pushed the issue-6415 branch from 0acc730 to b58670d Compare May 20, 2019 19:05

mast-eu closed this May 20, 2019

Move plugins automation moved this from Reviewer approved to Done May 20, 2019

mast-eu reopened this May 20, 2019

Move plugins automation moved this from Done to In progress May 20, 2019

mast-eu merged commit 706e248 into gitextensions:master May 20, 2019

Move plugins automation moved this from In progress to Done May 20, 2019

mast-eu mentioned this pull request May 20, 2019

Plugin installation path #6415

Closed

RussKie removed the status: ready label May 20, 2019

maraf deleted the issue-6415 branch May 24, 2019 14:48

mast-eu mentioned this pull request Jun 25, 2019

Reintegration of PluginManager. #6664

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loading user plugins from app data folder. #6551

Loading user plugins from app data folder. #6551

maraf commented May 12, 2019 •

edited

Loading

codecov bot commented May 12, 2019 •

edited

Loading

mast-eu left a comment

mstv commented May 14, 2019

mast-eu commented May 14, 2019

maraf commented May 15, 2019

RussKie May 16, 2019

maraf May 17, 2019

RussKie May 16, 2019

maraf May 16, 2019

mast-eu May 16, 2019

RussKie May 16, 2019

maraf May 17, 2019 •

edited

Loading

mast-eu left a comment

maraf commented May 16, 2019

RussKie commented May 17, 2019

maraf commented May 17, 2019

maraf commented May 17, 2019

RussKie commented May 17, 2019 via email

maraf commented May 17, 2019

mast-eu commented May 17, 2019

mast-eu commented May 17, 2019

RussKie commented May 18, 2019 via email

RussKie May 18, 2019

maraf May 18, 2019

RussKie May 18, 2019

maraf commented May 18, 2019

RussKie May 18, 2019

maraf May 20, 2019

mast-eu commented May 18, 2019

maraf commented May 20, 2019

maraf commented May 20, 2019 •

edited

Loading

mast-eu commented May 20, 2019

mast-eu commented May 20, 2019

Loading user plugins from app data folder. #6551

Loading user plugins from app data folder. #6551

Conversation

maraf commented May 12, 2019 • edited Loading

codecov bot commented May 12, 2019 • edited Loading

Codecov Report

mast-eu left a comment

Choose a reason for hiding this comment

mstv commented May 14, 2019

mast-eu commented May 14, 2019

maraf commented May 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maraf May 17, 2019 • edited Loading

Choose a reason for hiding this comment

mast-eu left a comment

Choose a reason for hiding this comment

maraf commented May 16, 2019

RussKie commented May 17, 2019

maraf commented May 17, 2019

maraf commented May 17, 2019

RussKie commented May 17, 2019 via email

maraf commented May 17, 2019

mast-eu commented May 17, 2019

mast-eu commented May 17, 2019

RussKie commented May 18, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maraf commented May 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mast-eu commented May 18, 2019

maraf commented May 20, 2019

maraf commented May 20, 2019 • edited Loading

mast-eu commented May 20, 2019

mast-eu commented May 20, 2019

maraf commented May 12, 2019 •

edited

Loading

codecov bot commented May 12, 2019 •

edited

Loading

maraf May 17, 2019 •

edited

Loading

maraf commented May 20, 2019 •

edited

Loading