Skip to content
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

Rely more on profile objects and less on GUIDs #10982

Merged
merged 3 commits into from
Aug 23, 2021

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Aug 18, 2021

Right now, we store GUIDs in panes and most of the functions for interacting
with profiles on the settings model take GUIDs and look up profiles.

This pull request changes how we store and look up profiles to prefer profile
objects. Panes store strong references to their originating profiles, which
simplifies settings lookup for CloseOnExit and the bell settings. In fact,
deleting a pane's profile no longer causes it to forget which CloseOnExit
setting applies to it. Duplicating a pane that is hosting a deleted profile
(#5047) now duplicates the profile, even though it is otherwise unreachable.

This makes the world more consistent and allows us to eventually support panes
hosting profiles that do not have GUIDs that can be looked up in the profile
list. This is a gateway to #6776 and #10669, and consolidating the profile
lookup logic will help with #10952.

PR #10588 introduced TerminalSettings::CreateWithProfile and made
...CreateWithProfileByID a thin wrapper over top it, which looked up the profile
by GUID before proceeding. It has also been removed, as its last caller is gone.

Closes #5047

This commit also moves to storing a reference to the active profile
inside Pane and propagating it out of Pane through Tab. This lets us
duplicate a pane even if its profile is missing, and gives us the
freedom in the future to return a "base" profile (;))

Related to #5047.
@DHowett DHowett force-pushed the dev/duhowett/no-profile-guid branch from 9e5fd96 to 92c43c8 Compare August 18, 2021 23:18
@DHowett DHowett marked this pull request as ready for review August 19, 2021 16:24
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Loving the progress so far

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.h Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Pane.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/TabManagement.cpp Show resolved Hide resolved
@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Aug 20, 2021
@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Aug 20, 2021
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

At first I was worried that updating the settings of a profile, then duplicating it wouldn't work with this (we'd just use the old profile object), but no, looks like the code in Pane::updateSettings will still take care of that. So :shipit:

@DHowett
Copy link
Member Author

DHowett commented Aug 23, 2021

looks like the code in Pane::updateSettings will still take care of that

please help make sure that #10997 keeps that consistency 😄

@DHowett DHowett merged commit f6f5598 into main Aug 23, 2021
@DHowett DHowett deleted the dev/duhowett/no-profile-guid branch August 23, 2021 17:11
@ghost
Copy link

ghost commented Aug 31, 2021

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicating a tab/pane without a valid profile should still duplicate the settings from that tab
3 participants