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

Only update the icon of a tab it the icon actually _changed_ #2376

Merged
merged 4 commits into from
Aug 14, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
42 changes: 18 additions & 24 deletions src/cascadia/TerminalApp/App.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include "App.g.cpp"
#include "TerminalPage.h"
#include "Utils.h"

using namespace winrt::Windows::ApplicationModel::DataTransfer;
using namespace winrt::Windows::UI::Xaml;
Expand Down Expand Up @@ -686,16 +687,15 @@ namespace winrt::TerminalApp::implementation
if (lastFocusedProfileOpt.has_value())
{
const auto lastFocusedProfile = lastFocusedProfileOpt.value();

auto tabViewItem = tab->GetTabViewItem();
tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this, lastFocusedProfile, tabViewItem]() {
// _GetIconFromProfile has to run on the main thread
const auto* const matchingProfile = _settings->FindProfile(lastFocusedProfile);
if (matchingProfile)
{
tabViewItem.Icon(App::_GetIconFromProfile(*matchingProfile));
}
});
const auto* const matchingProfile = _settings->FindProfile(lastFocusedProfile);
if (matchingProfile)
{
tab->UpdateIcon(matchingProfile->GetExpandedIconPath());
}
else
{
tab->UpdateIcon({});
}
}
}

Expand Down Expand Up @@ -928,7 +928,7 @@ namespace winrt::TerminalApp::implementation
// Set this profile's tab to the icon the user specified
if (profile != nullptr && profile->HasIcon())
{
tabViewItem.Icon(_GetIconFromProfile(*profile));
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
newTab->UpdateIcon(profile->GetExpandedIconPath());
}

tabViewItem.PointerPressed({ this, &App::_OnTabClick });
Expand Down Expand Up @@ -1252,19 +1252,13 @@ namespace winrt::TerminalApp::implementation
{
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
if (profile.HasIcon())
{
std::wstring path{ profile.GetIconPath() };
const auto envExpandedPath{ wil::ExpandEnvironmentStringsW<std::wstring>(path.data()) };
winrt::hstring iconPath{ envExpandedPath };
winrt::Windows::Foundation::Uri iconUri{ iconPath };
Controls::BitmapIconSource iconSource;
// Make sure to set this to false, so we keep the RGB data of the
// image. Otherwise, the icon will be white for all the
// non-transparent pixels in the image.
iconSource.ShowAsMonochrome(false);
iconSource.UriSource(iconUri);
Controls::IconSourceElement elem;
elem.IconSource(iconSource);
return elem;
try
{
winrt::hstring iconPath{ profile.GetExpandedIconPath() };
return GetColoredIcon(iconPath);
}
CATCH_LOG();
return { nullptr };
}
else
{
Expand Down
15 changes: 10 additions & 5 deletions src/cascadia/TerminalApp/Profile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -568,14 +568,19 @@ void Profile::SetIconPath(std::wstring_view path)
}

// Method Description:
// - Returns this profile's icon path, if one is set. Otherwise returns the empty string.
// - Returns this profile's icon path, if one is set. Otherwise returns the
// empty string. This method will expand any environment variables in the
// path, if there are any.
// Return Value:
// - this profile's icon path, if one is set. Otherwise returns the empty string.
std::wstring_view Profile::GetIconPath() const noexcept
winrt::hstring Profile::GetExpandedIconPath() const
{
return HasIcon() ?
std::wstring_view{ _icon.value().c_str(), _icon.value().size() } :
std::wstring_view{ L"", 0 };
if (!HasIcon())
{
return { L"" };
}
winrt::hstring envExpandedPath{ wil::ExpandEnvironmentStringsW<std::wstring>(_icon.value().data()) };
return envExpandedPath;
}

// Method Description:
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/Profile.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class TerminalApp::Profile final
void SetConnectionType(GUID connectionType) noexcept;

bool HasIcon() const noexcept;
std::wstring_view GetIconPath() const noexcept;
winrt::hstring GetExpandedIconPath() const;
void SetIconPath(std::wstring_view path);

bool GetCloseOnExit() const noexcept;
Expand Down
16 changes: 16 additions & 0 deletions src/cascadia/TerminalApp/Tab.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include "pch.h"
#include "Tab.h"
#include "Utils.h"

using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Core;
Expand Down Expand Up @@ -142,6 +143,21 @@ void Tab::UpdateFocus()
_rootPane->UpdateFocus();
}

void Tab::UpdateIcon(const winrt::hstring iconPath)
{
// Don't reload our icon if it hasn't changed.
if (iconPath == _lastIconPath)
{
return;
}

_lastIconPath = iconPath;

_tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
_tabViewItem.Icon(GetColoredIcon(_lastIconPath));
Copy link
Contributor

Choose a reason for hiding this comment

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

This one lost its exception handler.. It also no longer checks whether the path was empty. Does this work right for transitioning from "an icon" to "no icon"?

Copy link
Member Author

Choose a reason for hiding this comment

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

The exception handler went up into GetColoredIcon, but I'll double check the icon->null path

});
}

// Method Description:
// - Gets the title string of the last focused terminal control in our tree.
// Returns the empty string if there is no such control.
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalApp/Tab.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ class Tab
void AddHorizontalSplit(const GUID& profile, winrt::Microsoft::Terminal::TerminalControl::TermControl& control);

void UpdateFocus();
void UpdateIcon(const winrt::hstring iconPath);

void ResizeContent(const winrt::Windows::Foundation::Size& newSize);
void ResizePane(const winrt::TerminalApp::Direction& direction);
void NavigateFocus(const winrt::TerminalApp::Direction& direction);
Expand All @@ -37,6 +39,7 @@ class Tab

private:
std::shared_ptr<Pane> _rootPane{ nullptr };
winrt::hstring _lastIconPath{};
DHowett-MSFT marked this conversation as resolved.
Show resolved Hide resolved

bool _focused{ false };
winrt::Microsoft::UI::Xaml::Controls::TabViewItem _tabViewItem{ nullptr };
Expand Down
27 changes: 27 additions & 0 deletions src/cascadia/TerminalApp/Utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,30 @@ std::wstring GetWstringFromJson(const Json::Value& json)
{
return winrt::to_hstring(json.asString()).c_str();
}

// Method Description:
// - Creates an IconElement for the given path. The icon returned is a colored
// icon. If we couldn't create the icon for any reason, we return an empty
// IconElement.
// Arguments:
// - path: the full, expanded path to the icon.
// Return Value:
// - An IconElement with its IconSource set, if possible.
winrt::Windows::UI::Xaml::Controls::IconElement GetColoredIcon(const winrt::hstring& path)
{
winrt::Windows::UI::Xaml::Controls::IconSourceElement elem{};
try
{
winrt::Windows::Foundation::Uri iconUri{ path };
winrt::Windows::UI::Xaml::Controls::BitmapIconSource iconSource;
// Make sure to set this to false, so we keep the RGB data of the
// image. Otherwise, the icon will be white for all the
// non-transparent pixels in the image.
iconSource.ShowAsMonochrome(false);
iconSource.UriSource(iconUri);
elem.IconSource(iconSource);
}
CATCH_LOG();

return elem;
}
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/Utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,5 @@ inline std::string JsonKey(const std::string_view key)
{
return static_cast<std::string>(key);
}

winrt::Windows::UI::Xaml::Controls::IconElement GetColoredIcon(const winrt::hstring& path);