Skip to content

File names should only be updated if parsing the file succeeds - #5827

Merged
elliette merged 8 commits into
flutter:masterfrom
elliette:issue-5709-2
May 22, 2023
Merged

elliette merged 8 commits into
flutter:masterfrom
elliette:issue-5709-2

Conversation

@elliette

Copy link
Copy Markdown
Member

Fixes #5709
Work towards #5703

Before we were wrapping our script parsing step in an unawaited, therefore we weren't waiting for the parsing to succeed to update the file name.

This meant that:

  • if a script took a long time to parse (ie it was a big script), we would immediately show the new name but still show the contents of the old script
  • if the script parsing failed, we would still show the old file but with the new file's name

Now we wait for parsing to succeed to update the file name. If parsing fails, we show an error to the user.

@elliette
elliette requested review from a team and bkonyi as code owners May 19, 2023 18:11
/// Show the given script location (without updating the script navigation
/// history).
void _showScriptLocation(
/// history). Returns a boolean value representing success or failure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

dart doc nit: put empty line after first sentence

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done!

await tester.pumpWidget(
wrapWithControllers(
const DebuggerScreenBody(),
const NotificationsView(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

wrapWithControllers already includes a NotificationsView in the widget tree, so we shouldn't need to add this here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done!

@kenzieschmoll kenzieschmoll left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

a couple nits then lgtm

@elliette
elliette merged commit 40348fa into flutter:master May 22, 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.

Out-of-sync file names and file contents

2 participants