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

Add a simple page for keybindings #9253

Merged
11 commits merged into from Feb 23, 2021
6 changes: 3 additions & 3 deletions src/cascadia/TerminalApp/ActionPaletteItem.cpp
Expand Up @@ -23,7 +23,7 @@ namespace winrt::TerminalApp::implementation
{
Name(command.Name());
KeyChordText(command.KeyChordText());
Icon(command.Icon());
Icon(command.IconPath());

_commandChangedRevoker = command.PropertyChanged(winrt::auto_revoke, [weakThis{ get_weak() }](auto& sender, auto& e) {
auto item{ weakThis.get() };
Expand All @@ -40,9 +40,9 @@ namespace winrt::TerminalApp::implementation
{
item->KeyChordText(senderCommand.KeyChordText());
}
else if (changedProperty == L"Icon")
else if (changedProperty == L"IconPath")
{
item->Icon(senderCommand.Icon());
item->Icon(senderCommand.IconPath());
}
}
});
Expand Down
22 changes: 14 additions & 8 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Expand Up @@ -93,21 +93,27 @@ namespace winrt::TerminalApp::implementation
winrt::fire_and_forget TerminalPage::SetSettings(CascadiaSettings settings, bool needRefreshUI)
{
_settings = settings;
if (needRefreshUI)
{
_RefreshUIForSettingsReload();
}

// Upon settings update we reload the system settings for scrolling as well.
// TODO: consider reloading this value periodically.
_systemRowsToScroll = _ReadSystemRowsToScroll();

auto weakThis{ get_weak() };
co_await winrt::resume_foreground(Dispatcher());
if (auto page{ weakThis.get() })
{
// Make sure to _UpdateCommandsForPalette before
// _RefreshUIForSettingsReload. _UpdateCommandsForPalette will make
// sure the KeyChordText of Commands is updated, which needs to
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
// happen before the Settings UI is reloaded and tries to re-read
// those values.
_UpdateCommandsForPalette();
CommandPalette().SetKeyMap(_settings.KeyMap());

if (needRefreshUI)
{
_RefreshUIForSettingsReload();
}

// Upon settings update we reload the system settings for scrolling as well.
// TODO: consider reloading this value periodically.
_systemRowsToScroll = _ReadSystemRowsToScroll();
}
}

Expand Down
51 changes: 51 additions & 0 deletions src/cascadia/TerminalSettingsEditor/Actions.cpp
@@ -0,0 +1,51 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "pch.h"
#include "Actions.h"
#include "Actions.g.cpp"
#include "ActionsPageNavigationState.g.cpp"
#include "EnumEntry.h"

using namespace winrt::Windows::UI::Xaml::Navigation;
using namespace winrt::Windows::Foundation;
using namespace winrt::Microsoft::Terminal::Settings::Model;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
Actions::Actions()
{
InitializeComponent();

_filteredActions = winrt::single_threaded_observable_vector<winrt::Microsoft::Terminal::Settings::Model::Command>();
}

void Actions::OnNavigatedTo(const NavigationEventArgs& e)
{
_State = e.Parameter().as<Editor::ActionsPageNavigationState>();

for (const auto& [k, command] : _State.Settings().GlobalSettings().Commands())
{
// Filter out nested commands, and commands that aren't bound to a
// key. This page is currently just for displaying the actions that
// _are_ bound to keys.
if (command.HasNestedCommands() || command.KeyChordText().empty())
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
{
continue;
}
_filteredActions.Append(command);
}
}

Collections::IObservableVector<Command> Actions::FilteredActions()
{
return _filteredActions;
}

void Actions::_OpenSettingsClick(const IInspectable& /*sender*/,
const Windows::UI::Xaml::RoutedEventArgs& /*eventArgs*/)
{
_State.RequestOpenJson();
}

}
49 changes: 49 additions & 0 deletions src/cascadia/TerminalSettingsEditor/Actions.h
@@ -0,0 +1,49 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

#include "Actions.g.h"
#include "ActionsPageNavigationState.g.h"
#include "Utils.h"

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
struct ActionsPageNavigationState : ActionsPageNavigationStateT<ActionsPageNavigationState>
{
public:
ActionsPageNavigationState(const Model::CascadiaSettings& settings) :
_Settings{ settings } {}

void RequestOpenJson()
{
_OpenJsonHandlers(nullptr, winrt::Microsoft::Terminal::Settings::Model::SettingsTarget::SettingsFile);
}

GETSET_PROPERTY(Model::CascadiaSettings, Settings, nullptr)
TYPED_EVENT(OpenJson, Windows::Foundation::IInspectable, Model::SettingsTarget);
};

struct Actions : ActionsT<Actions>
{
public:
Actions();

void OnNavigatedTo(const winrt::Windows::UI::Xaml::Navigation::NavigationEventArgs& e);

Windows::Foundation::Collections::IObservableVector<winrt::Microsoft::Terminal::Settings::Model::Command> FilteredActions();

GETSET_PROPERTY(Editor::ActionsPageNavigationState, State, nullptr);

private:
friend struct ActionsT<Actions>; // for Xaml to bind events
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
Windows::Foundation::Collections::IObservableVector<winrt::Microsoft::Terminal::Settings::Model::Command> _filteredActions{ nullptr };

void _OpenSettingsClick(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);
};
}

namespace winrt::Microsoft::Terminal::Settings::Editor::factory_implementation
{
BASIC_FACTORY(Actions);
}
23 changes: 23 additions & 0 deletions src/cascadia/TerminalSettingsEditor/Actions.idl
@@ -0,0 +1,23 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

import "EnumEntry.idl";

namespace Microsoft.Terminal.Settings.Editor
{
runtimeclass ActionsPageNavigationState
{
Microsoft.Terminal.Settings.Model.CascadiaSettings Settings;
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we only need GlobalAppSettings to get access to the list of commands. We should pass that in instead of the whole settings object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, this was me being quick and dirty

Copy link
Member

Choose a reason for hiding this comment

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

I suspect we will eventually need the whole thing. Just like Profiles needs Color Schemes.

what we really need is a viewmodel.

void RequestOpenJson();
event Windows.Foundation.TypedEventHandler<Object, Microsoft.Terminal.Settings.Model.SettingsTarget> OpenJson;
};

[default_interface] runtimeclass Actions : Windows.UI.Xaml.Controls.Page
{
Actions();
ActionsPageNavigationState State { get; };

IObservableVector<Microsoft.Terminal.Settings.Model.Command> FilteredActions { get; };

}
}
189 changes: 189 additions & 0 deletions src/cascadia/TerminalSettingsEditor/Actions.xaml
@@ -0,0 +1,189 @@
<!-- Copyright (c) Microsoft Corporation. All rights reserved. Licensed under
the MIT License. See LICENSE in the project root for license information. -->
<Page
x:Class="Microsoft.Terminal.Settings.Editor.Actions"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:local="using:Microsoft.Terminal.Settings.Editor"
xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
xmlns:muxc="using:Microsoft.UI.Xaml.Controls"
xmlns:SettingsModel="using:Microsoft.Terminal.Settings.Model"
mc:Ignorable="d">

<Page.Resources>
<ResourceDictionary>
<ResourceDictionary.MergedDictionaries>
<ResourceDictionary Source="CommonResources.xaml"/>
</ResourceDictionary.MergedDictionaries>

<local:StringIsEmptyConverter x:Key="CommandKeyChordVisibilityConverter"/>

<!-- Template for actions. This is _heavily_ copied from the command
palette, with modifications:
* We don't need to use a HighlightedTextControl, because we're
not filtering this list
* We don't need the chevron for nested commands
* We're not displaying the icon
Comment on lines +26 to +27
Copy link
Member

Choose a reason for hiding this comment

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

Wait... we should definitely handle these cases though. We're not showing the user what's in their settings.

At the very least, we should update the disclaimer to say that these are omitted right (regarding nested commands)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm gonna punt this to "have a real page for actions / commands". This is literally just the "do it quick and dirty" version.

I'll see if adding the icon is a pain though. That might be slick.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, at least the icons would be nice to have haha

* We're binding directly to a Command, not a FilteredCommand

If we wanted to reuse the command palette's list more directly,
that's theoretically possible, but then it would need to be
lifted out of TerminalApp and either moved into the
TerminalSettingsEditor or moved to it's own project consumed by
both TSE and TerminalApp.
-->
<DataTemplate x:Key="GeneralItemTemplate" x:DataType="SettingsModel:Command">

<!-- This HorizontalContentAlignment="Stretch" is important
to make sure it takes the entire width of the line -->
<ListViewItem HorizontalContentAlignment="Stretch"
AutomationProperties.Name="{x:Bind Name, Mode=OneWay}"
AutomationProperties.AcceleratorKey="{x:Bind KeyChordText, Mode=OneWay}">
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved

<Grid HorizontalAlignment="Stretch" ColumnSpacing="8" >
<Grid.ColumnDefinitions>
<ColumnDefinition Width="16"/>
<!-- icon -->
<ColumnDefinition Width="Auto"/>
<!-- command label -->
<ColumnDefinition Width="*"/>
<!-- key chord -->
<ColumnDefinition Width="16"/>
<!-- gutter for scrollbar -->
</Grid.ColumnDefinitions>

<TextBlock Grid.Column="1"
HorizontalAlignment="Left"
Text="{x:Bind Name, Mode=OneWay}"/>

<!-- The block for the key chord is only visible
when there's actual text set as the label. See
CommandKeyChordVisibilityConverter for details.
We're setting the accessibility view on the
border and text block to Raw because otherwise,
Narrator will read out the key chord. Problem is,
it already did that because it was the list item's
"AcceleratorKey". It's redundant. -->
<Border Grid.Column="2"
Visibility="{x:Bind KeyChordText,
Mode=OneWay,
Converter={StaticResource CommandKeyChordVisibilityConverter}}"
Style="{ThemeResource KeyChordBorderStyle}"
Padding="2,0,2,0"
HorizontalAlignment="Right"
VerticalAlignment="Center"
AutomationProperties.AccessibilityView="Raw">

<TextBlock Style="{ThemeResource KeyChordTextBlockStyle}"
FontSize="12"
Text="{x:Bind KeyChordText, Mode=OneWay}"
AutomationProperties.AccessibilityView="Raw" />
</Border>
</Grid>
</ListViewItem>
</DataTemplate>

<!-- These resources again, HEAVILY copied from the command palette -->
<ResourceDictionary.ThemeDictionaries>
<ResourceDictionary x:Key="Dark">
<!-- TextBox colors !-->
<SolidColorBrush x:Key="TextControlBackground" Color="#333333"/>
<SolidColorBrush x:Key="TextBoxPlaceholderTextThemeBrush" Color="#B5B5B5"/>
<SolidColorBrush x:Key="TextControlForeground" Color="#B5B5B5"/>
<SolidColorBrush x:Key="TextControlBorderBrush" Color="#404040"/>
<SolidColorBrush x:Key="TextControlButtonForeground" Color="#B5B5B5"/>

<SolidColorBrush x:Key="TextControlBackgroundPointerOver" Color="#404040"/>
<SolidColorBrush x:Key="TextControlForegroundPointerOver" Color="#FFFFFF"/>
<SolidColorBrush x:Key="TextControlBorderBrushPointerOver" Color="#404040"/>
<SolidColorBrush x:Key="TextControlButtonForegroundPointerOver" Color="#FF4343"/>

<SolidColorBrush x:Key="TextControlBackgroundFocused" Color="#333333"/>
<SolidColorBrush x:Key="TextControlForegroundFocused" Color="#FFFFFF"/>
<SolidColorBrush x:Key="TextControlBorderBrushFocused" Color="#404040"/>
<SolidColorBrush x:Key="TextControlButtonForegroundPressed" Color="#FFFFFF"/>
<SolidColorBrush x:Key="TextControlButtonBackgroundPressed" Color="#FF4343"/>

<!-- KeyChordText styles -->
<Style x:Key="KeyChordBorderStyle" TargetType="Border">
<Setter Property="BorderThickness" Value="1" />
<Setter Property="CornerRadius" Value="1" />
<Setter Property="Background" Value="{ThemeResource SystemAltMediumLowColor}" />
<Setter Property="BorderBrush" Value="{ThemeResource SystemControlForegroundBaseMediumBrush}" />
</Style>
<Style x:Key="KeyChordTextBlockStyle" TargetType="TextBlock">
<Setter Property="Foreground" Value="{ThemeResource SystemControlForegroundBaseMediumBrush}" />
</Style>

</ResourceDictionary>
<ResourceDictionary x:Key="Light">
<!-- TextBox colors !-->
<SolidColorBrush x:Key="TextControlBackground" Color="#CCCCCC"/>
<SolidColorBrush x:Key="TextBoxPlaceholderTextThemeBrush" Color="#636363"/>
<SolidColorBrush x:Key="TextControlBorderBrush" Color="#636363"/>
<SolidColorBrush x:Key="TextControlButtonForeground" Color="#636363"/>

<SolidColorBrush x:Key="TextControlBackgroundPointerOver" Color="#DADADA"/>
<SolidColorBrush x:Key="TextControlBorderBrushPointerOver" Color="#636363"/>
<SolidColorBrush x:Key="TextControlButtonForegroundPointerOver" Color="#FF4343"/>

<SolidColorBrush x:Key="TextControlBackgroundFocused" Color="#CCCCCC"/>
<SolidColorBrush x:Key="TextControlBorderBrushFocused" Color="#636363"/>
<SolidColorBrush x:Key="TextControlButtonForegroundPressed" Color="#FFFFFF"/>
<SolidColorBrush x:Key="TextControlButtonBackgroundPressed" Color="#FF4343"/>

<!-- KeyChordText styles -->
<Style x:Key="KeyChordBorderStyle" TargetType="Border">
<Setter Property="BorderThickness" Value="1" />
<Setter Property="CornerRadius" Value="1" />
<Setter Property="Background" Value="{ThemeResource SystemAltMediumLowColor}" />
<Setter Property="BorderBrush" Value="{ThemeResource SystemControlForegroundBaseMediumBrush}" />
</Style>
<Style x:Key="KeyChordTextBlockStyle" TargetType="TextBlock">
<Setter Property="Foreground" Value="{ThemeResource SystemControlForegroundBaseMediumBrush}" />
</Style>

</ResourceDictionary>
<ResourceDictionary x:Key="HighContrast">

<!-- KeyChordText styles (use XAML defaults for High Contrast theme) -->
<Style x:Key="KeyChordBorderStyle" TargetType="Border"/>
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
<Style x:Key="KeyChordTextBlockStyle" TargetType="TextBlock"/>

</ResourceDictionary>
</ResourceDictionary.ThemeDictionaries>


</ResourceDictionary>
</Page.Resources>

<ScrollViewer>
<StackPanel Style="{StaticResource SettingsStackStyle}">
<TextBlock x:Uid="Globals_KeybindingsDisclaimer"
Style="{StaticResource DisclaimerStyle}"/>
Comment on lines +159 to +160
Copy link
Member

Choose a reason for hiding this comment

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

(not @zadjii-msft's fault, but still relevant) This text block is not read by a screen reader (same with the disclaimer in base layer). I'll file a separate issue to track this one.

Copy link
Member

Choose a reason for hiding this comment

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

This is going to be important for us to fix before any settings UI goes stable. Why are they not read?

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to:
"Disclaimer" text boxes are not read in narrator #9255

<HyperlinkButton x:Uid="Globals_KeybindingsLink"
Click="_OpenSettingsClick" />

<!-- Keybindings -->

<!-- NOTE: Globals_Keybindings.Header is not defined, because that
would result in the page having "Keybindings" displayed twice, which
looks quite redundant -->
<ContentPresenter x:Uid="Globals_Keybindings" Margin="0">

<ListView HorizontalAlignment="Stretch"
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we should have a header for each column here. But that's so much extra work for a small release window. Plus additional loc burden.

VerticalAlignment="Stretch"
SelectionMode="Single"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SelectionMode="Single"
SelectionMode="None"
IsItemClickEnabled="False"

Should we make it so that you can't interact with this? There's no reason for a user to select entries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, why not?

CanReorderItems="False"
AllowDrop="False"
ItemsSource="{x:Bind FilteredActions}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ItemsSource="{x:Bind FilteredActions}"
ItemsSource="{x:Bind FilteredActions, Mode="OneWay"}"

We should bind this OneWay. This may have caused some mayhem for you in trying to figure out why it didn't repopulate?

ItemTemplate="{StaticResource GeneralItemTemplate}">
</ListView>

</ContentPresenter>

</StackPanel>

</ScrollViewer>
</Page>