Dyn 5339 backup settings #13853
Dyn 5339 backup settings #13853
Conversation
|
|
|
Hi @jesusalvino There was one failure in tests: Dynamo.Tests.Configuration.PreferenceSettingsTests.TestImportCopySettings |
fixed. |
| /// <summary> | ||
| /// Returns path of the Backup location | ||
| /// </summary> | ||
| string BackupLocation { get; set; } |
There was a problem hiding this comment.
Can you remind me if we can add things to the interface in core? I think this is API breaking right? @mjkkirschner @pinzart
There was a problem hiding this comment.
Keeping the property in the class.
|
|
||
| if (dynamoViewModel.Model.UpdateBackupLocation(newBackupLocation)) | ||
| { | ||
| CanResetBackupLocation = !dynamoViewModel.Model.IsDefaultBackupLocation(); |
There was a problem hiding this comment.
if CanResetBackupLocation = !dynamoViewModel.Model.IsDefaultBackupLocation(); is always valid, I would put it in the getter and remove the setter for that property. Looks like you dont have to set it to a different value here
There was a problem hiding this comment.
if
CanResetBackupLocation = !dynamoViewModel.Model.IsDefaultBackupLocation();is always valid, I would put it in the getter and remove the setter for that property. Looks like you dont have to set it to a different value here
It's if true if the user select a different location than the default, is false if the user reset the location to its default value.
There was a problem hiding this comment.
Understood, my point is that it looks like you are setting this same !dynamoViewModel.Model.IsDefaultBackupLocation(); to it on different places, which implicate that it could just be its getter
There was a problem hiding this comment.
Based on the result of the UpdateBackupLocation true if it's ok / false if the path is invalid, I need to set the value of CanResetBackupLocation because it enables or not the [Reset location button].
There was a problem hiding this comment.
yeah @jesusalvino In this case, do we still need the setter? usually only a getter here would be sufficient
There was a problem hiding this comment.
yeah @jesusalvino In this case, do we still need the setter? usually only a getter here would be sufficient
yes @QilongTang , I have just tested applying the Binding Mode and UpdateSourceTrigger in the xaml side with success, the explicit setter is necessary due to is the only way to raise the property change in order to enable or not the [Reset button] in the view immediately after the user selects a Backup folder
There was a problem hiding this comment.
Thank you @jesusalvino I missed that point. In this case, feel free to revert the getter to previous change to align with setter. sorry for dragging the conversation. Looks good
There was a problem hiding this comment.
Done @QilongTang , glad to be on the same page, all the comments / inquiries always are welcome before to confirm our code
| <string>C:\Users\jesus.alvino\Documents\armadar.dyf</string> | ||
| <string>C:\Users\jesus.alvino\Documents\WatchNode-4-Dictionary.dyn</string> | ||
| <string>C:\Dynamo\test\core\WatchTree.dyn</string> | ||
| <string>C:\Users\user\Documents\dynamofile.dyn</string> |
* UI changes * Backup Location as setting and minor UI changes * Labels and PathManager engine * Dealing with invalid Bakup location path and UI titles * Add the Reset Backup Location button * Adding the Run Settings Expander * Fixing weird chars * Don't touch the Solution * Update Unit Test and how to use the String * Fixing spacing * Solving PR comments * Excluding the solution file * Refactoring * back to field value instead of logic
Purpose
Implement the improvement https://jira.autodesk.com/browse/DYN-5339
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@QilongTang
FYIs
@RobertGlobant20
@Enzo707