Make all features switchable #525

heku · 2018-02-03T11:16:22Z

Hi, I complete this feature #296 finally. I made lots of changes, for simplifying your review, I give you a few explanations here.

This change aim at make all CodeMaid features switchable, when user switch off a feature, I not only hide its menu(s), because as you know, VS always invoke the OnBeforeQueryStatus method, to reduce any potential overhead, I remove/cut off/close as many as possible things, like remove menu from package's MenuCommandService (so that VS wont invoke OnBeforeQueryStatus any more), cut off all related event listeners (so that VS wont go into our event handlers), close its tool window (e.g. Spade, Build Progress). Regarding event listeners, if possible I only cut off the relationship between VS event and event listener.
VS Event ---- EventListener ---- EventConsumer
VS Event -X- EventListener ---- EventConsumer
Switchable features' menus flags change to DynamicVisibility and DefaultInvisible, so the .vsct file was updated.
An interface ISwitchable is added to indicate a switchable feature. now all commands and event listeners are implemented this interface.
An option page (General->Feature Switch) and a few settings (Feature_xxx) are added.
A class SettingMonitor is added, it's responsible for watching setting(s) and invoke callback(s) when setting(s) changed.
Make all commands and event listeners singleton.
Change the package autoload time to SolutionExistsAndFullyLoaded, because:
- All CodeMaid's features are meaningful only when there is solution/project/code.
- This change can simplify the package initialize code, things like ShellAvailable are removed.
- Don't impact VS launch time any more.
A few other code changes/enhancements.

All unit tests passed, but I'm not able to run the integration tests, I've selected the IntegrationTests.testsettings file as doc. I tried both VS 15.6 preview 3.0 and 15.5.5, if you know the reason, please teach me.

Please have it reviewed and feel free to let me know if you have any question or concern. As I'm not good at English, I don't write too many comments in code, if this is required, I'll try to add them later.

This PR also completes the #335, which you planned for v10.5.

codecadwallader · 2018-02-13T02:06:42Z

Thank you for the pull request, and the explanation of the changes. There's a lot to go over and I need to think through some of the implications so please be patient with me! :)

heku · 2018-02-13T23:25:47Z

Sure, I'm on holiday these days with limited internet access (phone, no PC), I'm not familiar with all code, maybe some places could be optimized. Feel free to ask me to do that or help me do that.

heku · 2018-03-01T05:25:20Z

Hi, I'm back, did you have chance to review these, any question for me?

codecadwallader · 2018-03-01T09:55:37Z

Unfortunately no I have not yet, but it is still on my radar!

codecadwallader · 2018-03-11T13:24:19Z

Thanks again for your patience, I'm starting to look into it. It's really interesting and I like what you have done! :)

heku · 2018-03-11T13:40:57Z

No problem, as I'm not very good at English, feel free to correct any nams/strings in the code.

codecadwallader

Thank you again for your hard work on this, I really appreciate it and I think the users will really appreciate it as well. I have a number of questions and suggestions, please let me know if there's anything I can help clarify or we can figure out together!

Also, as a heads up there is another pull request for localization going on (i.e. to support Chinese menus/labels/etc.). I think there will be a fair amount of conflict between the changes on this pull request and that one so that may be something we have to work through later (that one is further along).

codecadwallader · 2018-03-11T12:50:44Z

CodeMaid.UnitTests/Helpers/SettingMonitorTests.cs

+            int callbackTimes = 0;
+            monitor.Watch(s => s.Feature_CleanupAllCode, _ => callbackTimes++);
+
+            Assert.AreEqual(/*Initial Call Times*/1, callbackTimes);


What is /Initial Call Times/?

When we call monitor.Watch(setting, callback), the Watch method will invoke the callback at once to initialize the callback based on existing setting value for initialization.

codecadwallader · 2018-03-11T13:25:51Z

CodeMaid.config

@@ -16,6 +16,12 @@
      <setting name="Reorganizing_RunAtStartOfCleanup" serializeAs="String">
        <value>False</value>
      </setting>
+      <setting name="Feature_CleanupActiveCode" serializeAs="String">


Why are these features added here, but not all the other ones? I think perhaps they could be removed?

Because I see you force auto cleanup on file save for this repo, this feature requires Feature_CleanupActiveCode and Feature_SettingCleanupOnSave features on.

Ok I see, so you're trying to ensure those features are present in case someone normally has them disabled in their user-level settings.

codecadwallader · 2018-03-11T13:26:52Z

CodeMaid/Helpers/SettingMonitor.cs

+
+namespace SteveCadwallader.CodeMaid.Helpers
+{
+    public sealed class SettingMonitor<TSetting> where TSetting : ApplicationSettingsBase


I prefer to have where statements indented on the next line please.
Also I think this class could be SettingsMonitor since its monitoring a set vs. a single item.

It's ok, I will pull the update later.

codecadwallader · 2018-03-11T13:30:23Z

CodeMaid/Integration/Commands/AboutCommand.cs

+        public static void Initialize(CodeMaidPackage package)
+        {
+            Instance = new AboutCommand(package);
+            Instance.Switch(on: true);


So are all features switched on initially, and then turned back off? Or why does this default to true vs. referencing settings?

I made all features are switchable, but not all menus, the 'About' menu will always be there.

Makes sense, I was looking at About as an example of all of them but I can see why it is unique.

codecadwallader · 2018-03-11T13:33:25Z

CodeMaid/Integration/Commands/BaseCommand.cs

@@ -81,6 +80,18 @@ protected virtual void OnExecute()
            OutputWindowHelper.DiagnosticWriteLine($"{GetType().Name}.OnExecute invoked");
        }

+        public virtual void Switch(bool on)
+        {
+            if (on && Package.MenuCommandService.FindCommand(CommandID) == null)


Why is FindCommand needed before AddCommand, but not before RemoveCommand? If AddCommand/RemoveCommand will gracefully fail if it doesn't exist then its probably redundant. If it won't gracefully fail then it should probably be done in both cases?

By design the Switch method should be idempotent. In my test, I found the AddCommand method will fail when add one command twice, while the RemoveCommand won't fail.

Ok, so it's an optimization in the RemoveCommand to not bother finding the command since it will gracefully fail. Makes sense. 👍

codecadwallader · 2018-03-11T15:49:53Z

CodeMaid/Integration/Events/RunningDocumentTableEventListener.cs

@@ -24,8 +34,14 @@ internal RunningDocumentTableEventListener(CodeMaidPackage package)
            // Create and store a reference to the running document table.
            RunningDocumentTable = new RunningDocumentTable(package);

-            // Register with the running document table for events.
-            EventCookie = RunningDocumentTable.Advise(this);
+            // This listener services multiple features, watching if any of them switched.


As mentioned above in the SettingsMonitor class.. I'm wondering if this couldn't be simplified to just make two watch statements.

Yes but the code would be more complex I think, for example, if settingA and settingB are related to featureA, and featureA only works when both settings checked, we have to check another setting value(s) in callback code.

monitor.watch(settingA, value=> { if(value && Settings.Default.SettingB){ } }) monitor.watch(settingB, value=> { if(value && Settings.Default.SettingA){ } })

Of course we can define a method for both callback parameter

void callbackmethod()=> if(Settings.Default.settingA && Settings.Default.settingB) {} monitor.watch(settingA, callbackmethod ) monitor.watch(settingB, callbackmethod )

But another problem, if both settings changed, the callback will be invoked multiple times, so I let the Watch method accepts mutliple setting parameters.

Ok, that makes sense. Thanks for the explanation!

codecadwallader · 2018-03-11T15:51:57Z

CodeMaid/Integration/ISwitchable.cs

@@ -0,0 +1,7 @@
+namespace SteveCadwallader.CodeMaid.Integration
+{
+    internal interface ISwitchable


I think ISwitchableFeature might be more obvious how it is intended to be used. Even if another "Switch" concept was introduced we would want it to have its own interface (and perhaps they'd both inherit from this more generic interface).

Agree, I'll pull a request later.

codecadwallader · 2018-03-11T15:55:01Z

CodeMaid/UI/Dialogs/Options/General/FeatureViewModel.cs

+            };
+        }
+
+        public override string Header => "Feature Switch";


Lets change this to Features, and rename the classes accordingly (e.g. FeaturesDataTemplate.xaml FeaturesViewModel.cs please.

codecadwallader · 2018-03-11T16:07:42Z

CodeMaid/UI/Dialogs/Options/General/FeatureDataTemplate.xaml

+        xmlns:local="clr-namespace:SteveCadwallader.CodeMaid.UI.Dialogs.Options.General">
+    <DataTemplate DataType="{x:Type local:FeatureViewModel}">
+        <StackPanel>
+            <GroupBox Header="Cleanup">


I think perhaps we should reorganize these settings. One way would be to line them up with the Options (e.g. Cleaning, Collapsing, Digging, Finding, Formatting, Reorganizing, Switching, etc.) but they would be pretty fine grained group boxes. Another way would be to line them up by where they are found in the UI (e.g. Main Menu, Context Menus, Tool Windows). Thoughts?

OK, I'll take the first suggestion, because a feature can have multiple menus in different context (main menus, context menus...), the second one might not work.

codecadwallader · 2018-03-11T16:10:12Z

CodeMaid/Integration/Events/ShellEventListener.cs

@@ -1,160 +0,0 @@
-using Microsoft.VisualStudio;


Without this class, how do you know when the user changes their Visual Studio theme (e.g. from Light to Dark) so that Spade can refresh?

I use the built-in help class instead, see line 331 of CodeMaidPackage.cs

VSColorTheme.ThemeChanged += _ => ThemeManager.ApplyTheme();

heku · 2018-03-19T03:56:28Z

Hi, I commented your questions and committed the code update, feel free to let me know if you have any question.

codecadwallader

Thanks for the updates, those all look good! I don't think I have any further questions right now. I'll need to play around/test it some more before I can merge, but looking great!

codecadwallader · 2018-03-20T11:31:58Z

CodeMaid/Integration/Commands/AboutCommand.cs

+
+        public static AboutCommand Instance { get; private set; }
+
+        public static void Initialize(CodeMaidPackage package)


Ok, thanks for confirming!

codecadwallader · 2018-03-20T11:33:17Z

CodeMaid.config

@@ -16,6 +16,12 @@
      <setting name="Reorganizing_RunAtStartOfCleanup" serializeAs="String">
        <value>False</value>
      </setting>
+      <setting name="Feature_CleanupActiveCode" serializeAs="String">


Ok I see, so you're trying to ensure those features are present in case someone normally has them disabled in their user-level settings.

codecadwallader · 2018-03-20T11:34:10Z

CodeMaid/Integration/Commands/BaseCommand.cs

@@ -81,6 +80,18 @@ protected virtual void OnExecute()
            OutputWindowHelper.DiagnosticWriteLine($"{GetType().Name}.OnExecute invoked");
        }

+        public virtual void Switch(bool on)
+        {
+            if (on && Package.MenuCommandService.FindCommand(CommandID) == null)


Ok, so it's an optimization in the RemoveCommand to not bother finding the command since it will gracefully fail. Makes sense. 👍

codecadwallader · 2018-03-20T11:34:38Z

CodeMaid/Integration/Commands/BuildProgressToolWindowCommand.cs

@@ -33,7 +43,7 @@ private IVsWindowFrame BuildProgressWindowFrame
        {
            get
            {
-                var buildProgress = Package.BuildProgress;
+                var buildProgress = Package.BuildProgressForceLoad;


Ok, thanks for improving the consistency.

codecadwallader · 2018-03-24T12:23:22Z

One thing I was wondering is if someone disables a feature at their user settings level, but the solution they're loading has configuration options expecting it to be present.. what happens? I think since the feature isn't loaded it would simply fail to execute, and that's probably fine. I think when you explicitly turned on features inside CodeMaid's own solution settings was an example of overriding features to be enabled at the solution level (ignoring user preferences). Does that sound right and do you have any other thoughts around that?

codecadwallader · 2018-03-24T12:31:08Z

It is working fantastically - thank you! I will likely make a few tweaks to the options dialog but this is awesome and I'm really excited to have this improvement to the extension! BIG thank you for your time and contribution!

codecadwallader · 2018-03-24T12:41:47Z

This can be tested (by anyone interested) in our CI build here: http://vsixgallery.com/extension/4c82e17d-927e-42d2-8460-b473ac7df316/

heku · 2018-03-24T15:09:36Z

Thx, It's my pleasure. About the setting you're wondering, if a feature is turned off in user setting but configured in solution level, it will be ignored as a turned off feature means all its related code won't be executed.

codecadwallader · 2018-04-07T12:55:10Z

@heku FYI: For the order of the features I came up with a third option where we would organize the commands in the same way they are organized on the main menu. I think this makes the most sense and is more consistent, so I went ahead and made this change. Please let me know if you have any different thoughts!

heku · 2018-04-07T13:25:37Z

@codecadwallader Thank you, most make sense to me, except the Cleanup Selected Code and Cleanup All Code are placed in different group, this might be a little confused, in my opinion, it's better to always put these two items together. Maybe just move the Cleanup Selected Code up to the Cleaning group, or move the Cleanup All Code down to the Solution Explorer group. How do you think? Feel free to disagree my comment if it doesn't make sense.

codecadwallader · 2018-04-09T10:51:26Z

@heku Thanks for the feedback, makes sense. Cleanup All Code exists both inside and outside of the Solution Explorer but is more easily found outside of it. I'm moving Cleanup Selected Code up to the Cleaning section per your suggestion.

heku · 2018-04-09T11:43:44Z

Great, I have no problem now. Thank you.

heku added 6 commits February 3, 2018 17:41

Make all features switchable

b581929

Remove ServiceProvider property, use Package itself instead.

e08b3b9

Use built-in helper class VSColorTheme for theme changed notification.

f8c0d65

Ensure cleanup active code feature always works in current solution.

e555c4c

Ensure Spade inside event handlers register only when feature on

96ac18e

Disable Spade auto show as sometimes it shows but the feature is off.

7558960

codecadwallader self-assigned this Feb 13, 2018

codecadwallader requested changes Mar 11, 2018

View reviewed changes

Fix review problems

c987670

codecadwallader reviewed Mar 20, 2018

View reviewed changes

codecadwallader approved these changes Mar 24, 2018

View reviewed changes

codecadwallader merged commit a5953d0 into codecadwallader:master Mar 24, 2018

maikebing added a commit to maikebing/codemaid that referenced this pull request Mar 24, 2018

update for PR codecadwallader#525

b5cfccb

heku deleted the switch branch March 25, 2018 06:49

codecadwallader added this to the v10.5 milestone Apr 7, 2018

codecadwallader mentioned this pull request Apr 28, 2018

Disable CodeMaid Spade #335

Closed


		public static AboutCommand Instance { get; private set; }

		public static void Initialize(CodeMaidPackage package)

Make all features switchable #525

Make all features switchable #525

Conversation

heku commented Feb 3, 2018 • edited Loading

codecadwallader commented Feb 13, 2018

heku commented Feb 13, 2018

heku commented Mar 1, 2018

codecadwallader commented Mar 1, 2018

codecadwallader commented Mar 11, 2018

heku commented Mar 11, 2018

codecadwallader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heku Mar 12, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heku Mar 12, 2018 • edited Loading

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

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

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

heku commented Mar 19, 2018

codecadwallader left a 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

codecadwallader commented Mar 24, 2018

codecadwallader commented Mar 24, 2018

codecadwallader commented Mar 24, 2018

heku commented Mar 24, 2018

codecadwallader commented Apr 7, 2018 • edited Loading

heku commented Apr 7, 2018

codecadwallader commented Apr 9, 2018

heku commented Apr 9, 2018

heku commented Feb 3, 2018 •

edited

Loading

heku Mar 12, 2018 •

edited

Loading

heku Mar 12, 2018 •

edited

Loading

codecadwallader commented Apr 7, 2018 •

edited

Loading