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-5816 locale splash screen #14118

Merged
merged 62 commits into from
Aug 9, 2023
Merged

DYN-5816 locale splash screen #14118

merged 62 commits into from
Aug 9, 2023

Conversation

jesusalvino
Copy link
Contributor

Purpose

Display the proper language in the beginning of the Splash Screen according to the user preference | improvement https://jira.autodesk.com/browse/DYN-5816

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

Reviewers

@QilongTang
@reddyashish

FYIs

@RobertGlobant20
@Enzo707

@jesusalvino
Copy link
Contributor Author

ss_language

@avidit avidit changed the title Dyn 5816 locale splash screen DYN-5816 locale splash screen Jun 29, 2023
@QilongTang QilongTang added this to the 2.19.0 milestone Jun 30, 2023
@@ -273,6 +275,13 @@ public static DynamoModel MakeModel(bool CLImode, string asmPath = "", string ho
/// <returns></returns>
public static DynamoModel MakeModel(bool CLImode, string asmPath = "", HostAnalyticsInfo info = new HostAnalyticsInfo())
{
IPathResolver pathResolver = CreateIPathResolver(false, string.Empty, string.Empty, string.Empty);
Copy link
Contributor

@QilongTang QilongTang Jul 14, 2023

Choose a reason for hiding this comment

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

Have you tested in DynamoRevit yet? These string.empty params later will cause certain folder property to be missing. For sandbox it is OK but for integration case, we get these from the integration code. Unless you meant to not reuse the same pathResolver later?

@mjkkirschner
Copy link
Member

mjkkirschner commented Jul 15, 2023

@QilongTang @jesusalvino I have not looked too deeply at this PR - but consider:

  • that making things static is a good way to have interesting memory leaks during testing ... it all depends of course.
  • that the tests need to mock these objects to control Dynamo and if they are all modifying the same static instance you may find strange testing behavior.
  • when you use static objects you may need to think harder about concurrency ... again it depends if the usage is obviously or not the same object.... global objects in general make multi threading harder - not that I know this applies here specifically.

@QilongTang
Copy link
Contributor

Looks like none of the smoke tests passed, so we need further work here..

@QilongTang
Copy link
Contributor

@jesusalvino What is the state of this PR? Any remaining known regressions?

@jesusalvino
Copy link
Contributor Author

@jesusalvino What is the state of this PR? Any remaining known regressions?

No regression issues, I have just cleaned a file and some unicodes issues, all clean with this PR, I think is ready.

@QilongTang
Copy link
Contributor

Thank you @jesusalvino Will wait for the PR checks

@QilongTang
Copy link
Contributor

@jesusalvino There are 20 reported regressions, please check

@jesusalvino
Copy link
Contributor Author

@jesusalvino There are 20 reported regressions, please check

They look new, I will check them for sure.

@jesusalvino
Copy link
Contributor Author

@jesusalvino There are 20 reported regressions, please check

@QilongTang I have validated them and they don't work even locally in a fresh master branch. After asking to the Team I have realized they are related to https://autodesk.slack.com/archives/C1866AFQB/p1691517645345439

@QilongTang QilongTang merged commit 542dcfb into DynamoDS:master Aug 9, 2023
18 checks passed
QilongTang pushed a commit that referenced this pull request Aug 10, 2023
* Display the proper language in the Splash Screen

* spacing RootDirectories

* Keeping spacing

* Remove Splash Screen default title

* Dissect the Preferences creation from the DynamoModel ctor

* removing null validation

* Refactoring based on the PathManager and Preferences

* Adding a PathManager and Preference singleton

* cleaning text

* PathManager and PreferenceSetttings as singleton

* Cleaning spacing and others

* Using the singleton PathManager class instead of the Interface

* Changing the scope and documenting the function CreateIPathResolver

* Changing access to be flexible for integrators

* Removing unused function

* Setting the Preferences to the RecorderTests

* Add the PreferenceSettings.Instance to the DynamoCoreTests setup

* Adding the preference settings to the DynamoTestUIBase Start configuration

* Set the Preferences to the SystemTesttBase

* Set the PreferenceSettings.Instance to the DynamoViewModelUnitTest Start

* Dealing with the PathResolver

* Passing the Preferences to the DynamoModel Start function and update a comment

* Passing the Prererences to the VisualizationTest Start

* Update the MakeCLIModel function

* Passing the Preferences to the DefaultStartConfiguration

* Update AssemblySharedInfo.cs

* Passing the settings to the configuration from the parameter

* untouch the hostApplicationDirectory

* Untouching the preLoadedLibraries field and renaming

* Restoring fields and adding doc

* Adding the preferences to the start of some tests

* Validating the settings in the DynamoModelTestBase setup

* Adding Preferences to the config Settings Test

* Refactoring the PathManager and Preference creation on the DynamoModel ctor and DynamoModelTestBase

* Cleaning Tests setup and ignore Pref Instance property for xml

* Removing Pref Singleton to the CoreTest Setup

* Rollback fron the 6213 commit

* keep the StartInTestMode to false since this is an special case and set an explicit preferences instance

* Handling Preferences

* clieaning references

* Saving them with the proper Unicode

* Removing Singletion Preferences

* Explicit Settings since the StartInTestMode is false

* Add the specific Preferences

* Rollback the cc58

* Rollback the cc58

* Fix Solution

* Fixing the Assembly character and test file

* Forcing to update the cc58 commit

* Removing the test file

* Fixing Tests Unicode

* No new line

---------

Co-authored-by: Jesus Alfredo Alviño <jesus.alfredo.alvino@autodesk.com>
Co-authored-by: reddyashish <43763136+reddyashish@users.noreply.github.com>
@@ -48,8 +48,15 @@ struct PathManagerParams
internal IPathResolver PathResolver { get; set; }
}

class PathManager : IPathManager
public class PathManager : IPathManager
Copy link
Contributor

Choose a reason for hiding this comment

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

I want to clarify that all of the public methods of this class (which were hidden before because the class was private) are now going to be exposed as public members of the class. Not sure if this was intended
FYI @mjkkirschner @QilongTang

Copy link
Contributor

@QilongTang QilongTang Aug 14, 2023

Choose a reason for hiding this comment

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

This is a good point. @jesusalvino Does PathManager need to be a public singleton or internal?

Copy link
Contributor Author

@jesusalvino jesusalvino Aug 14, 2023

Choose a reason for hiding this comment

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

@QilongTang I think keep it within the .Core namespace should be enough, I will take a look

Copy link
Member

Choose a reason for hiding this comment

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

prefer internal if possible!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guys this is the PR #14263

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.

5 participants