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

set IsFirstRun to false if hide report options is true #11530

Merged
merged 3 commits into from
Mar 8, 2021

Conversation

mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Mar 3, 2021

Purpose

when hide report options is enabled, the prompt to request user analytics is also hidden - this function has the side effect that IsFirstRun is set to false so the gallery is not shown again.

If hide report options is enabled we set IsFirstRun to false after this check, but before trying to raise a gallery window.

TODO

  • tests

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

@mjkkirschner mjkkirschner changed the title Update DynamoView.xaml.cs set IsFirstRun to false if hide report options is true Mar 3, 2021
add internal setter
@mjkkirschner mjkkirschner requested a review from QilongTang March 8, 2021 16:26
@mjkkirschner mjkkirschner added this to the 2.11.0 milestone Mar 8, 2021
@@ -914,6 +914,11 @@ private void DynamoView_Loaded(object sender, EventArgs e)
// If first run, Collect Info Prompt will appear
UsageReportingManager.Instance.CheckIsFirstRun(this, dynamoViewModel.BrandingResourceProvider);
Copy link
Contributor

@QilongTang QilongTang Mar 8, 2021

Choose a reason for hiding this comment

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

It's a bit hard to understand from first glance, to me it seems we may want to fold the dynamoViewModel.HideReportOptions check into CheckIsFirstRun() at https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/Services/UsageReportingManager.cs#L154 so we do not skip the check/setting FirstRun entirely but only skip the usage prompt

Copy link
Member Author

Choose a reason for hiding this comment

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

@QilongTang let me give that a shot.

@mjkkirschner
Copy link
Member Author

@QilongTang PTAL

@mjkkirschner mjkkirschner merged commit 42595c0 into master Mar 8, 2021
@mjkkirschner mjkkirschner deleted the mjkkirschner-patch-dyn3468 branch March 8, 2021 19:06
@QilongTang
Copy link
Contributor

@mjkkirschner I think this one will need cherry-pick right?

@mjkkirschner
Copy link
Member Author

doing it now 😉

mjkkirschner added a commit to mjkkirschner/Dynamo that referenced this pull request Mar 8, 2021
* Update DynamoView.xaml.cs

* add test
add internal setter

* review comments

Co-authored-by: michael kirschner <[email protected]>
mjkkirschner added a commit that referenced this pull request Mar 8, 2021
* Update DynamoView.xaml.cs

* add test
add internal setter

* review comments

Co-authored-by: michael kirschner <[email protected]>

Co-authored-by: michael kirschner <[email protected]>
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.

2 participants