Skip to content

Add a WT_SETTINGS_DIR env variable that portable profiles can use#16949

Merged
DHowett merged 3 commits intomainfrom
dev/migrie/b/16295-portable-env-var
Mar 28, 2024
Merged

Add a WT_SETTINGS_DIR env variable that portable profiles can use#16949
DHowett merged 3 commits intomainfrom
dev/migrie/b/16295-portable-env-var

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Mar 27, 2024

Basically, title.

It'd be a neat idea for portable installs of the Terminal to reference files that are right there in the portable install.

This PR adds a WT_SETTINGS_DIR var to Terminal's own env block. This allows us to resolve profiles relative to our own settings folder.

Closes #16295

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. labels Mar 27, 2024
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I love it. I've also got a couple thoughts...

If we want to include the root path for WT, can it be WT_ROOT instead?

Alternatively... should it actually be a path to the settings root? That would let an entire settings folder -- whether it is in LocalAppData, the Package storage, or in a portable install -- roam independent of its EXE path. I ask mainly because the package root is immutable and useless to most people (for example).

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 27, 2024
@zadjii-msft
Copy link
Member Author

Alternatively... should it actually be a path to the settings root? That would let an entire settings folder -- whether it is in LocalAppData, the Package storage, or in a portable install

This is a better idea.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Mar 27, 2024
@zadjii-msft zadjii-msft self-assigned this Mar 27, 2024
@zadjii-msft
Copy link
Member Author

image

@zadjii-msft zadjii-msft assigned DHowett and unassigned zadjii-msft Mar 28, 2024
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Still wondering if it should be WT_SETTINGS_ROOT (or DIR) since it doesn’t point at the json file itself! Also make sure you update your PR title/body!

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 28, 2024
@lhecker
Copy link
Member

lhecker commented Mar 28, 2024

I believe _DIR is better than _ROOT because dirname exists but rootname doesn't.

@zadjii-msft zadjii-msft changed the title Add a WT_FOLDER_PATH env variable that portable profiles can use Add a WT_SETTINGS_DIR env variable that portable profiles can use Mar 28, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 28, 2024
@DHowett DHowett enabled auto-merge March 28, 2024 14:03
@DHowett DHowett requested a review from lhecker March 28, 2024 19:57
@DHowett DHowett added this pull request to the merge queue Mar 28, 2024
// Do this here, rather than at the top of main. This will prevent us from
// including this variable in the vars we serialize in the
// Remoting::CommandlineArgs up in HandleCommandlineArgs.
_setupFolderPathEnvVar();
Copy link
Member

Choose a reason for hiding this comment

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

FWIW an alternative, but more robust approach may be to have our own ExpandEnvironmentStringsW. It would also work better together with virtual env vars and the like. We already have such a function in a way in til::env already too.

Merged via the queue into main with commit 36c81f2 Mar 28, 2024
@DHowett DHowett deleted the dev/migrie/b/16295-portable-env-var branch March 28, 2024 20:44
DHowett pushed a commit that referenced this pull request Apr 22, 2024
…6949)

Basically, title.

It'd be a neat idea for portable installs of the Terminal to reference
files that are right there in the portable install.

This PR adds a `WT_SETTINGS_DIR` var to Terminal's own env block. This
allows us to resolve profiles relative to our own settings folder.

Closes #16295

(cherry picked from commit 36c81f2)
Service-Card-Id: 92352620
Service-Version: 1.20
DHowett added a commit that referenced this pull request Aug 5, 2025
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 profile
- `backgroundImage` on profile (appearance)
- `pixelShaderPath` and `pixelShaderImagePath` on profile (appearance)
- `icon` on command and the new tab menu entries

The 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 final
normalized 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 read[^2]. 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 wing
it (#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 same
reason. Neither was intended, and of the two the second _should_ have
worked but never could[^1].

For both `http(s?)` URLs and `ms-appx://` URLs which specify a package
name, we now strip everything except the filename. As an example...

If my fragment specifies `https://example.net/assets/foo.ico`, and my
fragment was loaded from `C:\Fragments`, Terminal will look *only* at
`C:\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
emoji 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 to
the 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 your `defaults` profile, it
will 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, even `pixelShaderPath`... but also,
`pixelShaderImagePath`!) It also expands support for _environment
variables_ 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 into `file://`
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 to
the 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.

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)

[^1]: Handling a `ms-appx` path requires us to _add their package to our
dependency 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!)

[^2]: We don't bother tracking where the defaults came from, because we
control everything about them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Settings Issues related to settings and customizability, for console or terminal Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Attention The core contributors need to come back around and look at this ASAP. Product-Terminal The new Windows Terminal.

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

Set environment variable WT_FOLDER_PATH to the folder path that contains WindowsTerminal.exe

3 participants