Fix error and create error handling for snapshot taking. #5520
Conversation
packages/devtools_app/lib/src/screens/memory/panes/diff/controller/diff_pane_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/memory/panes/diff/widgets/snapshot_list.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/memory/panes/diff/widgets/snapshot_list.dart
Outdated
Show resolved
Hide resolved
| Future<void> _takeSnapshot(BuildContext context) async { | ||
| try { | ||
| await controller.takeSnapshot(); | ||
| } catch (e, trace) { |
There was a problem hiding this comment.
| } catch (e, trace) { | |
| } catch (e, trace) { | |
| _log.shout(e,e,trace); |
It would be good to log this in case the user wants to send us some logs.
There was a problem hiding this comment.
The error dialog box will give all the details, copyable to clipboard.
There was a problem hiding this comment.
we also want to log this to our analytics though. @CoderDake will _log.shout propagate the error to our analytics?
| @@ -34,6 +39,51 @@ final dialogTextFieldDecoration = InputDecoration( | |||
| ), | |||
| ); | |||
|
|
|||
| /// A dialog, that reports unexpected error and allows to copy details and create issue. | |||
| class UnexpectedErrorDialog extends StatelessWidget { | |||
There was a problem hiding this comment.
Can you add a screenshot of the updated dialog to the PR comments? :-)
There was a problem hiding this comment.
It is scrollable when text is too large.
packages/devtools_app/lib/src/extension_points/extensions_external.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/extension_points/extensions_external.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/extension_points/extensions_external.dart
Outdated
Show resolved
Hide resolved
| if (issueDetails == null) | ||
| throw StateError( | ||
| 'Issue details cannot be null, because length limit is reached.', | ||
| ); |
There was a problem hiding this comment.
this error is a bit confusing 🤔 instead of throwing an error, lets add the check for issueDetails == null to the if check on line 60.
if (issueDetails == null || lengthToCut <=0) return fullUri;
There was a problem hiding this comment.
cut additionalInfo instead
packages/devtools_app/lib/src/extension_points/extensions_external.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/extension_points/extensions_external.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/extension_points/extensions_external.dart
Outdated
Show resolved
Hide resolved
| .toString(), | ||
| url: newDevToolsGitHubIssueUriLengthSafe( | ||
| additionalInfo: issueDetails, | ||
| environment: issueLinkDetails(), |
There was a problem hiding this comment.
we don't need to pass this in as a parameter. The issueLinkDetails can be looked up directly from inside the newDevToolsGitHubIssueUriLengthSafe method
There was a problem hiding this comment.
It is not very important which method makes this call.
I moved the call for testability: I do not want the test to worry about mocking the environment.
packages/devtools_app/test/extentsion_points/extension_external_test.dart
Outdated
Show resolved
Hide resolved
Fixes #5510
Screen.Recording.2023-03-24.at.3.05.25.PM.mov