Skip to content

Commit 4d3c223

Browse files
jiripolasekjiripolasek
authored
CmdPal: Fix grid views (#43991)
## Summary of the Pull Request This PR fixes the crash due to binding to a trimmed property. For this it converts runtime bindings on GridView to use `{x:Bind}` so this issue can't happen in the future. - Fixes a crash related to the `Visibility` property in gallery/grid views when trimmed during AOT builds. - Fixes ShowTitle and ShowSubtitle properties, they are now taken into account in a view. - Improves UI layout, removes some margins and maches the corner radius of the item contaienr with the item content in the gallery view. - Refactores gallery and grid views to move logic from the view to the view model so we can x:Bind to them. - Replaces `{Binding}` with `{x:Bind}` to improve performance and enable compile-time binding validation. - Properties related to grids are splatted on to the common `IGridPropertiesViewModel` interface. Subclassing would add extra overhead without substential benefit. - Adds new samples to showcase various grid view configurations. ## Pictures? Pictures! A) Gallery view (with title and subtitle) <img width="909" height="583" alt="image" src="https://github.com/user-attachments/assets/b807e7a8-412f-4817-8121-e3470c49e0c0" /> B) Gallery view (only title) <img width="903" height="582" alt="image" src="https://github.com/user-attachments/assets/b619d63f-04d0-42f2-9207-de256dc5e481" /> C) Gallery view (no title or subtitle) <img width="900" height="583" alt="image" src="https://github.com/user-attachments/assets/c48cd1fc-8f51-40c1-8bce-607916e9f742" /> D) Small icons <img width="907" height="582" alt="image" src="https://github.com/user-attachments/assets/8327da0a-fa45-443f-b52c-f0f1edd7b861" /> E) Medium icons (with labels) <img width="914" height="588" alt="image" src="https://github.com/user-attachments/assets/dee9fab1-54e8-45f8-96d7-502b121a6ac2" /> F) Medium icons (no labels) <img width="915" height="588" alt="image" src="https://github.com/user-attachments/assets/a32e8af2-6cb1-4106-91db-ca396253c0a3" /> <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist - [x] Closes: #43973 <!-- - [ ] Closes: #yyy (add separate lines for additional resolved issues) --> - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [ ] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx <!-- Provide a more detailed description of the PR, other things fixed, or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed
1 parent 1ba5a25 commit 4d3c223

File tree

13 files changed

+404
-115
lines changed

13 files changed

+404
-115
lines changed

‎src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/GalleryGridPropertiesViewModel.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,15 @@ public class GalleryGridPropertiesViewModel : IGridPropertiesViewModel
1111
{
1212
private readonly ExtensionObject<IGalleryGridLayout> _model;
1313

14+
public bool ShowTitle { get; private set; }
15+
16+
public bool ShowSubtitle { get; private set; }
17+
1418
public GalleryGridPropertiesViewModel(IGalleryGridLayout galleryGridLayout)
1519
{
1620
_model = new(galleryGridLayout);
1721
}
1822

19-
public bool ShowTitle { get; set; }
20-
21-
public bool ShowSubtitle { get; set; }
22-
2323
public void InitializeProperties()
2424
{
2525
var model = _model.Unsafe;

‎src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/IGridPropertiesViewModel.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,5 +6,9 @@ namespace Microsoft.CmdPal.Core.ViewModels;
66

77
public interface IGridPropertiesViewModel
88
{
9+
bool ShowTitle { get; }
10+
11+
bool ShowSubtitle { get; }
12+
913
void InitializeProperties();
1014
}

‎src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/ListItemViewModel.cs‎

Lines changed: 80 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,9 @@
1010

1111
namespace Microsoft.CmdPal.Core.ViewModels;
1212

13-
public partial class ListItemViewModel(IListItem model, WeakReference<IPageContext> context)
14-
: CommandItemViewModel(new(model), context)
13+
public partial class ListItemViewModel : CommandItemViewModel
1514
{
16-
public new ExtensionObject<IListItem> Model { get; } = new(model);
15+
public new ExtensionObject<IListItem> Model { get; }
1716

1817
public List<TagViewModel>? Tags { get; set; }
1918

@@ -32,6 +31,40 @@ public partial class ListItemViewModel(IListItem model, WeakReference<IPageConte
3231

3332
public string AccessibleName { get; private set; } = string.Empty;
3433

34+
public bool ShowTitle { get; private set; }
35+
36+
public bool ShowSubtitle { get; private set; }
37+
38+
public bool LayoutShowsTitle
39+
{
40+
get;
41+
set
42+
{
43+
if (SetProperty(ref field, value))
44+
{
45+
UpdateShowsTitle();
46+
}
47+
}
48+
}
49+
50+
public bool LayoutShowsSubtitle
51+
{
52+
get;
53+
set
54+
{
55+
if (SetProperty(ref field, value))
56+
{
57+
UpdateShowsSubtitle();
58+
}
59+
}
60+
}
61+
62+
public ListItemViewModel(IListItem model, WeakReference<IPageContext> context)
63+
: base(new(model), context)
64+
{
65+
Model = new ExtensionObject<IListItem>(model);
66+
}
67+
3568
public override void InitializeProperties()
3669
{
3770
if (IsInitialized)
@@ -93,33 +126,43 @@ protected override void FetchProperty(string propertyName)
93126

94127
switch (propertyName)
95128
{
96-
case nameof(Tags):
129+
case nameof(model.Tags):
97130
UpdateTags(model.Tags);
98131
break;
99-
case nameof(TextToSuggest):
100-
this.TextToSuggest = model.TextToSuggest ?? string.Empty;
132+
case nameof(model.TextToSuggest):
133+
TextToSuggest = model.TextToSuggest ?? string.Empty;
134+
UpdateProperty(nameof(TextToSuggest));
101135
break;
102-
case nameof(Section):
103-
this.Section = model.Section ?? string.Empty;
136+
case nameof(model.Section):
137+
Section = model.Section ?? string.Empty;
138+
UpdateProperty(nameof(Section));
104139
break;
105-
case nameof(Details):
140+
case nameof(model.Details):
106141
var extensionDetails = model.Details;
107142
Details = extensionDetails is not null ? new(extensionDetails, PageContext) : null;
108143
Details?.InitializeProperties();
109144
UpdateProperty(nameof(Details));
110145
UpdateProperty(nameof(HasDetails));
111146
UpdateShowDetailsCommand();
112147
break;
113-
case nameof(MoreCommands):
148+
case nameof(model.MoreCommands):
149+
UpdateProperty(nameof(MoreCommands));
114150
AddShowDetailsCommands();
115151
break;
116-
case nameof(Title):
117-
case nameof(Subtitle):
152+
case nameof(model.Title):
153+
UpdateProperty(nameof(Title));
154+
UpdateShowsTitle();
155+
UpdateAccessibleName();
156+
break;
157+
case nameof(model.Subtitle):
158+
UpdateProperty(nameof(Subtitle));
159+
UpdateShowsSubtitle();
118160
UpdateAccessibleName();
119161
break;
162+
default:
163+
UpdateProperty(propertyName);
164+
break;
120165
}
121-
122-
UpdateProperty(propertyName);
123166
}
124167

125168
// TODO: Do we want filters to match descriptions and other properties? Tags, etc... Yes?
@@ -206,11 +249,32 @@ private void UpdateTags(ITag[]? newTagsFromModel)
206249
// many COM exception issues.
207250
Tags = [.. newTags];
208251

209-
UpdateProperty(nameof(Tags));
210-
UpdateProperty(nameof(HasTags));
252+
// We're already in UI thread, so just raise the events
253+
OnPropertyChanged(nameof(Tags));
254+
OnPropertyChanged(nameof(HasTags));
211255
});
212256
}
213257

258+
private void UpdateShowsTitle()
259+
{
260+
var oldShowTitle = ShowTitle;
261+
ShowTitle = LayoutShowsTitle;
262+
if (oldShowTitle != ShowTitle)
263+
{
264+
UpdateProperty(nameof(ShowTitle));
265+
}
266+
}
267+
268+
private void UpdateShowsSubtitle()
269+
{
270+
var oldShowSubtitle = ShowSubtitle;
271+
ShowSubtitle = LayoutShowsSubtitle && !string.IsNullOrWhiteSpace(Subtitle);
272+
if (oldShowSubtitle != ShowSubtitle)
273+
{
274+
UpdateProperty(nameof(ShowSubtitle));
275+
}
276+
}
277+
214278
protected override void UnsafeCleanup()
215279
{
216280
base.UnsafeCleanup();

‎src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/ListViewModel.cs‎

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,7 @@ public partial class ListViewModel : PageViewModel, IDisposable
2424

2525
// Observable from MVVM Toolkit will auto create public properties that use INotifyPropertyChange change
2626
// https://learn.microsoft.com/dotnet/communitytoolkit/mvvm/observablegroupedcollections for grouping support
27-
[ObservableProperty]
28-
public partial ObservableCollection<ListItemViewModel> FilteredItems { get; set; } = [];
27+
public ObservableCollection<ListItemViewModel> FilteredItems { get; } = [];
2928

3029
public FiltersViewModel? Filters { get; set; }
3130

@@ -224,6 +223,8 @@ private void FetchItems()
224223
// TODO we can probably further optimize this by also keeping a
225224
// HashSet of every ExtensionObject we currently have, and only
226225
// building new viewmodels for the ones we haven't already built.
226+
var showsTitle = GridProperties?.ShowTitle ?? true;
227+
var showsSubtitle = GridProperties?.ShowSubtitle ?? true;
227228
foreach (var item in newItems)
228229
{
229230
// Check for cancellation during item processing
@@ -237,6 +238,8 @@ private void FetchItems()
237238
// If an item fails to load, silently ignore it.
238239
if (viewModel.SafeFastInit())
239240
{
241+
viewModel.LayoutShowsTitle = showsTitle;
242+
viewModel.LayoutShowsSubtitle = showsSubtitle;
240243
newViewModels.Add(viewModel);
241244
}
242245
}
@@ -583,6 +586,7 @@ public override void InitializeProperties()
583586
GridProperties = LoadGridPropertiesViewModel(model.GridProperties);
584587
GridProperties?.InitializeProperties();
585588
UpdateProperty(nameof(GridProperties));
589+
ApplyLayoutToItems();
586590

587591
ShowDetails = model.ShowDetails;
588592
UpdateProperty(nameof(ShowDetails));
@@ -608,22 +612,15 @@ public override void InitializeProperties()
608612
model.ItemsChanged += Model_ItemsChanged;
609613
}
610614

611-
private IGridPropertiesViewModel? LoadGridPropertiesViewModel(IGridProperties? gridProperties)
615+
private static IGridPropertiesViewModel? LoadGridPropertiesViewModel(IGridProperties? gridProperties)
612616
{
613-
if (gridProperties is IMediumGridLayout mediumGridLayout)
617+
return gridProperties switch
614618
{
615-
return new MediumGridPropertiesViewModel(mediumGridLayout);
616-
}
617-
else if (gridProperties is IGalleryGridLayout galleryGridLayout)
618-
{
619-
return new GalleryGridPropertiesViewModel(galleryGridLayout);
620-
}
621-
else if (gridProperties is ISmallGridLayout smallGridLayout)
622-
{
623-
return new SmallGridPropertiesViewModel(smallGridLayout);
624-
}
625-
626-
return null;
619+
IMediumGridLayout mediumGridLayout => new MediumGridPropertiesViewModel(mediumGridLayout),
620+
IGalleryGridLayout galleryGridLayout => new GalleryGridPropertiesViewModel(galleryGridLayout),
621+
ISmallGridLayout smallGridLayout => new SmallGridPropertiesViewModel(smallGridLayout),
622+
_ => null,
623+
};
627624
}
628625

629626
public void LoadMoreIfNeeded()
@@ -685,6 +682,7 @@ protected override void FetchProperty(string propertyName)
685682
GridProperties = LoadGridPropertiesViewModel(model.GridProperties);
686683
GridProperties?.InitializeProperties();
687684
UpdateProperty(nameof(IsGridView));
685+
ApplyLayoutToItems();
688686
break;
689687
case nameof(ShowDetails):
690688
ShowDetails = model.ShowDetails;
@@ -730,6 +728,21 @@ private void UpdateEmptyContent()
730728
});
731729
}
732730

731+
private void ApplyLayoutToItems()
732+
{
733+
lock (_listLock)
734+
{
735+
var showsTitle = GridProperties?.ShowTitle ?? true;
736+
var showsSubtitle = GridProperties?.ShowSubtitle ?? true;
737+
738+
foreach (var item in Items)
739+
{
740+
item.LayoutShowsTitle = showsTitle;
741+
item.LayoutShowsSubtitle = showsSubtitle;
742+
}
743+
}
744+
}
745+
733746
public void Dispose()
734747
{
735748
GC.SuppressFinalize(this);

‎src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/MediumGridPropertiesViewModel.cs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,15 @@ public class MediumGridPropertiesViewModel : IGridPropertiesViewModel
1111
{
1212
private readonly ExtensionObject<IMediumGridLayout> _model;
1313

14+
public bool ShowTitle { get; private set; }
15+
16+
public bool ShowSubtitle => false;
17+
1418
public MediumGridPropertiesViewModel(IMediumGridLayout mediumGridLayout)
1519
{
1620
_model = new(mediumGridLayout);
1721
}
1822

19-
public bool ShowTitle { get; set; }
20-
2123
public void InitializeProperties()
2224
{
2325
var model = _model.Unsafe;

‎src/modules/cmdpal/Core/Microsoft.CmdPal.Core.ViewModels/SmallGridPropertiesViewModel.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,10 @@ public class SmallGridPropertiesViewModel : IGridPropertiesViewModel
1111
{
1212
private readonly ExtensionObject<ISmallGridLayout> _model;
1313

14+
public bool ShowTitle => false;
15+
16+
public bool ShowSubtitle => false;
17+
1418
public SmallGridPropertiesViewModel(ISmallGridLayout smallGridLayout)
1519
{
1620
_model = new(smallGridLayout);

‎src/modules/cmdpal/Microsoft.CmdPal.UI/Converters/GridItemContainerStyleSelector.cs‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// Copyright (c) Microsoft Corporation
2+
// The Microsoft Corporation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using Microsoft.CmdPal.Core.ViewModels;
6+
using Microsoft.UI.Xaml;
7+
using Microsoft.UI.Xaml.Controls;
8+
9+
namespace Microsoft.CmdPal.UI;
10+
11+
internal sealed partial class GridItemContainerStyleSelector : StyleSelector
12+
{
13+
public IGridPropertiesViewModel? GridProperties { get; set; }
14+
15+
public Style? Small { get; set; }
16+
17+
public Style? Medium { get; set; }
18+
19+
public Style? Gallery { get; set; }
20+
21+
protected override Style? SelectStyleCore(object item, DependencyObject container)
22+
{
23+
return GridProperties switch
24+
{
25+
SmallGridPropertiesViewModel => Small,
26+
MediumGridPropertiesViewModel => Medium,
27+
GalleryGridPropertiesViewModel => Gallery,
28+
_ => Medium,
29+
};
30+
}
31+
}

‎src/modules/cmdpal/Microsoft.CmdPal.UI/Converters/GridItemTemplateSelector.cs‎

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,21 +20,12 @@ internal sealed partial class GridItemTemplateSelector : DataTemplateSelector
2020

2121
protected override DataTemplate? SelectTemplateCore(object item, DependencyObject dependencyObject)
2222
{
23-
DataTemplate? dataTemplate = Medium;
24-
25-
if (GridProperties is SmallGridPropertiesViewModel)
26-
{
27-
dataTemplate = Small;
28-
}
29-
else if (GridProperties is MediumGridPropertiesViewModel)
23+
return GridProperties switch
3024
{
31-
dataTemplate = Medium;
32-
}
33-
else if (GridProperties is GalleryGridPropertiesViewModel)
34-
{
35-
dataTemplate = Gallery;
36-
}
37-
38-
return dataTemplate;
25+
SmallGridPropertiesViewModel => Small,
26+
MediumGridPropertiesViewModel => Medium,
27+
GalleryGridPropertiesViewModel => Gallery,
28+
_ => Medium,
29+
};
3930
}
4031
}

0 commit comments

Comments
 (0)