Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@jonahwilliams
Copy link
Contributor

Fixes flutter/flutter#109195

The flutter tool converts all asset keys with spaces into URI encoded paths (replacing ' ' with '%20', for example). Perform the same encoding in ImmutableBuffer.fromAsset and FragmentProgram.fromAsset so that users can load assets with the same key they have written in the pubspec.

dart_main = "fixtures/ui_test.dart"
fixtures = [
"fixtures/DashInNooglerHat.jpg",
"fixtures/DashInNooglerHat%20WithSpace.jpg",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the fixtures are bundled as-is using file path as key, we need to simulate flutter tool encoding strategy by changing the file name.

@jonahwilliams jonahwilliams requested a review from zanderso August 9, 2022 16:16
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 9, 2022

  • The status or check suite Linux Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2022

// Escape `%` and `#` characters according to doc comment at
// https://github.com/ninja-build/ninja/blob/master/src/depfile_parser.cc#L28
static void EscapeString(std::string& str, std::stringstream& stream) {
Copy link
Member

Choose a reason for hiding this comment

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

CanCompileAndReflect here could have some logic added to it to emit and inspect the contents of the depfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave it a shot...

}
}

auto mapping =
Copy link
Member

Choose a reason for hiding this comment

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

Oh, my bad. I wasn't clear. The code here should output to the right file given the name of the fixture. Probably need to add a ReflectionDepfileName(). See how the other files are written out above. Also see here. Then the code in https://github.com/flutter/engine/blob/main/impeller/compiler/compiler_unittests.cc can pass a fixture with a %20 in the name, and inspect the depfile output.

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

LGTM

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 9, 2022
@auto-submit auto-submit bot merged commit e79d12a into flutter:main Aug 9, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 9, 2022
emilyabest pushed a commit to emilyabest/engine that referenced this pull request Aug 12, 2022
jonahwilliams pushed a commit to jonahwilliams/engine that referenced this pull request Aug 18, 2022
jonahwilliams pushed a commit to jonahwilliams/engine that referenced this pull request Aug 19, 2022
itsjustkevin pushed a commit that referenced this pull request Aug 22, 2022
* Automatically URI encode asset keys (#35270)

* ++
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Image.asset throws Unable to load asset when the filename contains space

2 participants