add package states under dbg mode #11698

pinzart90 · 2021-05-20T12:26:14Z

Purpose

This PR adds package states to the Installed Packages UI.
The UI changes are not final, they UX is still under review.
I also changed around some of the wording used.

UI changes preview

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

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

pinzart90 · 2021-05-20T12:32:34Z

src/DynamoPackages/Package.cs

@@ -113,6 +114,60 @@ public bool MarkedForUninstall
            internal set { markedForUninstall = value; RaisePropertyChanged("MarkedForUninstall"); }
        }

+        public bool EnableOldMarkedForUnistallState


seems that wpf can only bind to public properties...:(
If we want to have it under a debug mode first....we could just tag these temporary public methods...that they should not be used ..?

yeah, one thing we've done in the past is to mark them obsolete immediately - it's confusing, but it let's people know to be wary.

seems that wpf can only bind to public properties...:(
If we want to have it under a debug mode first....we could just tag these temporary public methods...that they should not be used ..?

yeah, I am supportive of both. Obsolete tag with some comments works well, is there any rule if the team can remove obsolete tag within releases? @mjkkirschner

I don't see any harm to removing an obsolete method that we've explicitly called out as DO NOT USE.

I don't see any harm to removing an obsolete method that we've explicitly called out as DO NOT USE.

Yes, that one I agree. I am curious about the case that we obsoleted something but decided that we want to keep it in the end, do we simply remove the obsolete tag?

yep :) - Again, what case concerns you? if no one is using the member for fear it might be removed, then what break could keeping it cause?

pinzart90 · 2021-05-20T20:13:18Z

Not sure about the HIG standards (yet) but this is how I chose to handle long texts:
The package state tag/label will have a max width of 150 pixels
The text will be trimmed after max width is exceeded.
The tooltips have more information : The full text form the label + details.

pinzart90 · 2021-05-21T14:50:46Z

src/DynamoCore/Properties/Resources.en-US.resx

+    <data name="PackageStateUnloadedTooltip" xml:space="preserve">
+    <value>Skipped.
+This package has not been loaded because another conflicting package was loaded before it.</value>
+  </data>


@mjkkirschner @Amoursol @Jingyi-Wen
It has been brought to my attention that the terms used are not really consistent (like loaded vs installed)
What about using the following terms:

Installed - package is loaded into Dynamo and ready to be used.

Scheduled for uninstall - package will be uninstalled after next Dynamo restart.

Skipped - package is available on disk but was not loaded into Dynamo due to conflict with another similar package.

Error - package was not loaded into Dynamo due to an unexpected error

Also what do you think about scheduled vs marked ....for uninstall ?

If you have any alternatives....please tag them with the numbers above so that we know which one is which :)

@pinzart @Jingyi-Wen What do you think about 3. Ignored? I mention it because I believe sometimes in our error messages and notifications we use the same term to describe packages that were not loaded. Either is fine with me though.

I would use Loaded vs. Installed since it implies that the package is downloaded as well as loaded in one word.

I wish to clarify what it means to say Scheduled for uninstall - does it mean scheduled for unload or scheduled to be nuked? If not clear, we should probably change it to say Scheduled for deletion or Scheduled to be unloaded

Instead of skipped why don't we just use the term Unloaded?

The main concern, I guess, is about using Loaded/Unloaded vs Scheduled for uninstall.
What @aparajit-pratap is suggesting is consistent and I would say they seem fine to me

I agree with @aparajit-pratap on using Loaded, since we call the window "Installed packages". And generally I think install and uninstall sounds more like user behaviors while load and unload sounds more like Dynamo behaviors

Agree on Scheduled for unloaded or deletion. Same reason above

Not loaded or Unloaded makes sense to me

I'm also in favour of Loaded as it uses stronger language (Installed could still theoretically fail to load)

Scheduled for Uninstall/Unload/Deletion is still the one that feels most ambiguous, but I don't have a better suggestion. In lieu of that I would lean towards Scheduled for Deletion/Unload as above.

Unloaded makes most sense to me here to match the inverse of the Loaded state.

Error is perfect here - can the user see the error message and do something about it in this state @pinzart?

@Amoursol Regarding the error state. Hopefully we can display something meaningful to the user....but that work will probably be a part of this jira task https://jira.autodesk.com/browse/DYN-3670

aparajit-pratap · 2021-05-21T15:36:36Z

src/DynamoCore/Properties/Resources.Designer.cs

+
+        /// <summary>
+        ///   Looks up a localized string similar to Error.
+        ///This package has not been installed due to an unexpected error..


Does installed mean downloaded and loaded here?

aparajit-pratap · 2021-05-21T15:38:10Z

src/DynamoCore/Properties/Resources.Designer.cs

+        }
+
+        /// <summary>
+        ///   Looks up a localized string similar to Scheduled for uninstall.


Does Uninstall mean completely deleted here or just unloaded?

aparajit-pratap · 2021-05-21T15:42:55Z

src/DynamoCore/Properties/Resources.en-US.resx

+    <data name="PackageStateUnloadedTooltip" xml:space="preserve">
+    <value>Skipped.
+This package has not been loaded because another conflicting package was loaded before it.</value>
+  </data>


I would use Loaded vs. Installed since it implies that the package is downloaded as well as loaded in one word.

I wish to clarify what it means to say Scheduled for uninstall - does it mean scheduled for unload or scheduled to be nuked? If not clear, we should probably change it to say Scheduled for deletion or Scheduled to be unloaded

Instead of skipped why don't we just use the term Unloaded?

aparajit-pratap · 2021-05-21T16:01:30Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageManagerClientViewModel.cs

@@ -777,6 +778,10 @@ internal void SetPackageState(PackageDownloadHandle packageDownloadHandle, strin
            Package dynPkg;
            if (packageDownloadHandle.Extract(DynamoViewModel.Model, downloadPath, out dynPkg))
            {
+                if (DebugModes.IsEnabled("DynamoPackageStates"))
+                {
+                    dynPkg.PackageState = Package.PackageStates.Loaded;


Shouldn't we set this only after the call to LoadPackages?

hmm...I did not attempt to properly set all the states.
I only focused on the UI/UX side of it.

It may make sense to actually set the states in the PackageManagerExtension.PackageLoader ...since we may have more visibility into error states there. Or maybe some other notification system...Not sure yet

I can add a comment that this specific code is temporary...and should be moved somewhere else...And I can move it below as well

aparajit-pratap · 2021-05-21T16:09:51Z

src/DynamoCoreWpf/Views/PackageManager/InstalledPackagesView.xaml

@@ -89,12 +89,30 @@
                                                   FontSize="11"
                                                   Foreground="#CCC"></TextBlock>
                                        <TextBlock Text="{x:Static p:Resources.InstalledPackageViewPendingInstallButton}"
-                                                   Visibility="{Binding Path=Model.MarkedForUninstall, Converter={StaticResource BooleanToVisibilityCollapsedConverter}}"
+                                                   Visibility="{Binding Path=Model.EnableOldMarkedForUnistallState, Converter={StaticResource BooleanToVisibilityCollapsedConverter}}"


Little confused here, why couldn't you simply change the implementation of the MarkedForUninstall property instead of introducing a new one?

I introduced another property because MarkedForUninstall is used in other parts of Dynamo, not only used in the UI...and I did not want to break that existing functionality by introducing the DebugMode flag into the equation.
Basically, if I had done : bool MarkedForUninstall { get {return !DebugModes.IsActive() && isMarkedForUninstall; } } then, while having the DebugMode active, the other parts of the Dynamo that use MarkedForUninstall would not work anymore.

MarkedForUninstall...should probably be replaced by the PackageStates ...when the implementation is ready

pinzart90 · 2021-05-21T20:32:13Z

src/DynamoCore/Properties/Resources.en-US.resx

+  </data>
+    <data name="PackageStateErrorTooltip" xml:space="preserve">
+    <value>Unloaded.
+This package has not been loaded due to an unexpected error.</value>


Maybe we can be more specific about the errors...once we know where/how to set the package states in the Dynamo code

pinzart90 · 2021-05-21T20:33:03Z

src/DynamoCore/Properties/Resources.en-US.resx

+  </data>
+    <data name="PackageStateUnloadedTooltip" xml:space="preserve">
+    <value>Unloaded.
+This package has not been loaded because another conflicting package was loaded before it.</value>


Same as above, we may be able to specify which package caused the current one to get into an Unloaded state

pinzart90 · 2021-05-21T21:02:59Z

Latest screenshot

sm6srw

Nice, LGTM!

add package states under dbg mode

29a5b1f

pinzart90 requested review from sm6srw, mjkkirschner and aparajit-pratap May 20, 2021 12:29

pinzart90 added UI WIP labels May 20, 2021

pinzart90 requested a review from QilongTang May 20, 2021 12:29

pinzart90 commented May 20, 2021

View reviewed changes

wrap long text

a3e1947

Update Package.cs

8f95c56

pinzart90 commented May 21, 2021

View reviewed changes

pinzart added 3 commits May 21, 2021 10:52

Update Package.cs

d532ec5

update

8112df2

Update PackageManagerClientViewModel.cs

2e07b8e

pinzart90 added PTAL Please Take A Look 👀 and removed WIP labels May 21, 2021

aparajit-pratap reviewed May 21, 2021

View reviewed changes

review comments

23dbcd2

pinzart90 commented May 21, 2021

View reviewed changes

pinzart90 requested a review from aparajit-pratap May 21, 2021 20:33

Merge branch 'master' into package_states

869d3a6

sm6srw approved these changes May 24, 2021

View reviewed changes

pinzart90 merged commit f656121 into master May 24, 2021

pinzart90 deleted the package_states branch May 24, 2021 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add package states under dbg mode #11698

add package states under dbg mode #11698

pinzart90 commented May 20, 2021 •

edited

Loading

pinzart90 May 20, 2021

mjkkirschner May 20, 2021 •

edited

Loading

QilongTang May 20, 2021

mjkkirschner May 20, 2021

QilongTang May 20, 2021

mjkkirschner May 20, 2021 •

edited

Loading

sm6srw May 24, 2021

pinzart90 commented May 20, 2021 •

edited

Loading

pinzart90 May 21, 2021

mjkkirschner May 21, 2021 •

edited

Loading

aparajit-pratap May 21, 2021 •

edited

Loading

pinzart90 May 21, 2021

Jingyi-Wen May 21, 2021

Amoursol May 24, 2021

pinzart90 May 24, 2021

aparajit-pratap May 21, 2021

aparajit-pratap May 21, 2021

aparajit-pratap May 21, 2021 •

edited

Loading

aparajit-pratap May 21, 2021

pinzart90 May 21, 2021

aparajit-pratap May 21, 2021

pinzart90 May 21, 2021 •

edited

Loading

pinzart90 May 21, 2021

pinzart90 May 21, 2021

pinzart90 May 21, 2021

pinzart90 commented May 21, 2021

sm6srw left a comment

add package states under dbg mode #11698

add package states under dbg mode #11698

Conversation

pinzart90 commented May 20, 2021 • edited Loading

Purpose

Declarations

Reviewers

FYIs

Choose a reason for hiding this comment

mjkkirschner May 20, 2021 • edited Loading

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 May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinzart90 commented May 20, 2021 • edited Loading

Choose a reason for hiding this comment

mjkkirschner May 21, 2021 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap May 21, 2021 • edited Loading

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

aparajit-pratap May 21, 2021 • edited Loading

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

pinzart90 May 21, 2021 • edited Loading

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

pinzart90 commented May 21, 2021

sm6srw left a comment

Choose a reason for hiding this comment

pinzart90 commented May 20, 2021 •

edited

Loading

mjkkirschner May 20, 2021 •

edited

Loading

mjkkirschner May 20, 2021 •

edited

Loading

pinzart90 commented May 20, 2021 •

edited

Loading

mjkkirschner May 21, 2021 •

edited

Loading

aparajit-pratap May 21, 2021 •

edited

Loading

aparajit-pratap May 21, 2021 •

edited

Loading

pinzart90 May 21, 2021 •

edited

Loading