-
Notifications
You must be signed in to change notification settings - Fork 635
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
DYN-4964-WorkingRange-Popup #13656
DYN-4964-WorkingRange-Popup #13656
Conversation
I've added a new button in the Workspace that when is clicked will show a popup containing the working ranges and the one currently selected. This new button will show a different image when the mouse is hover and when is clicked, also I've added a tooltip. For implementing this functionality I've added a new Popup (GeometryScalingPopup.xaml) and it's corresponding ViewModel. Finally I've added a converter that will receive as a parameter the current Working Range and will return a Brush with a specific color so the checkmark will be visible or not.
hi @RobertGlobant20 My understanding is that the geometry scaling setting in preferences is more like the default setting for new workspace, it is not meant to be in sync with the current setting in homeworkspace. Did I missing anything? FYI: @Jingyi-Wen |
@QilongTang then by reading again the AC in the task the GeometryScaling Popup should be read-only, right? |
I don't think this is correct. Users need to be able to set the workspace scaling for a workspace, not just read it. Are these actually two different preferences? |
Yes, they are. The current UX before this change is just confusing. I believe the current geometry scaling in the preference panel is acting as the current workspace scaling setting reflection, but almost all of the other settings in preferences are Dynamo-specific instead of workspace specific. That's why we started this task of enabling a new UI to distinguish both. |
Hi @QilongTang I'm a bit confused why these have to be 2 separate preferences. Could you please elaborate more? |
See my comments above. I would think of this problem as similar to the group styles user experience. There are defaults styles that would apply only when users choose to, otherwise they are purely settings that does not affect current workspace. There are in-workspace styles that apply to the current workspace only, once the current workspace is saved and closed, they are gone and not necessarily related to the next new workspace. |
I see. Then I think we can rename the one on canvas to "Workspace Geometry Scaling" and the Preferences Panel one "Default Geometry Scaling". Behavior-wise, changing the workspace one won't sync with the Preferences. |
@Jingyi-Wen I agree. @RobertGlobant20 Let me know if you feel the direction is proper, I think after the renaming, we will need to introduce the |
@QilongTang then in my understanding the new setting (Property) that will be added in Preferences ( |
HI @RobertGlobant20 and the dynViewModel.PreferencesViewModel.ScaleSize property will be set in the new popup shown in Workspace (both properties won't be sync but by default when a new workspace is created it will have the default Geometry Scaling value selected) from preference panel If I remember correctly PreferencesViewModel.ScaleSize is being serialized in the dyn file as ScaleFactor, what about the new added PreferencesViewModel.DefaultGeometryScaling, will be serialized? Thanks |
@QilongTang Just for start the discussion about it: when checking the code for adding the DefaultGeometryScaling property and making the code in the Preferences to use this new property I realized that there are more edge cases that I didn't consider, for example I can see that the RadioButtons are not directly binded to the PreferencesViewModel.ScaleSize (when the radiobutton is changed the PreferencesView.scaleValue is updated and later DynamoViewModel.ScaleFactorLog is updated with scaleValue when the Preferences is closed ) and also that after changing the Geometry Scale value and closing the Preferences panel the graph is run and is serialized until the Save button in Dynamo is pressed. Just to be in the same page I've created a list of requirements, let me know if all of them makes sense to you.
|
|
Added functionality for the new DefaultGeometryScaling property that will be serialized in the DynamoSettings.xml. Also I've disconnected the functionality of selecting the Geometry Scale for the current Workspace in the Preferences panel, now will be selected from the Dynamo workspace and will be serialized in the dyn file (as currently is happening).
@QilongTang I've done several changes for achieving the previous requirements, please review the commit: a2a3a1c |
Shall we rename the one on canvas to "Workspace Geometry Scaling" and the Preferences Panel one "Default Geometry Scaling"? |
Updated the string shown in Preferences panel (Geometry Scaling section) and the tooltip showed when the mouse is over the new Workspace button.
Fixed several comments also several methods were removed (like RadioGeometryScaling_Checked method) or moved The property GeoScalingViewModel was moved from DynamoViewModel to WorkspaceViewModel.. The property CurrentGeometryScaling was deleted due that was duplicating a functionality.
@RobertGlobant20 is the graph re-executing once you change the workspace geometry scale setting? I would expect it to but from this gif, it doesn't look like it is. At least the message near the |
Just so I understand correctly, the purpose of the default geometry scale setting is that when creating a new workspace, this value can be assigned to the workspace-specific setting, to begin with, and from that point onwards, whatever setting that's picked for the workspace setting, it will override the default setting for that workspace, is that correct? |
When changing the Workspace Geometry Scaling it was not running the graph so I did some changes so it will be running the graph every time is updated.
@RobertGlobant20 is the graph re-executing once you change the workspace geometry scale setting? I would expect it to but from this gif, it doesn't look like it is. At least the message near the @aparajit-pratap I just noticed that problem, I've applied a fix for that. Here is a GIF updated. |
@aparajit-pratap The Default Geometry Scale value (a property in PreferencesViewModel) will be just a reference as you said that when we create a new Workspace this value will be assigned to Workspace Geometry Scale (a property in GeometryScalingViewModel), then when we set the Workspace Geometry Scale the graph is automatically re-run and if we save the graph we will be saving in the dyn file the Workspace Geometry Scale value (ScaleFactor in the dyn file).
|
/// This event is generated every time the user clicks a Radio Button in the Geometry Scaling section | ||
/// The method just get the Radio Button clicked and saves the ScaleValue selected | ||
/// This are the values used for the scales: | ||
/// - 2 - Small |
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.
Well I think this is still 1 - Small
right?
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.
Well, this comment is trying to give information to developer related to which value could contain the scaleFactor variable, if you see to the ConvertUIToScaleFactor() method is using a formula so that if we pass the 0 index from UI (the first button - small) then the formula returns -2 and that's the value stored in ScaleValue.
This is a table of the values, I hope this make it more clear.
var t = new DelegateBasedAsyncTask(model.Scheduler, () => model.ResetEngine()); | ||
model.Scheduler.ScheduleForExecution(t); | ||
|
||
ShowStartPage = false; // Hide start page if there's one. | ||
|
||
var defaultWorkspace = Workspaces.FirstOrDefault(); |
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.
Could this be custom node workspace if user opened dyf first? I thought there are pointer already
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.
this code was also added in the part in which the user creates a new custom node, check the next commit:
c209794
@@ -2563,6 +2575,11 @@ internal bool ClearHomeWorkspaceInternal() | |||
|
|||
model.ClearCurrentWorkspace(); | |||
|
|||
var defaultWorkspace = Workspaces.FirstOrDefault(); | |||
//Every time that a new workspace is created we have to assign the Defautl Geometry Scaling value defined in Preferences (comming from DynamoSettings.xml) |
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.
It seems we can remove (comming from DynamoSettings.xml)
? If user, just modified the setting, then the setting is not serialized yet, as long as we mentioned Defautl Geometry Scaling value defined in Preferences
, it should be fine
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.
Fixed comment in the next commit:
c209794
GeoScalingPopup = new GeometryScalingPopup(ViewModel.DynamoViewModel); | ||
|
||
GeoScalingPopup.Placement = PlacementMode.Bottom; | ||
GeoScalingPopup.PlacementTarget = geometryScalingButton; |
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.
Do we need to set the placement everytime or can we set only once at initialization?
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've moved this piece of code inside the if statemente below so it will be executed one time, check the next commit:
c209794
{ | ||
var selectedButton = sender as Button; | ||
if (selectedButton == null) return; | ||
//var buttons = BaseGrid.Children.OfType<Button>(); |
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.
if not needed, let's delete 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.
deleted in the next commit: c209794
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.
LGTM with some comments, any chance you can add unit test for the new setting serialization/deserialization?
Add functionality for when a custom node is created the Workspace Scale Factor is set. Updating and removing some comments and also I started to add the unit test.
Updating Unit Test
Added a unit test for checking serialization/deserialization, please check the next commit: bcf9a45 |
I did the next fixes: The test TestImportCopySettings() was failing due that was reading the DynamoSettings-NewSettings.xml and comparing against the properties in PreferencesSetting so the DefaultScaleFactor was missing in the DynamoSettings-NewSettings.xml file. The test PreferencesGeoScaling_RunGraph_Automatic due that was opening the Preferences panel and changing the Geometry Scaling value for the workspace but now that this value was moved to the Workspace in a Popup then the code needed some changes so we can change the Geometry Scaling value using the Popup.
@RobertGlobant20 Can you take a look at the conflicts? |
@zeusongit conflicts fixed |
* DYN-4964-WorkingRange-Popup I've added a new button in the Workspace that when is clicked will show a popup containing the working ranges and the one currently selected. This new button will show a different image when the mouse is hover and when is clicked, also I've added a tooltip. For implementing this functionality I've added a new Popup (GeometryScalingPopup.xaml) and it's corresponding ViewModel. Finally I've added a converter that will receive as a parameter the current Working Range and will return a Brush with a specific color so the checkmark will be visible or not. * DYN-4964-WorkingRange-Popup CodeReview1 Added functionality for the new DefaultGeometryScaling property that will be serialized in the DynamoSettings.xml. Also I've disconnected the functionality of selecting the Geometry Scale for the current Workspace in the Preferences panel, now will be selected from the Dynamo workspace and will be serialized in the dyn file (as currently is happening). * DYN-4964-WorkingRange-Popup CodeReview1 Updated the string shown in Preferences panel (Geometry Scaling section) and the tooltip showed when the mouse is over the new Workspace button. * Build Fix When merging master to my branch there were some changed that I didn't noticed in the PR so I'm reverting back those changes. * DYN-4964-WorkingRange-Popup CodeReview 2 Fixed several comments also several methods were removed (like RadioGeometryScaling_Checked method) or moved The property GeoScalingViewModel was moved from DynamoViewModel to WorkspaceViewModel.. The property CurrentGeometryScaling was deleted due that was duplicating a functionality. * DYN-4964-WorkingRange-Popup CodeReview 2 When changing the Workspace Geometry Scaling it was not running the graph so I did some changes so it will be running the graph every time is updated. * DYN-4964-WorkingRange-Popup CodeReview 2 Add functionality for when a custom node is created the Workspace Scale Factor is set. Updating and removing some comments and also I started to add the unit test. * DYN-4964-WorkingRange-Popup CodeReview2 Updating Unit Test * DYN-4964-WorkingRange-Popup Fixing Tests I did the next fixes: The test TestImportCopySettings() was failing due that was reading the DynamoSettings-NewSettings.xml and comparing against the properties in PreferencesSetting so the DefaultScaleFactor was missing in the DynamoSettings-NewSettings.xml file. The test PreferencesGeoScaling_RunGraph_Automatic due that was opening the Preferences panel and changing the Geometry Scaling value for the workspace but now that this value was moved to the Workspace in a Popup then the code needed some changes so we can change the Geometry Scaling value using the Popup.
This reverts commit a526390.
* Dyn 5347 unified UI information icons (#13706) * standardizing information icons in the preference settings * Defending code (#13696) * Defending code * refining code * Improve Package Manager search sort order (#13700) * add search sort order * Add option to preview or hide preview of all geometry in a group and Fix top menu pixel difference (#13702) * Adjust border to fix 1 pixel anomaly * remove unused login grid * add logic to toggle preview for group * add analytics * Preview > Preview Geometry * ensure that signout option no seen when user sign out * DYN-4964-WorkingRange-Popup (#13656) * DYN-4964-WorkingRange-Popup I've added a new button in the Workspace that when is clicked will show a popup containing the working ranges and the one currently selected. This new button will show a different image when the mouse is hover and when is clicked, also I've added a tooltip. For implementing this functionality I've added a new Popup (GeometryScalingPopup.xaml) and it's corresponding ViewModel. Finally I've added a converter that will receive as a parameter the current Working Range and will return a Brush with a specific color so the checkmark will be visible or not. * DYN-4964-WorkingRange-Popup CodeReview1 Added functionality for the new DefaultGeometryScaling property that will be serialized in the DynamoSettings.xml. Also I've disconnected the functionality of selecting the Geometry Scale for the current Workspace in the Preferences panel, now will be selected from the Dynamo workspace and will be serialized in the dyn file (as currently is happening). * DYN-4964-WorkingRange-Popup CodeReview1 Updated the string shown in Preferences panel (Geometry Scaling section) and the tooltip showed when the mouse is over the new Workspace button. * Build Fix When merging master to my branch there were some changed that I didn't noticed in the PR so I'm reverting back those changes. * DYN-4964-WorkingRange-Popup CodeReview 2 Fixed several comments also several methods were removed (like RadioGeometryScaling_Checked method) or moved The property GeoScalingViewModel was moved from DynamoViewModel to WorkspaceViewModel.. The property CurrentGeometryScaling was deleted due that was duplicating a functionality. * DYN-4964-WorkingRange-Popup CodeReview 2 When changing the Workspace Geometry Scaling it was not running the graph so I did some changes so it will be running the graph every time is updated. * DYN-4964-WorkingRange-Popup CodeReview 2 Add functionality for when a custom node is created the Workspace Scale Factor is set. Updating and removing some comments and also I started to add the unit test. * DYN-4964-WorkingRange-Popup CodeReview2 Updating Unit Test * DYN-4964-WorkingRange-Popup Fixing Tests I did the next fixes: The test TestImportCopySettings() was failing due that was reading the DynamoSettings-NewSettings.xml and comparing against the properties in PreferencesSetting so the DefaultScaleFactor was missing in the DynamoSettings-NewSettings.xml file. The test PreferencesGeoScaling_RunGraph_Automatic due that was opening the Preferences panel and changing the Geometry Scaling value for the workspace but now that this value was moved to the Workspace in a Popup then the code needed some changes so we can change the Geometry Scaling value using the Popup. * Dyn 4964 popup geo scaling bugs (#13717) * DYN-4964-PopupGeoScaling-Bugs Fixing the next bugs reported by Aabi: - The Popup was not closing when clicking in the Dynamo workspace (adding a similar fix like the Workspace ContextMenu). - Adding a shadow over the new Geometry Scaling popup icon. * DYN-4964-WorkingRange-Popup Bug fixes There was a missing case that when the user click a Dynamo menu the Popup was not closing then I added some extra code to close it. * DYN-4964-WorkingRange-Popup Bug Code Review1 Removing extra trailing spaces. * Do not force user login for package search operation (#13725) * restrict user login for pkg search * check login state * Revert "Dyn 4964 popup geo scaling bugs (#13717)" This reverts commit 35005c3. * Revert "DYN-4964-WorkingRange-Popup (#13656)" This reverts commit a526390. * Markdown procedural image location (#13693) * Markdown procedural image location - fixed hard-coded image location - now identifies image location * Fixed Failing Tests - caught a bug causing issues when no image is present * Revert "Markdown procedural image location (#13693)" This reverts commit 299bbe9. * Address review comments for 2.17.1 cherrypick PR (#13729) --------- Co-authored-by: filipeotero <[email protected]> Co-authored-by: jesusalvino <[email protected]>
Purpose
Adding extra functionality for Working Range in Dynamo workspace
I've added a new button in the Workspace that when is clicked will show a popup containing the working ranges and the one currently selected. This new button will show a different image when the mouse is hover and when is clicked, also I've added a tooltip. For implementing this functionality I've added a new Popup (GeometryScalingPopup.xaml) and it's corresponding ViewModel. Finally I've added a converter that will receive as a parameter the current Working Range and will return a Brush with a specific color so the checkmark will be visible or not.
Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
Adding extra functionality for Working Range in Dynamo workspace
Reviewers
@QilongTang
FYIs
@Jingyi-Wen