Skip to content
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

SLVS-1561 Close notifications when solution is closed #5781

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public void Show_GeneratesCorrectNotificationStructure()
notificationActions[1].CommandText.Should().Be(BindingStrings.SharedBindingSuggestionInfoOptionText);
notificationActions[1].ShouldDismissAfterAction.Should().BeFalse();
notificationActions[2].Should().BeSameAs(doNotShowAgainMock.Object);
notification.CloseOnSolutionClose.Should().Be(true);
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,12 @@ public void Show(ServerType serverType, Action onConnectHandler)
var notification = new Notification(
id: string.Format(IdTemplate, solutionInfoProvider.GetSolutionName()),
message: string.Format(BindingStrings.SharedBindingSuggestionMainText, serverType),
actions: new INotificationAction[]
{
actions:
[
new NotificationAction(BindingStrings.SharedBindingSuggestionConnectOptionText, _ => onConnectHandler(), true),
new NotificationAction(BindingStrings.SharedBindingSuggestionInfoOptionText, _ => OnLearnMore(), false),
doNotShowAgainNotificationAction
});
]);

notificationService.ShowNotification(notification);
}
Expand Down
56 changes: 49 additions & 7 deletions src/Core.UnitTests/Notifications/NotificationServiceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public class NotificationServiceTests
{
private IInfoBarManager infoBarManager;
private IDisabledNotificationsStorage disabledNotificationsStorage;
private IActiveSolutionTracker activeSolutionTracker;
private IThreadHandling threadHandling;
private TestLogger logger;
private NotificationService testSubject;
Expand All @@ -38,9 +39,10 @@ public void TestInitialize()
{
infoBarManager = Substitute.For<IInfoBarManager>();
disabledNotificationsStorage = Substitute.For<IDisabledNotificationsStorage>();
activeSolutionTracker = Substitute.For<IActiveSolutionTracker>();
threadHandling = Substitute.For<IThreadHandling>();
logger = new TestLogger();
testSubject = new NotificationService(infoBarManager, disabledNotificationsStorage, threadHandling, logger);
testSubject = new NotificationService(infoBarManager, disabledNotificationsStorage, activeSolutionTracker, threadHandling, logger);

AllowRunningUiThreadActions();
}
Expand All @@ -51,6 +53,7 @@ public void MefCtor_CheckIsExported()
MefTestHelpers.CheckTypeCanBeImported<NotificationService, INotificationService>(
MefTestHelpers.CreateExport<IInfoBarManager>(),
MefTestHelpers.CreateExport<IDisabledNotificationsStorage>(),
MefTestHelpers.CreateExport<IActiveSolutionTracker>(),
MefTestHelpers.CreateExport<IThreadHandling>(),
MefTestHelpers.CreateExport<ILogger>());
}
Expand Down Expand Up @@ -105,7 +108,7 @@ public void ShowNotification_AttachesInfoBarToWindow_RunsOnUIThread()
Action showInfoBar = null;
var captureParamThreadHandling = Substitute.For<IThreadHandling>();
captureParamThreadHandling.RunOnUIThreadAsync(Arg.Do<Action>(arg => showInfoBar = arg));
var threadHandledTestSubject = new NotificationService(infoBarManager, disabledNotificationsStorage, captureParamThreadHandling, logger);
var threadHandledTestSubject = new NotificationService(infoBarManager, disabledNotificationsStorage, activeSolutionTracker, captureParamThreadHandling, logger);

// Trigger the ShowNotification without running the action to validate that the UI action is the one responsible for showing the notification
threadHandledTestSubject.ShowNotification(notification);
Expand Down Expand Up @@ -255,7 +258,38 @@ public void ShowNotification_WithToolWindowGuid_ShowOnToolWindow()

AssertNotificationWasShown(attachedNotification, aToolWindowId);
}

[TestMethod]
public void ActiveSolutionChangedEvent_WhenNoActiveNotification_NotificationNotClosed()
{
activeSolutionTracker.ActiveSolutionChanged += Raise.Event<EventHandler<ActiveSolutionChangedEventArgs>>(this, new ActiveSolutionChangedEventArgs(false));

infoBarManager.DidNotReceive().CloseInfoBar(Arg.Any<IInfoBar>());
}

[TestMethod]
public void ActiveSolutionChangedEvent_WhenSolutionClosedAndCloseOnSolutionClose_NotificationCloses()
{
var attachedNotification = ShowNotification(closeOnSolutionClose: true);

activeSolutionTracker.ActiveSolutionChanged += Raise.Event<EventHandler<ActiveSolutionChangedEventArgs>>(this, new ActiveSolutionChangedEventArgs(false));

AssertNotificationClosed(attachedNotification);
}

[TestMethod]
[DataRow(false, false)]
[DataRow(true, false)]
[DataRow(true, true)]
public void ActiveSolutionChangedEvent_WhenSolutionOpenedOrNotCloseOnSolutionClose_NotificationDoesNotClose(bool isSolutionOpen, bool closeOnSolutionClose)
{
var attachedNotification = ShowNotification(closeOnSolutionClose: closeOnSolutionClose);

activeSolutionTracker.ActiveSolutionChanged += Raise.Event<EventHandler<ActiveSolutionChangedEventArgs>>(this, new ActiveSolutionChangedEventArgs(isSolutionOpen));

AssertNotificationNotClosed(attachedNotification);
}

[TestMethod]
public void Dispose_ClosesNotification()
{
Expand All @@ -266,6 +300,14 @@ public void Dispose_ClosesNotification()
AssertNotificationClosed(attachedNotification);
}

[TestMethod]
public void Dispose_UnsubscribesFromActiveSolutionChangeEvents()
{
testSubject.Dispose();

activeSolutionTracker.Received(1).ActiveSolutionChanged -= Arg.Any<EventHandler<ActiveSolutionChangedEventArgs>>();
}

[TestMethod]
public void ShowNotification_WhenNonCriticalException_ExceptionCaught()
{
Expand Down Expand Up @@ -321,7 +363,7 @@ public void CloseNotification_ClosesInfoBar_RunsOnUIThread()
Action runThreadAction = null;
var captureParamThreadHandling = Substitute.For<IThreadHandling>();
captureParamThreadHandling.RunOnUIThreadAsync(Arg.Do<Action>(arg => runThreadAction = arg));
var threadHandledTestSubject = new NotificationService(infoBarManager, disabledNotificationsStorage, captureParamThreadHandling, logger);
var threadHandledTestSubject = new NotificationService(infoBarManager, disabledNotificationsStorage, activeSolutionTracker, captureParamThreadHandling, logger);

// Show a notification to be able to close it
threadHandledTestSubject.ShowNotification(notification);
Expand Down Expand Up @@ -368,18 +410,18 @@ private void AllowRunningUiThreadActions()
threadHandling.RunOnUIThreadAsync(Arg.Do<Action>(arg => arg()));
}

private AttachedNotification ShowNotification(string id = null, bool oncePerSession = true, bool disabled = false, params INotificationAction[] actions)
private AttachedNotification ShowNotification(string id = null, bool oncePerSession = true, bool disabled = false, bool closeOnSolutionClose = true, params INotificationAction[] actions)
{
var notification = CreateNotification(id, oncePerSession, disabled, actions);
var notification = CreateNotification(id, oncePerSession, disabled, closeOnSolutionClose, actions);
var attachedNotification = AttachNotification(notification);
testSubject.ShowNotification(notification);
return attachedNotification;
}

private INotification CreateNotification(string id = null, bool oncePerSession = true, bool disabled = false, params INotificationAction[] actions)
private INotification CreateNotification(string id = null, bool oncePerSession = true, bool disabled = false, bool closeOnSolutionClose = true, params INotificationAction[] actions)
{
var notificationId = id ?? Guid.NewGuid().ToString();
var notification = new Notification(notificationId, notificationId, actions, oncePerSession);
var notification = new Notification(notificationId, notificationId, actions, oncePerSession, closeOnSolutionClose);

disabledNotificationsStorage.IsNotificationDisabled(notificationId).Returns(disabled);

Expand Down
27 changes: 24 additions & 3 deletions src/Core/Notifications/INotificationService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,10 @@ public interface INotificationService : IDisposable
internal sealed class NotificationService : INotificationService
{
private static readonly Guid MainWindowId = Guid.Empty;

private readonly IInfoBarManager infoBarManager;
private readonly IDisabledNotificationsStorage notificationsStorage;
private readonly IActiveSolutionTracker activeSolutionTracker;
private readonly IThreadHandling threadHandling;
private readonly ILogger logger;

Expand All @@ -56,17 +57,22 @@ internal sealed class NotificationService : INotificationService
private AttachedNotification activeNotification;

[ImportingConstructor]
public NotificationService(IInfoBarManager infoBarManager,
public NotificationService(
IInfoBarManager infoBarManager,
IDisabledNotificationsStorage notificationsStorage,
IActiveSolutionTracker activeSolutionTracker,
IThreadHandling threadHandling,
ILogger logger)
{
this.infoBarManager = infoBarManager;
this.notificationsStorage = notificationsStorage;
this.activeSolutionTracker = activeSolutionTracker;
this.threadHandling = threadHandling;
this.logger = logger;

this.activeSolutionTracker.ActiveSolutionChanged += OnActiveSolutionChanged;
}

public void ShowNotification(INotification notification)
{
ShowNotification(notification, MainWindowId);
Expand Down Expand Up @@ -190,10 +196,25 @@ private AttachedNotification AttachInfoBar(INotification notification, Guid tool

return new AttachedNotification(notification, infoBar);
}

private void OnActiveSolutionChanged(object sender, ActiveSolutionChangedEventArgs e)
{
if (activeNotification == null)
{
return;
}

if (activeNotification.Notification.CloseOnSolutionClose && !e.IsSolutionOpen)
{
CloseNotification();
}
}

public void Dispose()
{
CloseNotification();

activeSolutionTracker.ActiveSolutionChanged -= OnActiveSolutionChanged;
}

private sealed record AttachedNotification(INotification Notification, IInfoBar InfoBar);
Expand Down
8 changes: 4 additions & 4 deletions src/Core/Notifications/Notification.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System;
using System.Collections.Generic;

namespace SonarLint.VisualStudio.Core.Notifications
{
public interface INotification
Expand All @@ -30,21 +27,24 @@ public interface INotification
IEnumerable<INotificationAction> Actions { get; }

bool ShowOncePerSession { get; }
bool CloseOnSolutionClose { get; }
}

public class Notification : INotification
{
public Notification(string id, string message, IEnumerable<INotificationAction> actions, bool showOncePerSession = true)
public Notification(string id, string message, IEnumerable<INotificationAction> actions, bool showOncePerSession = true, bool closeOnSolutionClose = true)
{
Id = id ?? throw new ArgumentNullException(nameof(id));
Message = message ?? throw new ArgumentNullException(nameof(message));
Actions = actions ?? throw new ArgumentNullException(nameof(actions));
ShowOncePerSession = showOncePerSession;
CloseOnSolutionClose = closeOnSolutionClose;
}

public string Id { get; }
public string Message { get; }
public IEnumerable<INotificationAction> Actions { get; }
public bool ShowOncePerSession { get; }
public bool CloseOnSolutionClose { get; }
}
}
2 changes: 0 additions & 2 deletions src/Integration/MefServices/SharedBindingSuggestionService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,9 @@
*/

using System.ComponentModel.Composition;
using System.Windows;
using SonarLint.VisualStudio.ConnectedMode.Binding.Suggestion;
using SonarLint.VisualStudio.ConnectedMode.Shared;
using SonarLint.VisualStudio.ConnectedMode.UI;
using SonarLint.VisualStudio.ConnectedMode.UI.ManageBinding;
using SonarLint.VisualStudio.Core;
using SonarLint.VisualStudio.Core.Binding;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
using SonarLint.VisualStudio.Core;
using SonarLint.VisualStudio.Core.Notifications;
using SonarLint.VisualStudio.SLCore.NodeJS.Notifications;
using Language = SonarLint.VisualStudio.SLCore.Common.Models.Language;

namespace SonarLint.VisualStudio.SLCore.UnitTests.NodeJS.Notifications
{
Expand All @@ -37,11 +38,11 @@ public void MefCtor_CheckIsExported()
}

[DataTestMethod]
[DataRow(SLCore.Common.Models.Language.JS, "321", "123", "123")]
[DataRow(SLCore.Common.Models.Language.CSS, "99.00.11", "9876.5432", "9876.5432")]
[DataRow(SLCore.Common.Models.Language.JS, "321", null, "Not found")]
[DataRow(SLCore.Common.Models.Language.TS, "5.4.3.2.1", null, "Not found")]
public void Show_ShowsCorrectMessageAndNotificationId(SLCore.Common.Models.Language language, string expectedVersion, string actualVersion, string displayActualVersion)
[DataRow(Language.JS, "321", "123", "123")]
[DataRow(Language.CSS, "99.00.11", "9876.5432", "9876.5432")]
[DataRow(Language.JS, "321", null, "Not found")]
[DataRow(Language.TS, "5.4.3.2.1", null, "Not found")]
public void Show_ShowsCorrectMessageAndNotificationId(Language language, string expectedVersion, string actualVersion, string displayActualVersion)
{
INotification createdNotification = null;
var notificationService = CreateNotificationService(n => createdNotification = n);
Expand All @@ -60,6 +61,7 @@ public void Show_ShowsCorrectMessageAndNotificationId(SLCore.Common.Models.Langu
createdNotification.Id.Should().Be("sonarlint.nodejs.min.version.not.found");
createdNotification.Message.Should().Be($"SonarLint: {language} analysis failed. Could not find a Node.js runtime (required: >={expectedVersion}, actual: {displayActualVersion}) on your computer.");
createdNotification.Actions.Count().Should().Be(2);
createdNotification.CloseOnSolutionClose.Should().Be(false);
}

[TestMethod]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System.Linq;
using SonarLint.VisualStudio.Core.Notifications;
using SonarLint.VisualStudio.SLCore.Notification;

Expand Down Expand Up @@ -59,6 +58,7 @@ public void Show_CreatesNotificationAndCallsShow()
notification.Id.Should().Be("sonarlint.sloop.restart.failed");
notification.Message.Should().Be("SonarLint background service failed to start");
notification.Actions.Should().HaveCount(1);
notification.CloseOnSolutionClose.Should().Be(false);

notification.Actions.First().Action(notification);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,13 @@ public void Show(string languageName, string minVersion, string currentVersion =
notificationService.ShowNotification(new VisualStudio.Core.Notifications.Notification(
id: NotificationId,
message: string.Format(NotificationStrings.NotificationUnsupportedNode, languageName, minVersion, currentVersion ?? NotificationStrings.NotificationNoneVersion),
actions: new INotificationAction[]
{
actions:
[
new NotificationAction(NotificationStrings.NotificationShowMoreInfoAction, _ => ShowMoreInfo(), false),
doNotShowAgainNotificationAction
}));
],
closeOnSolutionClose: false)
);
}

private void ShowMoreInfo()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System;
using System.ComponentModel.Composition;
using SonarLint.VisualStudio.Core.Notifications;

Expand Down Expand Up @@ -52,7 +51,8 @@ public void Show(Action act)
{
new NotificationAction(SLCoreStrings.SloopRestartFailedNotificationService_Restart, _ => act(), true)
},
showOncePerSession: false
showOncePerSession: false,
closeOnSolutionClose: false
);

notificationService.ShowNotification(notification);
Expand Down