-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Show a preview of the control in the SUI #9527
Conversation
Other random thought - should the preview be above the scroll viewer, so the preview is always visible as you scroll through the list of settings 🤔? @cinnamon-msft for design help here |
…/pabhoj/sui_control_preview
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.
Great work! Concerned about a few scenarios that I want to make sure we handle:
- Resizing
- Please resize the window and make sure it feels natural. We'll want some padding on the left and right side so that the TermControl is always visible.
- If possible, reflow would be awesome. But I bet that's hard. I'm ok with that being a follow-up, if we even want to pursue it.
- Make sure to test the smallest possible size of the window.
- Interactivity
- Can you select text in the TermControl? Originally, I thought we should. But we wouldn't be able to copy/paste, right? So maybe we should disable selection, and if people are interested, throw in selection/copy/paste via the mouse later?
- Misc settings
- Do the following profile settings do anything?
- Retro terminal effects
- Cursor shape & height
- Acrylic and acrylic opacity
- scrollbar visibility
- Do the following profile settings do anything?
- Accessibility
- Can you tab in/out of the TermControl?
- What does the screen reader say when you are focused on the TermControl? (I can probably check this one after the PR is out of draft)
- Preview Connection
- Probably better for a future consideration, but it would be cool if we could have the header for the shell pop up. Like, CMD would do the "Microsoft Windows [Version..." and Pwsh would do "PowerShell 7.1.3 Copyright". But we'd need a way to figure out what the output is and that just sounds messy.
- I'd also like it if we could get powerline in there, since that's really colorful. But again, that doesn't sound possible tbh.
- Honestly, I'm tempted to say that we should just open a real terminal connection in there? It might be overkill, but it would allow users to display whatever they want. That said, I think I'm ok with some simple output like this for now, then doing a more complex one in a follow-up task.
void PreviewConnection::Resize(uint32_t /*rows*/, uint32_t /*columns*/) noexcept | ||
{ | ||
} |
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.
Be sure to test what happens when you resize the window and the TermControl doesn't fit (or resizes). Idk how difficult it would be to add reflow, but I'm guessing we need something here?
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.
Reflow will be impossible. On resize, the connection should simply clear the terminal and re-print the entire preview text.
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.
The reason for this is that resize is a destructive operation when content is pushed off the top of the screen.
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.
This method still needs to be implemented for that right?
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.
This method still needs to be implemented for that right?
Ah it actually doesn't! The connection doesn't need to do anything
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.
Wait, no.. on resize, the connection must re-print the entire preview text. Resize with reflow destroys the content that goes off the top of the screen, and the connection needs to clear the screen and put it back. Now, since we never resize the control this isn't going to be a problem... but do keep it in mind.
<Border x:Name="ControlPreview" | ||
Height="200" | ||
Width="600"/> |
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.
A few things here:
- add a comment saying that this is where the terminal preview goes
- What happens if you resize the window? Maybe you'll want to set the MaxWidth instead?
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.
If only we had a normal constructor for the control, we could use <tc:TermControl Height="200"...
ABSOLUTELY NOT. Imagine I make a profile that runs...
We don't want to spawn TWO NEW PROCESSES for every preview update, either. The connection needs to be representative of what people use Terminal for. Powerline is not guaranteed (in the font); a shell is, emoji may be, etc. I'm imagining that we have emoji in there, and some characters that might ligature together, and some colors. Not a lot of them (check the color scheme page for that). The emoji will change when you toggle colored emoji, etc. |
Yeah I think this makes sense. I think it'll look okay once we change the Pivot to the horizontal NavView. |
You may experience weirdness nesting a NavigationView inside another NavigationView |
They all work except acrylic! |
I'm a bit conflicted on this. Definitely not blocking the PR for this, but we should probably file a follow-up to be able to provide a preview of the acrylic (if that's even possible). But if it's not too much work for you to do it now, that'd be cool too ;) haha. |
This comment has been minimized.
This comment has been minimized.
A few updates:
|
The preview term control is not focusable and cannot be tabbed in/out of! Also the height of the preview has been reduced by half so that the scroller below the pivot has more space. (can't post a screenshot because of secrets) |
Notes from Demo w/ @cinnamon-msft
Content inside the preview
I think it'll look nice and kinda match the initial output of PowerShell/CMD.
Miscellaneous notes
|
This is incompatible with keeping it visible while you scroll, which you loved.
What about Advanced > Text Antialiasing, Advanced > Bell Notification Style, General > Tab Title, General > Tab Color (when we have tab preview), General > Icon?
I think this will look bad.
The initial output of PowerShell/CMD is atrocious, and I see no reason for us to make that mistake again. We've been showing people how to customize We need to display, at the minimum: Colors. Emoji (because we will have a [disable emoji] [(x) use color emoji] switch, per user request). Italic text. Bold text (when we have bold.) All of those are not represented in the shell preamble.
Hard veto. Absolutely not. There's no purpose apart from making ourselves feel accomplished.
This is incompatible with moving the preview under the pivot, as you suggested. /cc @cinnamon-msft, since some of the refuted opinions might have been hers |
Move terminal preview to be under the pivot control/selectorThis should be possible by doing the following: <PivotItem x:Uid="Profile_Appearance">
<Grid>
<Grid.RowDefinitions>
<RowDefinition Height="Auto"/>
<RowDefinition Height="*"/>
</Grid.RowDefinitions>
<Border x:Name="ControlPreview"
Height="320"
Margin="8,0,8,0"
HorizontalAlignment="Stretch"
Grid.Row="0"/>
<ScrollViewer Grid.Row="1">
<StackPanel>
<!-- All of the settings controls -->
</ScrollViewer>
</Grid>
</PivotItem Tested it out myself, and that doesn't break the scrolling. Now, the only question left is...
This'll require a broader discussion on the design. Let's talk about this at our next sync meeting. Align the preview to the leftI agree that this'll look bad too (sorry Kayla haha). But it should be a quick one-line change. @PankajBhojwani mind making that change and sending it over to @cinnamon-msft (or even the team over email if you want). I think it's just a matter of visualizing it tbh. Content inside the previewThis'll require a broader discussion on the design. Let's talk about this at our next sync meeting. |
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.
Honestly, it seems like most of the issues left are design-related.
Blocking for two reasons...
- we need consensus on the design (we'll talk about that in the upcoming sync meeting)
- resolution on using
TerminalSettings::ApplyProfileSettings
instead ofTermControl::Settings
setter
Also, don't forget to update the PR description.
WINRT_PROPERTY(IControlSettings, Settings); | ||
|
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.
Mildly concerned here. I'm worried that somebody later will come along and call the setter, then accidentally completely break the TerminalSettings
inheritance line this way.
Can we just expose TerminalSettings::_ApplyProfileSettings()
and use that instead?
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.
Omg, I'm realizing that I told you earlier to change this to WINRT_PROPERTY
. I was looking at the code, not the architecture. Sorry! haha
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 don't necessarily think this should be part of TS's public API.
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.
Eh, fine. But I'm curious, why not?
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'm firmly opposed to the idea that TermControl has any concept of a Profile. It should be given a fully-constructed IControlSettings
, and it's up to the application hosting the control to decide how to populate that object.
I agree that exposing the Settings setter is definitely a footgun. I don't have a better idea right now though...
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'm still confused about your (plural "you") hesitance on exposing TerminalSettings::ApplyProfileSettings
and going with exposing a setter on TermControl::Settings
. An inheritable TerminalSettings
was designed to be that you would never have to set anything, just add children. TermControl
does/should not have any knowledge of a TerminalSettings
; it's only working with IControlSettings
, I agree with that.
My proposal is that TerminalSettingsEditor
(the "application hosting the control") calls control.Settings().as<TerminalSettings>().ApplyProfileSettings(preview)
. That's how we generally interact with a TermControl's settings anyways so it's not a crazy idea.
This proposal also doesn't expose the setter. In exposing the setter, there's a risk that someone will inevitably call the setter in TermApp
and break the inheritance tree for TerminalSettings
. The inheritance tree is becoming more and more important, especially as we added in AppearanceConfig
(a great example of UnfocusedAppearance
inheriting from DefaultAppearance
, and how calling the setter could easily break everything).
If you don't feel that this shouldn't be a part of TS's public API, fine. But please give me more of a response than "nah" so I can learn from this.
<VisualStateManager.VisualStateGroups> | ||
<VisualStateGroup> | ||
<VisualState> | ||
<VisualState.StateTriggers> | ||
<!-- | ||
Official guidance states that 640 is the breakpoint between small and medium devices. | ||
Since MinWindowWidth is an inclusive range, we need to add 1 to it. | ||
--> | ||
<AdaptiveTrigger MinWindowWidth="641" /> | ||
</VisualState.StateTriggers> | ||
|
||
<VisualState.Setters> | ||
<Setter Target="ControlPreview.Width" Value="640" /> | ||
</VisualState.Setters> | ||
</VisualState> | ||
</VisualStateGroup> | ||
</VisualStateManager.VisualStateGroups> |
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.
You can't do this by adding MinWidth="640"
to the Border
?
(or would it be MaxWidth="640"
? I always get confused with visual states haha)
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.
Omg you are completely right, we don't need this visual state manager at all! Thanks :)
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.
Had to add this back in because we now change the horizontal alignment with this, and for some reason that causes the preview to disappear into oblivion unless we also set the width
void PreviewConnection::Resize(uint32_t /*rows*/, uint32_t /*columns*/) noexcept | ||
{ | ||
} |
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.
This method still needs to be implemented for that right?
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'm seeing a crash -- blocking for bug bash
…/pabhoj/sui_control_preview
…/pabhoj/sui_control_preview
Let's get the conflicts resolved! 😄 |
We use the control's width and height now!
Nothing, the control's width and height are fixed
We use a system theme resource colour now :) |
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.
Future design notes:
Having the TermSettings come out of the viewmodel is a good idea. If you embrace this, you can make the viewmodel send a settings change notification for TermSettings
, and you can delete all of the UpdateSettings
calls from Profiles itself. This would be letting the VM do the work for you.
For now, though: I'm going to apply my comments and merge this. Excellent work, and thank you for doing it!
void PreviewConnection::Resize(uint32_t /*rows*/, uint32_t /*columns*/) noexcept | ||
{ | ||
} |
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.
Wait, no.. on resize, the connection must re-print the entire preview text. Resize with reflow destroys the content that goes off the top of the screen, and the connection needs to clear the screen and put it back. Now, since we never resize the control this isn't going to be a problem... but do keep it in mind.
BorderBrush="#333333" | ||
BorderThickness="1" /> | ||
BorderThickness="1" | ||
BorderBrush="{ThemeResource ToggleButtonDisabledBorderThemeBrush}"/> |
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.
As long as this.. uh, looks fine. I guess. It's a weird one to use-- we're not a toggle button 😄
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.
This is an older commit! It uses SystemControlForegroundBaseMediumLowBrush
🎉 Handy links: |
🎉 Handy links: |
In the 'Appearance' tab of a profile, show a preview of what the control looks like
PR Checklist