Minimally invasive notification counter #12987

pinzart90 · 2022-06-09T15:15:53Z

Purpose

Generates log of all notifications generated in Dynamo specifically:

WPF binding PropertyChanged events
Model NotificationObject's PropertyChanged events
ObservableCollection CollectionChanged events

A notification counter that generates/updates a notifications.log file in Dynamo's executing assembly folder (every 5 seconds - if any notifications exist). The log file can be deleted at any time (even while Dynamo is running)
Notification logs will always try to be merged/added back to existing logs in the notifications.log folder. Deleting the notifications.log basically allow you to do a fresh count of notifications (should wait 5 seconds after deleting to make sure no cached notifications are added)

Debug builds will have a new dll in the bin folder - 0Harmony.dll
Release builds will not have the new dll.

Instructions to use:
Debug builds :

Create/update the debug.config file (in Dynamo's executing assembly folder) to include the NotificationCounter debug mode.
After Dynamo starts, the debug mode can be switched off.

Release builds :

Copy the 0Harmony.dll from nuget in Dynamo's executing assembly folder
Create/update the debug.config file (in Dynamo's executing assembly folder) to include the NotificationCounter debug mode.

Example debug.config

<?xml version="1.0" encoding="utf-8"?>
<DebugModes>
    <DebugMode name="NotificationCounter" enabled="true"/>
</DebugModes>

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

mjkkirschner · 2022-06-09T19:13:54Z

src/DynamoCore/Utilities/NotificationCounter.cs

+
+ private static void Timer_Elapsed(object sender, ElapsedEventArgs e)
+ {
+ try


why the locks? Will a timer ever fire concurrently?

Yes. The timer can fire multiple times ...even if a previous handler has not finished yet.
Locking the filePath ensure there is no concurrent access to the file content.

src/DynamoCore/Utilities/NotificationCounter.cs

pinzart90 · 2022-06-22T17:41:59Z

src/DynamoCore/DynamoCore.csproj

- </PackageReference>
+ <PackageReference Include="Lib.Harmony" Version="2.2.1" ExcludeAssets="runtime" Condition="'$(Configuration)' != 'Debug'" />
+ <PackageReference Include="Lib.Harmony" Version="2.2.1" ExcludeAssets="none" Condition="'$(Configuration)' == 'Debug'" />
+ <PackageReference Include="DynamoVisualProgramming.LibG_227_0_0" Version="2.13.0.8745" GeneratePathProperty="true">


Debug builds will copy the harmony dll to the bin folder.
Release builds will not copy it.

QilongTang · 2022-06-30T16:01:01Z

Our team is also touching the notifications display. Is this meant to change how the current Notifications view extension works?

mjkkirschner · 2022-08-16T16:56:41Z

@QilongTang I'm pretty sure "Notifications" here refers to property changes /binding updates in WPF.

mjkkirschner · 2022-09-12T18:44:17Z

@pinzart90 do you think we could call this something like binding events or something like that instead of notifications to disambiguate this from the user notifications work?

mjkkirschner · 2022-09-12T18:48:11Z

src/DynamoCore/DynamoCore.csproj

@@ -21,6 +21,8 @@
 <Reference Include="System.Configuration" />
 </ItemGroup>
 <ItemGroup>
+ <PackageReference Include="Lib.Harmony" Version="2.2.1" ExcludeAssets="runtime" Condition="'$(Configuration)' != 'Debug'" />


so in release builds we exclude the runtime assets, and in debug builds we don't... we need the non debug condition so the build succeeds?

I don't know if this means we should add harmony to the about box, license info etc since even though we don't distribute it in release builds we still pull it.

Ugh..you're right. That would need legal clarification.
I rather have devs need to pull it manually than go through another round with legal

haha, what about using reflection to call it so theres no compile time dep - that would be one less setup step for devs who are not going to use this I guess.

maybe I should make this a package or put it in an existing package like TuneUp

the other thing to consider @pinzart90 is that MIT is a preapproved license so I think you can just use it and update the license info without discussion.

about using reflection to call it so theres no compile time dep
Makes development really tough ...
I updated the about box, as you suggested

mjkkirschner

just a few more comments!

also if you reboot dynamo will the notifications log be merged between sessions?

mjkkirschner · 2022-09-13T20:06:14Z

LICENSE.txt

@@ -530,6 +530,16 @@ Licensing information: © Autodesk, Inc. All Rights Reserved.

 The Artifakt font software is Autodesk proprietary and confidential, and may be used only by authorized users and only for Autodesk business purposes. Any use not authorized by Autodesk is not permitted and is an infringement of Autodesk's intellectual property rights as well as a breach of your agreement with Autodesk. Go to https://brand.autodesk.com/brand-system/typography for detailed usage guidelines on when and how to use the Artifakt designer collection.

+Lib.Harmony v.2.2.1
+Copyright (c) 2017 Andreas Pardeike


can you add the MIT license type here as well so it's easy to identify without comparing legalese?

mjkkirschner · 2022-09-13T20:09:08Z

src/DynamoCore/DynamoCore.csproj

@@ -128,7 +130,6 @@
 <NodeHelpFiles Include="$(SolutionDir)..\doc\distrib\NodeHelpFiles\**\*.*" />
 <OpenSourceLicenses Include="$(SolutionDir)..\doc\distrib\Open Source Licenses\**\*.*" />
 <LocalizedResources Include="$(SolutionDir)..\extern\Localized\**\*.*" />
- <ExternAnalytics Include="$(SolutionDir)..\extern\Analytics.NET\*.*" />


another one I missed, why is this removed? Should it have been removed as part of your other PR?

mjkkirschner · 2022-09-13T20:09:46Z

src/DynamoCore/Utilities/NotificationCounter.cs

+ internal int counter;
+ }
+
+ internal static class NotificationCounter


I think this name is still confusing for devs working on notifications and will be in the future as well, especially since this class has no summary.

mjkkirschner · 2022-09-13T20:18:48Z

@pinzart90 it also appears that the test job for this PR has failed a few times in a row while running the tests, log is not telling me much but have not deep dived, it seems to be running on dyn_perf_002 - maybe take a look on that machine.

pinzart added 2 commits June 9, 2022 11:15

add notification counter

e7c9806

Create 0Harmony.dll

a8d45a0

mjkkirschner reviewed Jun 9, 2022

View reviewed changes

src/DynamoCore/Utilities/NotificationCounter.cs Outdated Show resolved Hide resolved

pinzart added 4 commits June 9, 2022 15:22

Update NotificationCounter.cs

cd99617

Merge branch 'master' into notification_counter

1419249

Merge branch 'master' into notification_counter

9adef43

update

3c8f090

pinzart90 commented Jun 22, 2022

View reviewed changes

pinzart90 marked this pull request as ready for review June 22, 2022 17:52

pinzart added 4 commits September 12, 2022 14:35

Merge branch 'master' into notification_counter

2af1875

Update DynamoCore.csproj

db17d9c

Update DynamoCore.csproj

33f7a75

Update DebugModes.cs

bb91563

pinzart90 requested review from aparajit-pratap, sm6srw and mjkkirschner September 12, 2022 18:43

mjkkirschner reviewed Sep 12, 2022

View reviewed changes

pinzart added 3 commits September 12, 2022 14:53

Update DebugModes.cs

d1ea83f

Update NotificationCounter.cs

21db321

update about box

02d8d07

pinzart90 requested a review from mjkkirschner September 13, 2022 13:51

mjkkirschner reviewed Sep 13, 2022

View reviewed changes

pinzart90 added 3 commits December 28, 2023 22:27

Merge branch 'master' into notification_counter

517867f

Update DynamoCore.csproj

c180346

Update DynamoCore.csproj

a9dbeb4

sm6srw marked this pull request as draft January 16, 2024 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimally invasive notification counter #12987

Minimally invasive notification counter #12987

pinzart90 commented Jun 9, 2022 •

edited

mjkkirschner Jun 9, 2022

pinzart90 Jun 9, 2022

pinzart90 Jun 22, 2022

QilongTang commented Jun 30, 2022

mjkkirschner commented Aug 16, 2022

mjkkirschner commented Sep 12, 2022

mjkkirschner Sep 12, 2022

pinzart90 Sep 12, 2022

mjkkirschner Sep 12, 2022 •

edited

pinzart90 Sep 12, 2022

mjkkirschner Sep 12, 2022

pinzart90 Sep 13, 2022

mjkkirschner left a comment

mjkkirschner Sep 13, 2022

mjkkirschner Sep 13, 2022

mjkkirschner Sep 13, 2022

mjkkirschner commented Sep 13, 2022

Minimally invasive notification counter #12987

Are you sure you want to change the base?

Minimally invasive notification counter #12987

Conversation

pinzart90 commented Jun 9, 2022 • edited

Purpose

Declarations

Release Notes

Reviewers

FYIs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QilongTang commented Jun 30, 2022

mjkkirschner commented Aug 16, 2022

mjkkirschner commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Sep 12, 2022 • edited

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

mjkkirschner 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

mjkkirschner commented Sep 13, 2022

pinzart90 commented Jun 9, 2022 •

edited

mjkkirschner Sep 12, 2022 •

edited