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.

Sign up for GitHub

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

Jump to bottom

[Settings UI] [Colour schemes page] Redesign Colour Buttons #8997

Closed
wants to merge 5 commits into from
Closed

[Settings UI] [Colour schemes page] Redesign Colour Buttons #8997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
731c75f
Redesign colour buttons
Chips1234 Feb 1, 2021
63f86a9
Add non-bright colours row
Chips1234 Feb 5, 2021
56d3f42
Split colour table into two
Chips1234 Feb 14, 2021
062b9f8
Merge branch 'main' into dev/chips1234/redesign-colour-buttons-8765
Chips1234 Feb 14, 2021
3a3477f
Fix error
Chips1234 Feb 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 66 additions & 24 deletions src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,25 @@ using namespace winrt::Windows::Foundation::Collections;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
static constexpr int ColorTableDivider = 8;

static const std::array<hstring, 16> TableColorNames = {
RS_(L"ColorScheme_Black/Header"),
RS_(L"ColorScheme_Red/Header"),
RS_(L"ColorScheme_Green/Header"),
RS_(L"ColorScheme_Yellow/Header"),
RS_(L"ColorScheme_Blue/Header"),
RS_(L"ColorScheme_Purple/Header"),
RS_(L"ColorScheme_Cyan/Header"),
RS_(L"ColorScheme_White/Header"),
RS_(L"ColorScheme_BrightBlack/Header"),
RS_(L"ColorScheme_BrightRed/Header"),
RS_(L"ColorScheme_BrightGreen/Header"),
RS_(L"ColorScheme_BrightYellow/Header"),
RS_(L"ColorScheme_BrightBlue/Header"),
RS_(L"ColorScheme_BrightPurple/Header"),
RS_(L"ColorScheme_BrightCyan/Header"),
RS_(L"ColorScheme_BrightWhite/Header")
RS_(L"ColorScheme_Black/ToolTip"),
RS_(L"ColorScheme_Red/ToolTip"),
RS_(L"ColorScheme_Green/ToolTip"),
RS_(L"ColorScheme_Yellow/ToolTip"),
RS_(L"ColorScheme_Blue/ToolTip"),
RS_(L"ColorScheme_Purple/ToolTip"),
RS_(L"ColorScheme_Cyan/ToolTip"),
RS_(L"ColorScheme_White/ToolTip"),
RS_(L"ColorScheme_BrightBlack/ToolTip"),
RS_(L"ColorScheme_BrightRed/ToolTip"),
RS_(L"ColorScheme_BrightGreen/ToolTip"),
RS_(L"ColorScheme_BrightYellow/ToolTip"),
RS_(L"ColorScheme_BrightBlue/ToolTip"),
RS_(L"ColorScheme_BrightPurple/ToolTip"),
RS_(L"ColorScheme_BrightCyan/ToolTip"),
RS_(L"ColorScheme_BrightWhite/ToolTip")
};

static const std::array<std::wstring, 9> InBoxSchemes = {
Expand All @@ -53,7 +55,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

ColorSchemes::ColorSchemes() :
_ColorSchemeList{ single_threaded_observable_vector<Model::ColorScheme>() },
_CurrentColorTable{ single_threaded_observable_vector<Editor::ColorTableEntry>() }
_CurrentNonBrightColorTable{ single_threaded_observable_vector<Editor::ColorTableEntry>() },
_CurrentBrightColorTable{ single_threaded_observable_vector<Editor::ColorTableEntry>() }
{
InitializeComponent();
}
Expand All @@ -71,7 +74,15 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
for (uint8_t i = 0; i < TableColorNames.size(); ++i)
{
auto entry = winrt::make<ColorTableEntry>(i, Windows::UI::Color{ 0, 0, 0, 0 });
_CurrentColorTable.Append(entry);

if (i < ColorTableDivider)
{
_CurrentNonBrightColorTable.Append(entry);
}
else
{
_CurrentBrightColorTable.Append(entry);
}
}

// Try to look up the scheme that was navigated to. If we find it, immediately select it.
Expand Down Expand Up @@ -137,16 +148,16 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
}

// Function Description:
// - Called when a ColorPicker control has selected a new color. This is specifically
// - Called when a ColorPicker control of the non-bright colour table has selected a new color. This is specifically
// called by color pickers assigned to a color table entry. It takes the index
// that's been stuffed in the Tag property of the color picker and uses it
// to update the color table accordingly.
// to update the non-bright color table accordingly.
// Arguments:
// - sender: the color picker that raised this event.
// - sender: the color picker of the non-bright colour table that raised this event.
// - args: the args that contains the new color that was picked.
// Return Value:
// - <none>
void ColorSchemes::ColorPickerChanged(IInspectable const& sender,
void ColorSchemes::NonBrightColorPickerChanged(IInspectable const& sender,
ColorChangedEventArgs const& args)
{
if (auto picker = sender.try_as<ColorPicker>())
Expand All @@ -155,7 +166,31 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
auto index = winrt::unbox_value<uint8_t>(tag);
CurrentColorScheme().SetColorTableEntry(index, args.NewColor());
_CurrentColorTable.GetAt(index).Color(args.NewColor());
_CurrentNonBrightColorTable.GetAt(index).Color(args.NewColor());
}
}
}

// Function Description:
// - Called when a ColorPicker control of the bright colour table has selected a new color. This is specifically
// called by color pickers assigned to a color table entry. It takes the index
// that's been stuffed in the Tag property of the color picker and uses it
// to update the bright color table accordingly.
// Arguments:
// - sender: the color picker of the bright colour table that raised this event.
// - args: the args that contains the new color that was picked.
// Return Value:
// - <none>
void ColorSchemes::BrightColorPickerChanged(IInspectable const& sender,
ColorChangedEventArgs const& args)
{
if (auto picker = sender.try_as<ColorPicker>())
{
if (auto tag = picker.Tag())
{
auto index = winrt::unbox_value<uint8_t>(tag);
CurrentColorScheme().SetColorTableEntry(index, args.NewColor());
_CurrentBrightColorTable.GetAt(index - ColorTableDivider).Color(args.NewColor());
}
}
}
Expand Down Expand Up @@ -284,6 +319,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

// Function Description:
// - Updates the currently modifiable color table based on the given current color scheme.
// There are 7 non-bright colours and 7 bright colours.
Copy link

@htcfreek htcfreek Feb 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is false. we have 8+8 colors. But the list index goes from 0-7. If you count from 0-7 you count 8 times.

// Arguments:
// - colorScheme: the color scheme to retrieve the color table from
// Return Value:
Expand All @@ -292,7 +328,13 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
for (uint8_t i = 0; i < TableColorNames.size(); ++i)
{
_CurrentColorTable.GetAt(i).Color(colorScheme.Table()[i]);
// Fill the element of "_CurrentNonBrightColorTable" with the non-bright colors (lower index on the color scheme object's table: 0-7).
int indexNonBrightColor = i;
// Fill the element of "_CurrentBrightColorTable" with the bright colors (higher index on the color scheme object's table: 8-15).
int indexBrightColor = i + ColorTableDivider;

_CurrentNonBrightColorTable.GetAt(i).Color(colorScheme.Table()[indexNonBrightColor]);
_CurrentBrightColorTable.GetAt(i).Color(colorScheme.Table()[indexBrightColor]);
}
Comment on lines 329 to 338
Copy link

@htcfreek htcfreek Feb 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Have you really tried this code?
I think this can't fix the error because you haven't used the whole code. (See my review before.)

  1. You didn't changed the condition of the loop which leads imo to to many iterations. (We only need 8 iterations from 0 to 7 and not 16 iterations from 0 to 15.)

  2. Please copy all my comments in the code suggestion. Now there are information that are missing.

}

Expand Down
6 changes: 4 additions & 2 deletions src/cascadia/TerminalSettingsEditor/ColorSchemes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
void OnNavigatedTo(const winrt::Windows::UI::Xaml::Navigation::NavigationEventArgs& e);

void ColorSchemeSelectionChanged(winrt::Windows::Foundation::IInspectable const& sender, winrt::Windows::UI::Xaml::Controls::SelectionChangedEventArgs const& args);
void ColorPickerChanged(winrt::Windows::Foundation::IInspectable const& sender, winrt::Windows::UI::Xaml::Controls::ColorChangedEventArgs const& args);
void NonBrightColorPickerChanged(winrt::Windows::Foundation::IInspectable const& sender, winrt::Windows::UI::Xaml::Controls::ColorChangedEventArgs const& args);
void BrightColorPickerChanged(winrt::Windows::Foundation::IInspectable const& sender, winrt::Windows::UI::Xaml::Controls::ColorChangedEventArgs const& args);
void AddNew_Click(winrt::Windows::Foundation::IInspectable const& sender, winrt::Windows::UI::Xaml::RoutedEventArgs const& e);

void Rename_Click(winrt::Windows::Foundation::IInspectable const& sender, winrt::Windows::UI::Xaml::RoutedEventArgs const& e);
Expand All @@ -39,7 +40,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
void DeleteConfirmation_Click(winrt::Windows::Foundation::IInspectable const& sender, winrt::Windows::UI::Xaml::RoutedEventArgs const& e);

GETSET_PROPERTY(Editor::ColorSchemesPageNavigationState, State, nullptr);
GETSET_PROPERTY(Windows::Foundation::Collections::IObservableVector<winrt::Microsoft::Terminal::Settings::Editor::ColorTableEntry>, CurrentColorTable, nullptr);
GETSET_PROPERTY(Windows::Foundation::Collections::IObservableVector<winrt::Microsoft::Terminal::Settings::Editor::ColorTableEntry>, CurrentNonBrightColorTable, nullptr);
GETSET_PROPERTY(Windows::Foundation::Collections::IObservableVector<winrt::Microsoft::Terminal::Settings::Editor::ColorTableEntry>, CurrentBrightColorTable, nullptr);
GETSET_PROPERTY(Windows::Foundation::Collections::IObservableVector<Model::ColorScheme>, ColorSchemeList, nullptr);

WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalSettingsEditor/ColorSchemes.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ namespace Microsoft.Terminal.Settings.Editor
Boolean IsRenaming { get; };

Microsoft.Terminal.Settings.Model.ColorScheme CurrentColorScheme { get; };
Windows.Foundation.Collections.IObservableVector<ColorTableEntry> CurrentColorTable;
Windows.Foundation.Collections.IObservableVector<ColorTableEntry> CurrentNonBrightColorTable;
Windows.Foundation.Collections.IObservableVector<ColorTableEntry> CurrentBrightColorTable;
Windows.Foundation.Collections.IObservableVector<Microsoft.Terminal.Settings.Model.ColorScheme> ColorSchemeList { get; };
}

Expand Down
Loading