Fixing multiple output windows bug #16

alexgav · 2018-03-03T05:28:22Z

… was already static. However, all of the web editors code checks for pane existence prior to creating one. Not sure how it can exist if our static is null, but it appears to be possible. So checking for existence before creating seems to fix that.

I still went ahead and added (ProjectName) suffix to messages. However, the current logging design is kinf of iffy. Existing Log was a essentially a static class, but wasn't declared as such because Mads wanted it to implement ILogger interface. So we were actually creating multiple instances of that essentially static class... Really weird. Anyway, I created PerProjectLogger that has instance data now, and converted Logger into the actual static class it was really supposed to be. So PerProjectLogger suffixes messages with project name, while static messages of the Logger don't... It doesn't look horrible, but a bit weird. We can discuss the design more. Below is example output restoring jquery in two different projects in the same solution.

Restoring jquery@3.3.0... (WebApplication1)

1 libraries restored in 0.22 seconds

Restoring jquery@3.3.0... (WebApplication3)

1 libraries restored in 0.07 seconds

… was already static. However, all of the web editors code checks for pane existence prior to creating one. Not sure how it can exist if our static is null, but it appears to be possible. So checking for existence before creating seems to fix that. I still went ahead and added (ProjectName) suffix to messages. However, the current logging design is kinf of iffy. Existing Log was a essentially a static class, but wasn't declared as such because Mads wanted it to implement ILogger interface. So we were actually creating multiple instances of that essentially static class... Really weird. Anyway, I created PerProjectLogger that has instance data now, and converted Logger into the actual static class it was really supposed to be. So PerProjectLogger suffixes messages with project name, while static messages of the Logger don't... It doesn't look horrible, but a bit weird. We can discuss the design more. Below is example output restoring jquery in two different projects in the same solution. Restoring jquery@3.3.0... (WebApplication1) 1 libraries restored in 0.22 seconds Restoring jquery@3.3.0... (WebApplication3) 1 libraries restored in 0.07 seconds

raquelsa · 2018-03-07T22:13:54Z

src/LibraryManager.Vsix/Contracts/PerProjectLogger.cs

+
+ public void Log(string message, LogLevel level)
+ {
+ Logger.Log($"{message} ({_projectName})", level);


_projectName can be null here if we fail to get projectName

Should we have a fallback string for a default if we can't resolve a project name?

Not sure what the default project name would be. I think if it's null, we should just omit it. Anything else would look bad to the users. I'm not sure in which case project name would be null though - do you guys know of such cases?

Anyway - I added null check and will be using empty string for now if there is no project name - so the output would be as before this change.

jimmylewis · 2018-03-08T08:46:06Z

src/LibraryManager.Vsix/Contracts/HostInteraction.cs

 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

 using EnvDTE;


nit: usings aren't sorted correctly (System first)

jimmylewis

Also, could you clean up the commit message to be more in line with good commit message practices?

jimmylewis · 2018-03-08T08:50:12Z

src/LibraryManager.Vsix/Contracts/HostInteraction.cs

 {
 string cwd = Path.GetDirectoryName(configFilePath);
 WorkingDirectory = cwd;
+ Logger = new PerProjectLogger(VsHelpers.GetProjectName(configFilePath));


Can we use a DI pattern for the Logger to make this class more testable?

Sure, taking it as a parameter now and added internal setter as well.

jimmylewis · 2018-03-08T08:50:32Z

src/LibraryManager.Vsix/Contracts/Logger.cs

 using Microsoft.Web.LibraryManager.Contracts;
 using Microsoft.VisualStudio.Shell.Interop;
 using System;
+using Microsoft.VisualStudio;


nit: sort usings

Cleaned up.

jimmylewis · 2018-03-08T08:52:58Z

src/LibraryManager.Vsix/Contracts/Logger.cs

 namespace Microsoft.Web.LibraryManager.Vsix
 {
- public class Logger : ILogger
+ public static class Logger


We should chat about this. I prefer to avoid static classes if possible, they're hard to test. It would be better IMO if we had a mechanism to manage the lifetime of the instance (like an IoC container would provide).

It also feels weird that Logger doesn't implement ILogger, but another type does and all it does is call methods on Logger. That seems like it ought to be using an inherited class instead.

I don't think inheritance between PerProjectLogger and Logger would be appropriate here as "Logger" is essentially a VS Log Sink, just a wrapper around Status Bar & OutputWindow, and there will always be just one of them. PerProjectLogger gets created by the project, use by external "clients", and sends output to the VS Log Sink. So I think the composition is more appropriate here than inheritance.

As far as static objects vs singletons and making things testable, I totally agree in general. In this particular case, Logger is simply a very thin wrapper around VS Output Window and Status Bar, so I don't think it makes sense to test it via unit tests. I'll leave it as static for now, and we can chat more about it if you want.

jimmylewis · 2018-03-08T08:53:15Z

src/LibraryManager.Vsix/Contracts/Logger.cs

- public class Logger : ILogger
+ public static class Logger
 {
+ private static Guid _outputPaneGuid = new Guid("cce35aef-ace6-4371-b1e1-8efa3cdc8324");


This should be readonly.

Just kidding, can't be, passed by ref to a few methods below (even though they don't change it).

jimmylewis · 2018-03-08T09:05:06Z

src/LibraryManager.Vsix/Contracts/PerProjectLogger.cs

+
+namespace Microsoft.Web.LibraryManager.Vsix.Contracts
+{
+ internal class PerProjectLogger : ILogger


As commented on Logger, it feels like this ought to be extending the Logger class (before it was static). HostInteraction would get an instance of this type through DI.

See my comments above. We can chat more about it in person.

jimmylewis · 2018-03-08T09:08:37Z

src/LibraryManager.Vsix/Contracts/PerProjectLogger.cs

+
+ public void Log(string message, LogLevel level)
+ {
+ Logger.Log($"{message} ({_projectName})", level);


Should we have a fallback string for a default if we can't resolve a project name?

jimmylewis · 2018-03-08T09:26:05Z

src/LibraryManager.Vsix/Shared/VsHelpers.cs

+
+ IVsUIShellOpenDocument vsUIShellOpenDocument = ServiceProvider.GlobalProvider.GetService(typeof(SVsUIShellOpenDocument)) as IVsUIShellOpenDocument;
+
+ IOleServiceProvider serviceProviderUnused = null;


nit: you don't need this, you can just use out _ in the method call and it will be discarded.

https://docs.microsoft.com/en-us/dotnet/csharp/discards

same with docInProject

Nice, didn't know about these, thanks!

… was already static. However, all of the web editors code checks for pane existence prior to creating one. Not sure how it can exist if our static is null, but it appears to be possible. So checking for existence before creating seems to fix that. I still went ahead and added (ProjectName) suffix to messages. However, the current logging design is kinf of iffy. Existing Log was a essentially a static class, but wasn't declared as such because Mads wanted it to implement ILogger interface. So we were actually creating multiple instances of that essentially static class... Really weird. Anyway, I created PerProjectLogger that has instance data now, and converted Logger into the actual static class it was really supposed to be. So PerProjectLogger suffixes messages with project name, while static messages of the Logger don't... It doesn't look horrible, but a bit weird. We can discuss the design more. Below is example output restoring jquery in two different projects in the same solution. Restoring jquery@3.3.0... (WebApplication1) 1 libraries restored in 0.22 seconds Restoring jquery@3.3.0... (WebApplication3) 1 libraries restored in 0.07 seconds

alexgav requested review from bhsubra, jimmylewis, jodavis and raquelsa March 3, 2018 05:28

raquelsa reviewed Mar 7, 2018

View reviewed changes

jimmylewis reviewed Mar 8, 2018

View reviewed changes

jimmylewis requested changes Mar 8, 2018

View reviewed changes

alexgav added 2 commits April 3, 2018 17:34

Rebased

4959a78

alexgav changed the title ~~Fixing bug 568432 (multiple output windows). Actually the output pane…~~ Fixing multiple output windows bug Apr 6, 2018

alexgav closed this Jun 27, 2018

jimmylewis deleted the dev/alexgav/Bug568432 branch June 27, 2018 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixing multiple output windows bug #16

Fixing multiple output windows bug #16

Uh oh!

alexgav commented Mar 3, 2018

raquelsa Mar 7, 2018

jimmylewis Mar 8, 2018

alexgav Apr 6, 2018

jimmylewis Mar 8, 2018

jimmylewis left a comment

jimmylewis Mar 8, 2018

alexgav Apr 6, 2018

jimmylewis Mar 8, 2018

alexgav Apr 6, 2018

jimmylewis Mar 8, 2018

jimmylewis Mar 8, 2018

alexgav Apr 6, 2018

jimmylewis Mar 8, 2018

alexgav Apr 6, 2018

alexgav Apr 6, 2018

jimmylewis Mar 8, 2018

alexgav Apr 6, 2018

jimmylewis Mar 8, 2018

jimmylewis Mar 8, 2018

jimmylewis Mar 8, 2018

jimmylewis Mar 8, 2018

alexgav Apr 6, 2018

Labels

4 participants


		IVsUIShellOpenDocument vsUIShellOpenDocument = ServiceProvider.GlobalProvider.GetService(typeof(SVsUIShellOpenDocument)) as IVsUIShellOpenDocument;

		IOleServiceProvider serviceProviderUnused = null;

Fixing multiple output windows bug #16

Fixing multiple output windows bug #16

Uh oh!

Conversation

alexgav commented Mar 3, 2018

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

jimmylewis 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

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

Labels

4 participants