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

Make all features switchable #525

Merged
merged 7 commits into from Mar 24, 2018

Conversation

Projects
None yet
2 participants
@heku
Contributor

heku commented Feb 3, 2018

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

This comment has been minimized.

Owner

codecadwallader commented Feb 13, 2018

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! :)

@codecadwallader codecadwallader self-assigned this Feb 13, 2018

@heku

This comment has been minimized.

Contributor

heku commented Feb 13, 2018

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

This comment has been minimized.

Contributor

heku commented Mar 1, 2018

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

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Mar 1, 2018

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

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Mar 11, 2018

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

@heku

This comment has been minimized.

Contributor

heku commented Mar 11, 2018

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).

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 11, 2018

Owner

What is /Initial Call Times/?

This comment has been minimized.

@heku

heku Mar 12, 2018

Contributor

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.

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 11, 2018

Owner

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

This comment has been minimized.

@heku

heku Mar 12, 2018

Contributor

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 20, 2018

Owner

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.

This comment has been minimized.

@heku

heku Mar 20, 2018

Contributor

Yes.

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 11, 2018

Owner

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.

This comment has been minimized.

@heku

heku Mar 12, 2018

Contributor

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

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 11, 2018

Owner

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

This comment has been minimized.

@heku

heku Mar 12, 2018

Contributor

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 20, 2018

Owner

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

@@ -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)

This comment has been minimized.

@codecadwallader

codecadwallader Mar 11, 2018

Owner

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?

This comment has been minimized.

@heku

heku Mar 12, 2018

Contributor

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.

This comment has been minimized.

@codecadwallader

codecadwallader Mar 20, 2018

Owner

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

This comment has been minimized.

@heku

heku Mar 20, 2018

Contributor

Yes.

@@ -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.

This comment has been minimized.

@codecadwallader

codecadwallader Mar 11, 2018

Owner

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

This comment has been minimized.

@heku

heku Mar 12, 2018

Contributor

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.

This comment has been minimized.

@codecadwallader

codecadwallader Mar 20, 2018

Owner

Ok, that makes sense. Thanks for the explanation!

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 11, 2018

Owner

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).

This comment has been minimized.

@heku

heku Mar 12, 2018

Contributor

Agree, I'll pull a request later.

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 11, 2018

Owner

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

This comment has been minimized.

@heku

heku Mar 12, 2018

Contributor

Agree.

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 11, 2018

Owner

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?

This comment has been minimized.

@heku

heku Mar 12, 2018

Contributor

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.

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 11, 2018

Owner

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?

This comment has been minimized.

@heku

heku Mar 12, 2018

Contributor

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

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

This comment has been minimized.

Contributor

heku commented Mar 19, 2018

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!

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 20, 2018

Owner

Ok, thanks for confirming!

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 20, 2018

Owner

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.

@@ -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)

This comment has been minimized.

@codecadwallader

codecadwallader Mar 20, 2018

Owner

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

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

This comment has been minimized.

@codecadwallader

codecadwallader Mar 20, 2018

Owner

Ok, thanks for improving the consistency.

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Mar 24, 2018

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 codecadwallader merged commit a5953d0 into codecadwallader:master Mar 24, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Mar 24, 2018

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

This comment has been minimized.

Owner

codecadwallader commented Mar 24, 2018

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

@heku

This comment has been minimized.

Contributor

heku commented Mar 24, 2018

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.

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

@heku heku deleted the heku:switch branch Mar 25, 2018

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

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Apr 7, 2018

@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!

2018-04-07_12-30-03

devenv_2018-04-07_12-51-56

@heku

This comment has been minimized.

Contributor

heku commented Apr 7, 2018

@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

This comment has been minimized.

Owner

codecadwallader commented Apr 9, 2018

@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

This comment has been minimized.

Contributor

heku commented Apr 9, 2018

Great, I have no problem now. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment