Skip to content

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
if (!dynamoViewModel.HideReportOptions)
{
// 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 <michael.kirschner@autodesk.com>
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 <michael.kirschner@autodesk.com>

Co-authored-by: michael kirschner <michael.kirschner@autodesk.com>
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