-
Notifications
You must be signed in to change notification settings - Fork 8.7k
Rewrite media resource handling (relative path icons, web URLs) #19143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5c0ab16
to
99f3c8c
Compare
This comment has been minimized.
This comment has been minimized.
99f3c8c
to
cb2daea
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding all the comments to MediaResourceSupport.h. Helps a ton 😊
Also a big fan of Utils::IsLikelyToBeEmojiOrSymbolIcon()
@@ -132,8 +132,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |||
|
|||
bool Inlining() const; | |||
void Inlining(bool value); | |||
|
|||
hstring Icon() const { return _FolderEntry.Icon().Path(); } | |||
void Icon(const hstring&) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, could you just add the implementation now? The implementation existed before with the GETSET_OBSERVABLE_PROJECTED_SETTING
macro. Even though we're not using it, it feels weird to have an empty setter. When somebody comes by and tries to add support for icons later, it'll silently fail because the setter isn't implemented. I think it's worth getting ahead of that and just implementing it here.
src/types/inc/utils.hpp
Outdated
// and many non-ZWJ emoji) | ||
// - It is 1-5 code units long and ends in a Unicode Variation Selector (U+FE00 to U+FE0F) | ||
// (https://www.unicode.org/reports/tr51/#Emoji_Variation_Sequences) | ||
// - It contains U+200D Zero Width Joiner within the first 8 code units (which suggests that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this (
doesn't have a corresponding )
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
tried to doc it: MicrosoftDocs/terminal#875 |
There were instances where changing the icon or resetting it did not result in the text box changing. Regressed in #19143
This pull request broadly rewrites how we handle all media resources in
the Terminal settings model.
What is a media resource?
A media resource is any JSON property that refers to a file on disk,
including:
icon
on profilebackgroundImage
on profile (appearance)pixelShaderPath
andpixelShaderImagePath
on profile (appearance)icon
on command and the new tab menu entriesThe last two bullet points were newly discovered during the course of
this work.
Description of Changes
In every place the settings model used to store a string for a media
path, it now stores an
IMediaResource
.A media resource must be resolved before it's used. When resolved, it
can report whether it is
Ok
(found, valid) and what the finalnormalized path was.
This allows the settings model to apply some new behaviors.
One of those new behaviors is resolving media paths relative to the
JSON file that referred to them. This means fragments and user settings
can now contain local images, pixel shaders and more and refer to them
by filename.
Relative path support requires us to track the path from which every
media resource "container" was read1. For "big" objects like Profile,
we track it directly in the object and for each layer. This means that
fragments updating a profile pass their relative base path into the
mix. For some of the entries such as those in
newTabMenu
, we just wingit (#19191). For everything that is recursively owned by a parent that
has a path (say each Command inside an ActionMap), we pass it in from
the parent during media resolution.
During resolution, we now track exactly which layer an icon,
background image, or pixel shader path came from and read the "base
path" from only that layer. The base path is not inherited.
Another new behavior is in the handling of web and other URLs.
Canonical and a few other WSL distributors had to resort to web URLs for
icons because we did not support loading them from the package. Julia
tried to use
ms-appx://JuliaPackageNameHere/path/to/icon
for the samereason. Neither was intended, and of the two the second should have
worked but never could2.
For both
http(s?)
URLs andms-appx://
URLs which specify a packagename, we now strip everything except the filename. As an example...
If my fragment specifies
https://example.net/assets/foo.ico
, and myfragment was loaded from
C:\Fragments
, Terminal will look only atC:\Fragments\foo.ico
.This works today for Julia (they put their icon in the fragment folder
hoping that one day we would support this.) It will require some work
from existing WSL distributors.
I'm told that this is similar to how XML schema documents work.
Now, icons are special. They support Emoji and Segoe Icons. This PR
adds an early pass to avoid resolving anything that looks like an
emoji.
This PR intentionally expands the heuristic definition of an emoji. It
used to only cover 1-2 code unit emoji, which prevented the use of any
empji more complicated than "man in business suite levitating."
An icon path will now be considered an emoji or symbol icon if it is
composed of a single grapheme cluster (as measured by ICU.)
This is not perfect, as it errs on the side of allowing too many
things... but each of those things is technically a single grapheme
cluster and is a perfectly legal FontIcon ;)
Profile icons are even more special than icons. They have an
additional fallback behavior which we had to preserve. When a profile
icon fails validation, or is expressly set to
null
, we fall back tothe EXE specified in the command line.
Because we do this fallback during resolution, and the icon may be
inherited by any higher profile, we can only resolve it against the
commandline at the same level as the failed or nulled icon.
Therefore, if you specify
icon: null
in yourdefaults
profile, itwill only ever resolve to
cmd.exe
for any profile that inherits it(unless you change
defaults.commandline
).This change expands support for the magic keywords
desktopWallpaper
and
none
to all media paths (yes, evenpixelShaderPath
... but also,pixelShaderImagePath
!) It also expands support for environmentvariables to all of those places. Yes, we had like forty different
handlers for different types of string path. They are now uniform.
Resource Validation
Media resources which are not found are "rejected". If a rejected
resource lives in user settings, we will generate a warning and
display it.
In the future, we could detect this in the Settings UI and display a
warning inline.
Surprises
I learned that
Windows.Foundation.Uri
parses file paths intofile://
URIs, but does not offer you a way to get the original file path back
out. If you pass
C:\hello world
,Uri.Path
will return/C:/hello%20world
. I kid you not.As a workaround, we bail out of URL handling if the
:
is too close tothe start (indicating an absolute file path).
Testing
I added a narow test hook in the media resource resolver, which is
removed completely by link-time code generation. It is a real joy.
The test cases are all new and hopefully comprehensive.
TODO
Closes #19075
Closes #16295
Closes #10359 (except it doesn't support fonts)
Supersedes #16949 somewhat (
WT_SETTINGS_DIR
)Refs #18679
Refs #19215 (future work)
Refs #19201 (future work)
Refs #19191 (future work)
Footnotes
We don't bother tracking where the defaults came from, because we
control everything about them. ↩
Handling a
ms-appx
path requires us to add their package to ourdependency graph for the entire duration during which the resource will
be used. For us, that could be any time (like opening the command
palette for the first time!) ↩