-
Notifications
You must be signed in to change notification settings - Fork 621
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
Minimally invasive notification counter #12987
base: master
Are you sure you want to change the base?
Conversation
|
||
private static void Timer_Elapsed(object sender, ElapsedEventArgs e) | ||
{ | ||
try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the locks? Will a timer ever fire concurrently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/DynamoCore.csproj
Outdated
</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"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug builds will copy the harmony dll to the bin folder.
Release builds will not copy it.
Our team is also touching the notifications display. Is this meant to change how the current |
@QilongTang I'm pretty sure "Notifications" here refers to property changes /binding updates in WPF. |
@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? |
src/DynamoCore/DynamoCore.csproj
Outdated
@@ -21,6 +21,8 @@ | |||
<Reference Include="System.Configuration" /> | |||
</ItemGroup> | |||
<ItemGroup> | |||
<PackageReference Include="Lib.Harmony" Version="2.2.1" ExcludeAssets="runtime" Condition="'$(Configuration)' != 'Debug'" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I should make this a package or put it in an existing package like TuneUp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
about using reflection to call it so theres no compile time dep
Makes development really tough ...
I updated the about box, as you suggested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a few more comments!
also if you reboot dynamo will the notifications log be merged between sessions?
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add the MIT license type here as well so it's easy to identify without comparing legalese?
src/DynamoCore/DynamoCore.csproj
Outdated
@@ -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\*.*" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another one I missed, why is this removed? Should it have been removed as part of your other PR?
internal int counter; | ||
} | ||
|
||
internal static class NotificationCounter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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. |
Purpose
Generates log of all notifications generated in Dynamo specifically:
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 thenotifications.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 :
NotificationCounter
debug mode.Release builds :
NotificationCounter
debug mode.Example debug.config
Declarations
Check these if you believe they are true
*.resx
filesRelease 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