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.

Sign up for GitHub

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

Jump to bottom

Disable Network Traffice When Notification Center Disabled #13286

Merged
merged 2 commits into from
Sep 2, 2022
Merged

Disable Network Traffice When Notification Center Disabled #13286

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3434c0
update
QilongTang Sep 2, 2022
5f5f76d
Remain public
QilongTang Sep 2, 2022
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
19 changes: 10 additions & 9 deletions src/Notifications/NotificationCenterController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Collections.Generic;
using System.Configuration;
using System.IO;
using System.Linq;
using System.Net;
Expand All @@ -11,7 +9,6 @@
using System.Windows;
using System.Windows.Controls;
using System.Windows.Controls.Primitives;
using Dynamo.Configuration;
using Dynamo.Controls;
using Dynamo.Logging;
using Dynamo.Notifications.View;
Expand All @@ -23,11 +20,11 @@ namespace Dynamo.Notifications
{
[ClassInterface(ClassInterfaceType.AutoDual)]
[ComVisible(true)]
public class scriptObject
public class ScriptObject
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a name vialation, I think in C# public class are all capitalized

{
Action<object[]> onMarkAllAsRead;

internal scriptObject(Action<object []> onMarkAllAsRead)
internal ScriptObject(Action<object []> onMarkAllAsRead)
{
this.onMarkAllAsRead = onMarkAllAsRead;
}
Expand All @@ -53,7 +50,7 @@ public class NotificationCenterController
private static readonly string jsEmbeddedFile = "Dynamo.Notifications.node_modules._dynamods.notifications_center.build.index.bundle.js";
private static readonly string NotificationCenterButtonName = "notificationsButton";

private DynamoLogger logger;
private readonly DynamoLogger logger;
private string jsonStringFile;
private NotificationsModel notificationsModel;

Expand All @@ -78,8 +75,12 @@ internal NotificationCenterController(DynamoView view, DynamoLogger dynLogger)
notificationUIPopup.webView.EnsureCoreWebView2Async();
notificationUIPopup.webView.CoreWebView2InitializationCompleted += WebView_CoreWebView2InitializationCompleted;
logger = dynLogger;

RequestNotifications();
// If user turns on the feature, they will need to restart Dynamo to see the count
// This ensures no network traffic when Notification center feature is turned off
if (dynamoViewModel.PreferenceSettings.EnableNotificationCenter)
{
RequestNotifications();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know the RequestNotifications(); only populates the number of notifications in the bell icon but this won't prevent that the webapp consume the REST API (not sure if that is the expected behavior).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RobertGlobant20 This will prevent Dynamo making the REST API calls, do you mean the WebApp will still do it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about simply not loading the extension?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QilongTang if the objective is to prevent just Dynamo to make REST API calls then is OK but if you want also to prevent those calls in the WebApp then is a different story.

Copy link
Contributor Author

@QilongTang QilongTang Sep 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RobertGlobant20 @mjkkirschner My understanding is that the latest version of Notification Center is using process.env to control the endpoint and REST API calls will be made depending on if the endpoint address exist or not. When consumed by Dynamo, the end point is empty. I believe @filipeotero made such change so when used in Dynamo, the WebApp itself should not pull anything but rely on info passed by Dynamo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@filipeotero Please let me know if my understanding is correct. If not, then happy to follow up

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QilongTang You are correct. The webApp itself will not request notifications if process.env.NOTIFICATION_URL is not set and it is empty when the app is consumed by dynamo. Dynamo calls the endpoint and passes notifications to window.setNotifications.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. In Dynamo, the notification center is waiting to push the notifications. Executing the notification center in development mode will grab the notifications by itself.

}
}

private void WebView_NavigationCompleted(object sender, Microsoft.Web.WebView2.Core.CoreWebView2NavigationCompletedEventArgs e)
Expand Down Expand Up @@ -177,7 +178,7 @@ private void WebView_CoreWebView2InitializationCompleted(object sender, Microsof
notificationUIPopup.webView.CoreWebView2.NavigateToString(htmlString);
// Hosts an object that will expose the properties and methods to be called from the javascript side
notificationUIPopup.webView.CoreWebView2.AddHostObjectToScript("scriptObject",
new scriptObject(OnMarkAllAsRead));
new ScriptObject(OnMarkAllAsRead));
}
}

Expand Down