Skip to content

Conversation

@RobertGlobant20
Copy link
Contributor

Purpose

Added code for creating the WebView2 cache folder in the AppData location because when tries to create it inside C:\Program Files\Autodesk\Revit 2023\AddIns\DynamoForRevit is crashing due to permissions then now will be created in the AppData location.

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

Release Notes

Added code for creating the WebView2 cache folder in the AppData location

Reviewers

@QilongTang

FYIs

@mjkkirschner

Added code for create the WebView2 cache folder in the AppData location because when tries to create it inside C:\Program Files\Autodesk\Revit 2023\AddIns\DynamoForRevit is crashing due to permissions.
@RobertGlobant20
Copy link
Contributor Author

RobertGlobant20 commented Sep 6, 2022

This is a GIF showing the notifications on Dynamo for Revit but also showing the crash when closing Dynamo and notifications opened.
DynamoRevit_ShowingNotification_Crash

@RobertGlobant20
Copy link
Contributor Author

@QilongTang related to this question, the initialization in constructor was since the beginning I'm just creating a method named InitializeBrowserAsync() and call it in the constructor, do you think it should not be initialized in the constructor?
image

@QilongTang
Copy link
Contributor

@QilongTang related to this question, the initialization in constructor was since the beginning I'm just creating a method named InitializeBrowserAsync() and call it in the constructor, do you think it should not be initialized in the constructor? image

No, I did not see the previous call. I think @mjkkirschner could be right that previously everything was async so the view tests could finish before WebView2 was required

};
}
notificationUIPopup.webView.CoreWebView2InitializationCompleted += WebView_CoreWebView2InitializationCompleted;
await notificationUIPopup.webView.EnsureCoreWebView2Async();
Copy link
Contributor

@QilongTang QilongTang Sep 6, 2022

Choose a reason for hiding this comment

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

This await should not be required right? If our theory is correct, I think we can stick to the previous way of initilizing WebView2 async and apply the View closing fix as in your other PR. I will do some experiments

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 well not sure if await is required or not but I knew that we should be awaiting async calls to finish otherwise we cannot control the flow of calls (if we need to use WebView2 later), for example in this case there is a call to RequestNotifications(); method before (in the original code was after), then not sure which problems could produce in the future but as you said let's test the behavior.
I will proceed to make the changes as you are suggesting and later we can do some experiments.
Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

@RobertGlobant20 I made the change in #13295 and running tests. Because I see this as one of the big difference before/after in your previous 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.

FYI: commit: 9dcf602

Copy link
Contributor

Choose a reason for hiding this comment

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

@RobertGlobant20 For the RequestNotifications() call it should not affect WebView2 initialization, because the actual passing of information is bind to the event NavigationCompleted. So not until WebView2 was launched and navigated to the html, there would be no trial of passing info

Not using await for initializing WebView2
@QilongTang
Copy link
Contributor

@QilongTang QilongTang added this to the 2.16.0 milestone Sep 6, 2022
@QilongTang QilongTang merged commit 02e5192 into DynamoDS:master Sep 6, 2022
@QilongTang
Copy link
Contributor

Tests passed merging. @RobertGlobant20 Please file the second PR based on this which would fix the crash

@QilongTang QilongTang deleted the DYN-5245-CreateCustom-UserDataFolder branch September 6, 2022 21:04
QilongTang pushed a commit that referenced this pull request Sep 7, 2022
* DYN-5245-Revit-NotificationCenter-Crashing

Added code for create the WebView2 cache folder in the AppData location because when tries to create it inside C:\Program Files\Autodesk\Revit 2023\AddIns\DynamoForRevit is crashing due to permissions.

* DYN-5245-Revit-NotificationCenter-Crashing Code Review

Not using await for initializing WebView2
@QilongTang QilongTang mentioned this pull request Sep 7, 2022
8 tasks
QilongTang added a commit that referenced this pull request Sep 7, 2022
* DYN-5245-DynamoRevit-CreateCustom UserDataFolder (#13293)

* DYN-5245-Revit-NotificationCenter-Crashing

Added code for create the WebView2 cache folder in the AppData location because when tries to create it inside C:\Program Files\Autodesk\Revit 2023\AddIns\DynamoForRevit is crashing due to permissions.

* DYN-5245-Revit-NotificationCenter-Crashing Code Review

Not using await for initializing WebView2

* DYN-5245-DynamoRevit-Crash-WhenExiting (#13299)

* DYN-5245-Revit-NotificationCenter-Crashing

I added some code for stopping the WebView2 instance when Dynamo is being closed because otherwise is crashing and also not loading the notifications.
Also the Dispose method was deleted due that we are doing the same in the new method added SuspendCoreWebviewAsync.

* DYN-5245-Revit-NotificationCenter-Crashing

Removing call to TrySuspendAsync();

Co-authored-by: Roberto T <61755417+RobertGlobant20@users.noreply.github.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.

2 participants