Skip to content
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-6857: Repurposing homepage preferences changes(#15139) #15215

Merged
merged 4 commits into from
May 15, 2024

Conversation

reddyashish
Copy link
Contributor

@reddyashish reddyashish commented May 14, 2024

Purpose

JIRA: https://jira.autodesk.com/browse/DYN-6857

This is to get the changes from a previous PR(#15139) that was reverted as the build was failing with regressions.

Will be fixing the underlying issues in this new PR.

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
  • This PR contains no files larger than 50 MB

Release Notes

homepage preferences (#15139)

Reviewers

@dnenov @QilongTang

FYIs

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

- added a single serialized preferences storage for the HomePage

(cherry picked from commit 534f6cf)
@github-actions github-actions bot changed the title Repurposing homepage preferences changes(#15139) DYN-6857: Repurposing homepage preferences changes(#15139) May 14, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-6857

Copy link

github-actions bot commented May 14, 2024

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

- added the new serialized property to the preferences DynamoSettings.xml
@reddyashish
Copy link
Contributor Author

Thanks Deyan, TestImportCopySettings is now passing. The other failing test ContainsEmptyListOrNullTest is flaky. Waiting for the final serlf-serve to pass.

@QilongTang QilongTang added this to the 3.2 milestone May 15, 2024
@QilongTang
Copy link
Contributor

@reddyashish Please address the merge conflict

@reddyashish
Copy link
Contributor Author

Resolved the conflicts. The last test job has passed. So will merge this after the basic tests are done.
@QilongTang can you approve this after taking a look?

@@ -32,6 +32,7 @@
<WindowW>1936</WindowW>
<WindowH>1056</WindowH>
<UseHardwareAcceleration>false</UseHardwareAcceleration>
<HomePageSettingsSerialized>{"recentPageViewMode":"grid","samplesViewMode":"list"}</HomePageSettingsSerialized>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnenov Why do we serialize the setting as a json block instead of xml list? This is slightly different than other Dynamo settings

Copy link
Collaborator

@dnenov dnenov May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @QilongTang - I am glad you cast a glance at this! What I am trying to achieve is minimize the amount of separate setting variables we introduce for the HomePage while future proofing the process of introducing new settings. With that idea, I wanted to use a Dictionary<string, obj> (name of the property, value of the property) - as we don't know what type of value a certain setting might have. This type of dictionary is not directly serializable though, so I used a slight workaround to go through .json serialized string.

It's the object part that did not allow me to serialize to xaml directly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hindsight, a Dictionary<string, string> might have worked just fine, at least for the time being.

Let me know what you think, I am more than happy to rework that bit of code to make things less complicated if possible!

Copy link
Contributor

@QilongTang QilongTang May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am leaning towards a Dictionary<string, string> instead so we can serialize and stick to XML. In terms of the specific setting, if casting to bool or double is required in the future, we can do that. I do appreciate your thinking on the future proof though, but we also have customers manually create settings template for the whole company so I would like to prioritize consistency. Let me know if you would like to put it as another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Aaron. Discussed this with Deyan and he will make that change in a new PR. Will go ahead and merge this.

@reddyashish reddyashish merged commit 6aa533d into DynamoDS:master May 15, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants