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

[PowerRename] Show UI info if item cannot be renamed #19934

Merged
merged 11 commits into from
Aug 24, 2022
Merged

[PowerRename] Show UI info if item cannot be renamed #19934

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b9291f3
PowerRename cleanup
stefansjfw Aug 10, 2022
a1571cf
Extract ExplorerItem as a UserControl
stefansjfw Aug 11, 2022
fb4c8e5
Add VisualStateManager
stefansjfw Aug 12, 2022
39c9a6e
UI fixes
niels9001 Aug 12, 2022
6a57af9
Implement error UI logic
stefansjfw Aug 12, 2022
946459e
Merge remote-tracking branch 'origin/main' into stefan/powerrename_wip
stefansjfw Aug 16, 2022
0e3ef87
Update src/modules/powerrename/lib/PowerRenameManager.cpp
stefansjfw Aug 17, 2022
6276b6c
Update src/modules/powerrename/lib/PowerRenameManager.cpp
stefansjfw Aug 17, 2022
1942532
Folder max path is 247
stefansjfw Aug 17, 2022
660a647
Merge branch 'stefan/powerrename_wip' of github.com:microsoft/PowerTo…
stefansjfw Aug 17, 2022
4a55f22
Implement State() properly
stefansjfw Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
98 changes: 81 additions & 17 deletions src/modules/powerrename/PowerRenameUILib/ExplorerItem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,42 @@
#include "pch.h"
#include "pch.h"
#include "ExplorerItem.h"
#if __has_include("ExplorerItem.g.cpp")
#include "ExplorerItem.g.cpp"
#endif

namespace {
using namespace winrt;
using namespace Microsoft::UI::Xaml;
using namespace Microsoft::Windows::ApplicationModel::Resources;

namespace
{
const wchar_t fileImagePath[] = L"ms-appx:///Assets/file.png";
const wchar_t folderImagePath[] = L"ms-appx:///Assets/folder.png";

std::wstring PowerRenameItemRenameStatusToString(PowerRenameItemRenameStatus status)
{
switch (status)
{
case PowerRenameItemRenameStatus::Init:
{
return L"Normal";
}
case PowerRenameItemRenameStatus::ShouldRename:
{
return L"Highlight";
}
case PowerRenameItemRenameStatus::ItemNameInvalidChar:
{
return L"Error";
}
case PowerRenameItemRenameStatus::ItemNameTooLong:
{
return L"Error";
}
default:
return L"Normal";
}
}
}

namespace winrt::PowerRenameUI::implementation
Expand All @@ -13,7 +45,6 @@ namespace winrt::PowerRenameUI::implementation
m_id{ id }, m_idStr{ std::to_wstring(id) }, m_original{ original }, m_renamed{ renamed }, m_type{ type }, m_depth{ depth }, m_checked{ checked }
{
m_imagePath = (m_type == static_cast<UINT>(ExplorerItemType::Folder)) ? folderImagePath : fileImagePath;
m_highlight = m_checked && !m_renamed.empty() ? Microsoft::UI::Xaml::Visibility::Visible : Microsoft::UI::Xaml::Visibility::Collapsed;
}

int32_t ExplorerItem::Id()
Expand Down Expand Up @@ -51,17 +82,11 @@ namespace winrt::PowerRenameUI::implementation
{
m_renamed = value;
m_propertyChanged(*this, Microsoft::UI::Xaml::Data::PropertyChangedEventArgs{ L"Renamed" });

auto visibility = m_checked && !m_renamed.empty() ? Microsoft::UI::Xaml::Visibility::Visible : Microsoft::UI::Xaml::Visibility::Collapsed;
if (m_highlight != visibility)
{
m_highlight = visibility;
m_propertyChanged(*this, Microsoft::UI::Xaml::Data::PropertyChangedEventArgs{ L"Highlight" });
}
}
}

double ExplorerItem::Indentation() {
double ExplorerItem::Indentation()
{
return static_cast<double>(m_depth) * 12;
}

Expand Down Expand Up @@ -96,18 +121,35 @@ namespace winrt::PowerRenameUI::implementation
m_checked = value;
m_propertyChanged(*this, Microsoft::UI::Xaml::Data::PropertyChangedEventArgs{ L"Checked" });

auto visibility = m_checked && !m_renamed.empty() ? Microsoft::UI::Xaml::Visibility::Visible : Microsoft::UI::Xaml::Visibility::Collapsed;
if (m_highlight != visibility)
if (m_checked && !m_renamed.empty())
{
VisualStateManager::GoToState(*this, PowerRenameItemRenameStatusToString(m_state), false);
}
else
{
m_highlight = visibility;
m_propertyChanged(*this, Microsoft::UI::Xaml::Data::PropertyChangedEventArgs{ L"Highlight" });
VisualStateManager::GoToState(*this, L"Normal", false);
}
}
}

Microsoft::UI::Xaml::Visibility ExplorerItem::Highlight()
int32_t ExplorerItem::State()
{
return m_highlight;
throw hresult_not_implemented();
SeraphimaZykova marked this conversation as resolved.
Show resolved Hide resolved
}

void ExplorerItem::State(int32_t value)
{
m_state = static_cast<PowerRenameItemRenameStatus>(value);
ErrorMessageTxt().Text(StateToErrorMessage());

if (m_renamed == L"")
{
VisualStateManager::GoToState(*this, L"Normal", false);
}
else
{
VisualStateManager::GoToState(*this, PowerRenameItemRenameStatusToString(m_state), false);
}
}

winrt::event_token ExplorerItem::PropertyChanged(winrt::Microsoft::UI::Xaml::Data::PropertyChangedEventHandler const& handler)
Expand All @@ -119,4 +161,26 @@ namespace winrt::PowerRenameUI::implementation
{
m_propertyChanged.remove(token);
}

std::wstring ExplorerItem::StateToErrorMessage()
{
auto factory = winrt::get_activation_factory<ResourceManager, IResourceManagerFactory>();
ResourceManager manager = factory.CreateInstance(L"resources.pri");

switch (m_state)
{
case PowerRenameItemRenameStatus::ItemNameInvalidChar:
{
return std::wstring{ manager.MainResourceMap().GetValue(L"Resources/ErrorMessage_InvalidChar").ValueAsString() };
}
case PowerRenameItemRenameStatus::ItemNameTooLong:
{
return std::wstring{ manager.MainResourceMap().GetValue(L"Resources/ErrorMessage_FileNameTooLong").ValueAsString() };
}
default:
return {};
}

}

}
24 changes: 17 additions & 7 deletions src/modules/powerrename/PowerRenameUILib/ExplorerItem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
#pragma once
#pragma once

#include "winrt/Microsoft.UI.Xaml.h"
#include "winrt/Microsoft.UI.Xaml.Markup.h"
#include "winrt/Microsoft.UI.Xaml.Interop.h"
#include "winrt/Microsoft.UI.Xaml.Controls.Primitives.h"
#include "ExplorerItem.g.h"
#include "PowerRenameInterfaces.h"

namespace winrt::PowerRenameUI::implementation
{
Expand All @@ -10,7 +16,7 @@ namespace winrt::PowerRenameUI::implementation
Folder = 0,
File = 1
};

ExplorerItem() = default;

ExplorerItem(int32_t id, hstring const& original, hstring const& renamed, int32_t type, uint32_t depth, bool checked);
Expand All @@ -26,12 +32,14 @@ namespace winrt::PowerRenameUI::implementation
void Type(int32_t value);
bool Checked();
void Checked(bool value);
Microsoft::UI::Xaml::Visibility Highlight();
Windows::Foundation::Collections::IObservableVector<PowerRenameUI::ExplorerItem> Children();
winrt::event_token PropertyChanged(Microsoft::UI::Xaml::Data::PropertyChangedEventHandler const& handler);
int32_t State();
void State(int32_t value);
winrt::event_token PropertyChanged(winrt::Microsoft::UI::Xaml::Data::PropertyChangedEventHandler const& handler);
void PropertyChanged(winrt::event_token const& token) noexcept;

private:
std::wstring StateToErrorMessage();

int32_t m_id;
hstring m_idStr;
winrt::hstring m_original;
Expand All @@ -40,10 +48,12 @@ namespace winrt::PowerRenameUI::implementation
hstring m_imagePath;
int32_t m_type;
bool m_checked;
Microsoft::UI::Xaml::Visibility m_highlight;
PowerRenameItemRenameStatus m_state;
winrt::event<Microsoft::UI::Xaml::Data::PropertyChangedEventHandler> m_propertyChanged;

};
}

namespace winrt::PowerRenameUI::factory_implementation
{
struct ExplorerItem : ExplorerItemT<ExplorerItem, implementation::ExplorerItem>
Expand Down
4 changes: 2 additions & 2 deletions src/modules/powerrename/PowerRenameUILib/ExplorerItem.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
namespace PowerRenameUI
{
runtimeclass ExplorerItem : Microsoft.UI.Xaml.Data.INotifyPropertyChanged
[default_interface] runtimeclass ExplorerItem : Microsoft.UI.Xaml.Controls.UserControl, Microsoft.UI.Xaml.Data.INotifyPropertyChanged
{
ExplorerItem();
ExplorerItem(Int32 id, String original, String renamed, Int32 type, UInt32 depth, Boolean checked);
Expand All @@ -12,6 +12,6 @@ namespace PowerRenameUI
String ImagePath { get; };
Int32 Type;
Boolean Checked;
Microsoft.UI.Xaml.Visibility Highlight { get; };
Int32 State;
}
}
123 changes: 123 additions & 0 deletions src/modules/powerrename/PowerRenameUILib/ExplorerItem.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
<UserControl
x:Class="PowerRenameUI.ExplorerItem"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
xmlns:local="using:PowerRenameUI"
xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
mc:Ignorable="d">

<Grid Height="28" Padding="0,0,0,0">
<Grid.ColumnDefinitions>
<ColumnDefinition Width="*" />
<ColumnDefinition Width="4" />
<!-- Some spacing in between -->
<ColumnDefinition Width="*" />
<ColumnDefinition Width="Auto" />
<!-- Error tooltip -->
</Grid.ColumnDefinitions>

<Border
Name="HighlightBorder"
Grid.ColumnSpan="4"
Margin="0,0,0,-1"
HorizontalAlignment="Stretch"
VerticalAlignment="Stretch"
Background="Transparent" />

<Grid
Grid.Column="0"
Margin="10,4,0,4"
HorizontalAlignment="Left">
<Grid.ColumnDefinitions>
<ColumnDefinition MinWidth="{x:Bind Indentation}" />
<ColumnDefinition Width="Auto" />
<ColumnDefinition Width="Auto" />
<ColumnDefinition Width="Auto" />
</Grid.ColumnDefinitions>


<CheckBox
Name="{x:Bind IdStr}"
Grid.Column="1"
Width="28"
Height="28"
MinWidth="0"
AutomationProperties.HelpText="{x:Bind Renamed}"
AutomationProperties.Name="{x:Bind Original}"
Content=""
IsChecked="{x:Bind Checked, Mode=TwoWay}"
IsTabStop="True"
TabIndex="0"
XYFocusKeyboardNavigation="Enabled" />
<Image
Grid.Column="2"
Width="16"
Margin="4,0,0,0"
HorizontalAlignment="Left"
Source="{x:Bind ImagePath}" />
<TextBlock
Grid.Column="3"
Margin="6,0,4,0"
VerticalAlignment="Center"
FontSize="12"
Foreground="{ThemeResource TextFillColorSecondaryBrush}"
Text="{x:Bind Original, Mode=OneWay}"
TextTrimming="CharacterEllipsis" />
</Grid>
<TextBlock
Grid.Column="2"
Margin="0,0,4,0"
VerticalAlignment="Center"
FontSize="12"
FontWeight="SemiBold"
Foreground="{ThemeResource AccentTextFillColorPrimaryBrush}"
Text="{x:Bind Renamed, Mode=OneWay}"
TextTrimming="CharacterEllipsis" />

<Border
x:Name="ErrorIcon"
Grid.Column="3"
Width="12"
Height="12"
CornerRadius="12"
Margin="4,0,8,0"
Background="{ThemeResource SystemFillColorCriticalBrush}"
Visibility="Collapsed">
<TextBlock
HorizontalAlignment="Center"
VerticalAlignment="Center"
FontFamily="{ThemeResource SymbolThemeFontFamily}"
Foreground="White"
FontSize="8"
Text="&#xE10A;" />
Comment on lines +78 to +93
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use glyph icons with the official brushes and symbols for the error icon in the muxc:InfoBar? This would fix the colors and make the x centered in the circle.

PowerToys/src/settings-ui/Settings.UI/Views/PowerLauncherPage.xaml

Line 284 in 24f4405

<!-- Error badge -->

Copy link
Collaborator

@Jay-o-Way Jay-o-Way Aug 16, 2022 •
edited

Choose a reason for hiding this comment

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

@niels9001 E10A is not allowed. You should know this by now. Please remember our agreement.
And please never hard-code a font color.

Copy link
Collaborator

@htcfreek htcfreek Aug 17, 2022 •
edited

Choose a reason for hiding this comment

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

@stefansjfw
Did you see my review?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, that's Q for @niels9001 :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@niels9001
Here is a list of all infobar theme resources: https://docs.microsoft.com/en-us/windows/winui/api/microsoft.ui.xaml.controls.infobar?view=winui-2.8#all-theme-resources

Copy link
Collaborator

@Jay-o-Way Jay-o-Way Aug 18, 2022 •
edited

Choose a reason for hiding this comment

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

@stefansjfw and @niels9001 - Using a <border> to achieve a colored background for an icon? Don't take it the wrong way, but I expect better from you guys (!) (try to find a compliment in there...)

Have a look at https://docs.microsoft.com/windows/apps/design/style/segoe-fluent-icons-font#layering-and-mirroring and let's use

<Grid>
    Icon="InfoBarIconBackgroundGlyph" Foreground="InfoBarErrorSeverityIconBackground" // red color
    Icon="InfoBarErrorIconGlyph Foreground="InfoBarErrorSeverityIconForeground"
</Grid>

Copy link
Collaborator

@htcfreek htcfreek Aug 18, 2022 •
edited

Choose a reason for hiding this comment

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

@Jay-o-Way
I agree with and that's why I wrote it.

And btw. we can use the background of the error bar (InfoBarErrorSeverityBackgroundBrush) as background for the table row. Then we are in line with system colors.

Copy link
Contributor

Choose a reason for hiding this comment

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

@niels9001
Here is a list of all infobar theme resources: https://docs.microsoft.com/en-us/windows/winui/api/microsoft.ui.xaml.controls.infobar?view=winui-2.8#all-theme-resources

The InfoBar brushes inherit other brushes... the one we use, so this is fine. See WinUI repo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the brushes I am okay with what we have. But can we use the X-Glyph-Resource from info bar. The current implementation you did isn't centered on the red background and this looks not good. (See screenshots in PR description.)

<ToolTipService.ToolTip>
<TextBlock
x:Name="ErrorMessageTxt"
Text="Error message goes here"
TextWrapping="Wrap" />
</ToolTipService.ToolTip>
</Border>

<VisualStateManager.VisualStateGroups>
<VisualStateGroup x:Name="CommonStates">
<VisualState x:Name="Normal" />
<VisualState x:Name="Highlight">
<VisualState.Setters>
<Setter Target="HighlightBorder.Opacity" Value="0.1" />
<Setter Target="HighlightBorder.BorderBrush" Value="{ThemeResource AccentTextFillColorPrimaryBrush}" />
<Setter Target="HighlightBorder.Background" Value="{ThemeResource AccentTextFillColorPrimaryBrush}" />
</VisualState.Setters>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct me if I'm mistaken, but I don't see a Icon visibility = none here. Isn't that needed to hide it when it's back to normal?

Copy link
Contributor

@niels9001 niels9001 Aug 19, 2022 •
edited

Choose a reason for hiding this comment

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

No, the closing tag indicates that the Normal state falls back to the default states defined in the XAML - in this case, the icon is already collapsed.

EDIT: Oh, going from Error -> Highlight state? @stefansjfw Is that a valid scenario or will it go through the Normal state first at all times? If not we might want to add the collapsing of the Error icon in the Highlight state as @Jay-o-Way mentioned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Error -> Highlight state works as expeected

</VisualState>
<VisualState x:Name="Error">
<VisualState.Setters>
<Setter Target="HighlightBorder.Opacity" Value="1" />
<Setter Target="HighlightBorder.BorderBrush" Value="{ThemeResource SystemFillColorCriticalBackgroundBrush}" />
<Setter Target="HighlightBorder.Background" Value="{ThemeResource SystemFillColorCriticalBackgroundBrush}" />
<Setter Target="ErrorIcon.Visibility" Value="Visible" />
</VisualState.Setters>
</VisualState>
</VisualStateGroup>
</VisualStateManager.VisualStateGroups>
</Grid>
</UserControl>
4 changes: 0 additions & 4 deletions src/modules/powerrename/PowerRenameUILib/MainWindow.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,5 @@ namespace PowerRenameUI

String OriginalCount;
String RenamedCount;

void AddExplorerItem(Int32 id, String original, String renamed, Int32 type, UInt32 depth, Boolean checked);
void UpdateExplorerItem(Int32 id, String newName);
void UpdateRenamedExplorerItem(Int32 id, String newOriginalName);
}
}