Skip to content

Conversation

@jesusalvino
Copy link
Contributor

Purpose

Enabling the Dynamo Environment based on the IsEnabled properties of their main components instead of the Preferences Deactivated Event for the implementation of the improvement https://jira.autodesk.com/browse/DYN-5656.

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

pref_enabling


public void EnableEnvironment(bool isEnabled)
{
this.titleBarGrid.IsEnabled = isEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

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

When the pref window is opened, these are already disabled, right? why do we need this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, because it was being opened using ShowDialog(), but in this way I couldn't find how to detect when the user click outside the preferences, that's Why I'm using Show() instead so I need to disable/enable the main components of Dynamo explicitly

Copy link
Contributor

Choose a reason for hiding this comment

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

so you mean Show() will set PreferencesWindow.IsLoaded to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it does it implicitly, I check the IsLoaded property for visibility purpose because even if we close the preferences view , the field reference preferencesWindow is still alive in the DynamoView class

Copy link
Contributor

@QilongTang QilongTang May 9, 2023

Choose a reason for hiding this comment

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

it seems to me we are spending extra effort just to get the state of it using WPF API. I feel it's not worth the extra effort, how about when we call ShowDialog(), we set a state boolean under preferencesWindow so we can query the state? Otherwise, everytime we add a new grid, we need to remember to enable/disable this, seems like a pain

Copy link
Contributor Author

@jesusalvino jesusalvino May 9, 2023

Choose a reason for hiding this comment

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

Using the ShowDialog() didn't allow me to detect when the user click outside the preferences view (main goal), when we use it we don't worry about the state because it locks the Dynamo environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh you mean even MouseLeftButtonDown event cant be triggered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you mean even MouseLeftButtonDown event cant be triggered?

exactly, now I'm trying to disable only the mainGrid ( the Dynamo main container) instead of every main part of Dynamo, I will send my findings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QilongTang with my last commit 14c28d2 we won't worry about enable/disable specific Dynamo parts if we add a new grid.

Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

LGTM with one last comment

@QilongTang QilongTang merged commit 6918082 into DynamoDS:master May 9, 2023
@avidit avidit changed the title Enabling the Dynamo Environment DYN-5656 Enabling the Dynamo Environment May 10, 2023
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