Skip to content

Code Quality: Removed ISidebarViewModel #17972

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

Open
0x5bfa wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Code Quality: Removed ISidebarViewModel #17972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0c7408a
Update the UI automation classes
0x5bfa Dec 18, 2025
314d8a3
Removed ISidebarViewModel
0x5bfa Dec 18, 2025
8cbdfc4
Revert the DP changes
0x5bfa Dec 19, 2025
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
37 changes: 1 addition & 36 deletions src/Files.App.Controls/Sidebar/ISidebarViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,43 +7,8 @@

namespace Files.App.Controls
{
public record ItemInvokedEventArgs(PointerUpdateKind PointerUpdateKind) { }
public record ItemDroppedEventArgs(object DropTarget, DataPackageView DroppedItem, SidebarItemDropPosition dropPosition, DragEventArgs RawEvent) { }
public record ItemDragOverEventArgs(object DropTarget, DataPackageView DroppedItem, SidebarItemDropPosition dropPosition, DragEventArgs RawEvent) { }
public record ItemContextInvokedArgs(object? Item, Point Position) { }

public interface ISidebarViewModel
{
/// <summary>
/// The source/list of items that will be rendered in the sidebar
/// </summary>
object SidebarItems { get; }
Copy link
Contributor

@marcelwgn marcelwgn Dec 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why didnt we already remove this as part of #15594?
Why did we even need add the MenuItems property for that fix if we just bound to the same thing at the end of the day?

Copy link
Member Author

@0x5bfa 0x5bfa Dec 20, 2025

Choose a reason for hiding this comment

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

This was used also in the SidebarView's automation peer class (which I changed in this PR).
I don't remember but no reason to delete it in that PR.

Copy link
Contributor

@marcelwgn marcelwgn Dec 20, 2025

Choose a reason for hiding this comment

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

In that PR all of those should have switched away from that property, having two truths which elements we present is really strange. Regardless, I guess we get rid of it now.


/// <summary>
/// Gets invoked when the context was requested for an item in the sidebar.
/// Also applies when context was requested for the pane itsself.
/// </summary>
/// <param name="sender">The sender of this event</param>
/// <param name="args">The <see cref="ItemContextInvokedArgs"/> for this event.</param>
void HandleItemContextInvokedAsync(object sender, ItemContextInvokedArgs args);

/// <summary>
/// Gets invoked when an item drags over any item of the sidebar.
/// </summary>
/// <param name="args">The <see cref="ItemDragOverEventArgs"/> for this event.</param>
/// <returns>A <see cref="Task"/> that represents the asynchronous operation.</returns>
Task HandleItemDragOverAsync(ItemDragOverEventArgs args);

/// <summary>
/// Gets invoked when an item is dropped on any item of the sidebar.
/// </summary>
/// <param name="args">The <see cref="ItemDroppedEventArgs"/> for this event.</param>
/// <returns>A <see cref="Task"/> that represents the asynchronous operation.</returns>
Task HandleItemDroppedAsync(ItemDroppedEventArgs args);

/// <summary>
/// Gets invoked when an item is invoked (double clicked) on any item of the sidebar.
/// </summary>
/// <param name="item">The item that was invoked.</param>
void HandleItemInvokedAsync(object item, PointerUpdateKind pointerUpdateKind);
}
}
12 changes: 3 additions & 9 deletions src/Files.App.Controls/Sidebar/SidebarItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,12 +404,7 @@ private async void ItemBorder_DragOver(object sender, DragEventArgs e)
VisualStateManager.GoToState(this, "DragInsertBelow", true);
}

if (Owner is not null)
{
var deferral = e.GetDeferral();
await Owner.RaiseItemDragOver(this, insertsAbove, e);
deferral.Complete();
}
Owner?.RaiseItemDragOver(this, insertsAbove, e);
}

private void ItemBorder_ContextRequested(UIElement sender, Microsoft.UI.Xaml.Input.ContextRequestedEventArgs args)
Expand All @@ -423,11 +418,10 @@ private void ItemBorder_DragLeave(object sender, DragEventArgs e)
UpdatePointerState();
}

private async void ItemBorder_Drop(object sender, DragEventArgs e)
private void ItemBorder_Drop(object sender, DragEventArgs e)
{
UpdatePointerState();
if (Owner is not null)
await Owner.RaiseItemDropped(this, DetermineDropTargetPosition(e), e);
Owner?.RaiseItemDropped(this, DetermineDropTargetPosition(e), e);
}

private SidebarItemDropPosition DetermineDropTargetPosition(DragEventArgs args)
Expand Down
22 changes: 9 additions & 13 deletions src/Files.App.Controls/Sidebar/SidebarItemAutomationPeer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@

namespace Files.App.Controls
{
/// <summary>
/// Exposes <see cref="SidebarItem"/> types to Microsoft UI Automation.
/// </summary>
public sealed partial class SidebarItemAutomationPeer : FrameworkElementAutomationPeer, IInvokeProvider, IExpandCollapseProvider, ISelectionItemProvider
{
public ExpandCollapseState ExpandCollapseState
Expand All @@ -17,6 +20,7 @@ public ExpandCollapseState ExpandCollapseState
{
if (Owner.HasChildren)
return Owner.IsExpanded ? ExpandCollapseState.Expanded : ExpandCollapseState.Collapsed;

return ExpandCollapseState.LeafNode;
}
}
Expand All @@ -27,7 +31,7 @@ public ExpandCollapseState ExpandCollapseState

public SidebarItemAutomationPeer(SidebarItem owner) : base(owner)
{
this.Owner = owner;
Owner = owner;
}

protected override AutomationControlType GetAutomationControlTypeCore()
Expand All @@ -49,28 +53,22 @@ protected override object GetPatternCore(PatternInterface patternInterface)
else if (patternInterface == PatternInterface.ExpandCollapse)
{
if (Owner.CollapseEnabled)
{
return this;
}
}

return base.GetPatternCore(patternInterface);
}

public void Collapse()
{
if (Owner.CollapseEnabled)
{
Owner.IsExpanded = false;
}
}

public void Expand()
{

if (Owner.CollapseEnabled)
{
Owner.IsExpanded = true;
}
}

public void Invoke()
Expand Down Expand Up @@ -106,13 +104,11 @@ protected override int GetPositionInSetCore()
private IList GetOwnerCollection()
{
if (Owner.FindAscendant<SidebarItem>() is SidebarItem parent && parent.Item?.Children is IList list)
{
return list;
}
if (Owner?.Owner is not null && Owner.Owner.ViewModel.SidebarItems is IList items)
{

if (Owner?.Owner is not null && Owner.Owner?.MenuItemsSource is IList items)
return items;
}

return new List<object>();
}
}
Expand Down
8 changes: 0 additions & 8 deletions src/Files.App.Controls/Sidebar/SidebarView.Properties.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,6 @@ public double NegativeOpenPaneLength
public static readonly DependencyProperty NegativeOpenPaneLengthProperty =
DependencyProperty.Register(nameof(NegativeOpenPaneLength), typeof(double), typeof(SidebarView), new PropertyMetadata(null));

public ISidebarViewModel ViewModel
{
get => (ISidebarViewModel)GetValue(ViewModelProperty);
set => SetValue(ViewModelProperty, value);
}
public static readonly DependencyProperty ViewModelProperty =
DependencyProperty.Register(nameof(ViewModel), typeof(ISidebarViewModel), typeof(SidebarView), new PropertyMetadata(null));

public ISidebarItemModel SelectedItem
{
get => (ISidebarItemModel)GetValue(SelectedItemProperty);
Expand Down
21 changes: 10 additions & 11 deletions src/Files.App.Controls/Sidebar/SidebarView.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ public sealed partial class SidebarView : UserControl, INotifyPropertyChanged
{
private const double COMPACT_MAX_WIDTH = 200;

public event EventHandler<object>? ItemInvoked;
public event EventHandler<ItemInvokedEventArgs>? ItemInvoked;
public event EventHandler<ItemContextInvokedArgs>? ItemContextInvoked;
public event EventHandler<ItemDragOverEventArgs>? ItemDragOver;
public event EventHandler<ItemDroppedEventArgs>? ItemDropped;
public event PropertyChangedEventHandler? PropertyChanged;

internal SidebarItem? SelectedItemContainer = null;
Expand All @@ -40,26 +42,24 @@ internal void RaiseItemInvoked(SidebarItem item, PointerUpdateKind pointerUpdate
if (item.Item is null || item.IsGroupHeader) return;

SelectedItem = item.Item;
ItemInvoked?.Invoke(item, item.Item);
ViewModel.HandleItemInvokedAsync(item.Item, pointerUpdateKind);
ItemInvoked?.Invoke(item, new(pointerUpdateKind));
}

internal void RaiseContextRequested(SidebarItem item, Point e)
{
ItemContextInvoked?.Invoke(item, new ItemContextInvokedArgs(item.Item, e));
ViewModel.HandleItemContextInvokedAsync(item, new ItemContextInvokedArgs(item.Item, e));
ItemContextInvoked?.Invoke(item, new(item.Item, e));
}

internal async Task RaiseItemDropped(SidebarItem sideBarItem, SidebarItemDropPosition dropPosition, DragEventArgs rawEvent)
internal void RaiseItemDropped(SidebarItem sideBarItem, SidebarItemDropPosition dropPosition, DragEventArgs rawEvent)
{
if (sideBarItem.Item is null) return;
await ViewModel.HandleItemDroppedAsync(new ItemDroppedEventArgs(sideBarItem.Item, rawEvent.DataView, dropPosition, rawEvent));
ItemDropped?.Invoke(this, new(sideBarItem.Item, rawEvent.DataView, dropPosition, rawEvent));
}

internal async Task RaiseItemDragOver(SidebarItem sideBarItem, SidebarItemDropPosition dropPosition, DragEventArgs rawEvent)
internal void RaiseItemDragOver(SidebarItem sideBarItem, SidebarItemDropPosition dropPosition, DragEventArgs rawEvent)
{
if (sideBarItem.Item is null) return;
await ViewModel.HandleItemDragOverAsync(new ItemDragOverEventArgs(sideBarItem.Item, rawEvent.DataView, dropPosition, rawEvent));
ItemDragOver?.Invoke(this, new(sideBarItem.Item, rawEvent.DataView, dropPosition, rawEvent));
}

private void UpdateMinimalMode()
Expand Down Expand Up @@ -230,8 +230,7 @@ private void SidebarResizer_ManipulationCompleted(object sender, ManipulationCom

private void MenuItemHostScrollViewer_ContextRequested(UIElement sender, ContextRequestedEventArgs e)
{
var newArgs = new ItemContextInvokedArgs(null, e.TryGetPosition(this, out var point) ? point : default);
ViewModel.HandleItemContextInvokedAsync(this, newArgs);
ItemContextInvoked?.Invoke(this, new(null, e.TryGetPosition(this, out var point) ? point : default));
e.Handled = true;
}

Expand Down
14 changes: 7 additions & 7 deletions src/Files.App.Controls/Sidebar/SidebarViewAutomationPeer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

namespace Files.App.Controls
{
/// <summary>
/// Exposes <see cref="SidebarView"/> types to Microsoft UI Automation.
/// </summary>
public sealed partial class SidebarViewAutomationPeer : FrameworkElementAutomationPeer, ISelectionProvider
{
public bool CanSelectMultiple => false;
Expand All @@ -20,20 +23,17 @@ public SidebarViewAutomationPeer(SidebarView owner) : base(owner)

protected override object GetPatternCore(PatternInterface patternInterface)
{
if (patternInterface == PatternInterface.Selection)
{
if (patternInterface is PatternInterface.Selection)
return this;
}

return base.GetPatternCore(patternInterface);
}

public IRawElementProviderSimple[] GetSelection()
{
if (Owner.SelectedItemContainer != null)
return
[
ProviderFromPeer(CreatePeerForElement(Owner.SelectedItemContainer))
];
return [ProviderFromPeer(CreatePeerForElement(Owner.SelectedItemContainer))];

return [];
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Files.App/ViewModels/UserControls/SidebarViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

namespace Files.App.ViewModels.UserControls
{
public sealed partial class SidebarViewModel : ObservableObject, IDisposable, ISidebarViewModel
public sealed partial class SidebarViewModel : ObservableObject, IDisposable
{
private INetworkService NetworkService { get; } = Ioc.Default.GetRequiredService<INetworkService>();
private IUserSettingsService UserSettingsService { get; } = Ioc.Default.GetRequiredService<IUserSettingsService>();
Expand Down
7 changes: 5 additions & 2 deletions src/Files.App/Views/MainPage.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,14 @@
HorizontalContentAlignment="Stretch"
DisplayMode="{x:Bind SidebarAdaptiveViewModel.SidebarDisplayMode, Mode=TwoWay}"
IsPaneOpen="{x:Bind ViewModel.IsSidebarPaneOpen, Mode=TwoWay}"
ItemContextInvoked="SidebarControl_ItemContextInvoked"
ItemDragOver="SidebarControl_ItemDragOver"
ItemDropped="SidebarControl_ItemDropped"
ItemInvoked="SidebarControl_ItemInvoked"
Loaded="SidebarControl_Loaded"
MenuItemsSource="{x:Bind SidebarAdaptiveViewModel.SidebarItems, Mode=OneWay}"
OpenPaneLength="{x:Bind UserSettingsService.AppearanceSettingsService.SidebarWidth, Mode=TwoWay}"
SelectedItem="{x:Bind SidebarAdaptiveViewModel.SidebarSelectedItem, Mode=TwoWay}"
ViewModel="{x:Bind SidebarAdaptiveViewModel}">
SelectedItem="{x:Bind SidebarAdaptiveViewModel.SidebarSelectedItem, Mode=TwoWay}">

<!-- Inner Content -->
<controls:SidebarView.InnerContent>
Expand Down
23 changes: 23 additions & 0 deletions src/Files.App/Views/MainPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using Microsoft.UI.Xaml.Navigation;
using Windows.Foundation.Metadata;
using Windows.Graphics;
using Windows.UI.Input;
using WinUIEx;
using GridSplitter = Files.App.Controls.GridSplitter;
using VirtualKey = Windows.System.VirtualKey;
Expand Down Expand Up @@ -465,5 +466,27 @@ private void Page_PointerReleased(object sender, PointerRoutedEventArgs e)
// shortcuts from working properly, see https://github.com/microsoft/microsoft-ui-xaml/issues/6467
DispatcherQueue.TryEnqueue(() => ContentPageContext.ShellPage?.PaneHolder.FocusActivePane());
}

private void SidebarControl_ItemContextInvoked(object sender, ItemContextInvokedArgs e)
{
SidebarAdaptiveViewModel.HandleItemContextInvokedAsync(sender, e);
}

private async void SidebarControl_ItemDragOver(object sender, ItemDragOverEventArgs e)
{
var deferral = e.RawEvent.GetDeferral();
await SidebarAdaptiveViewModel.HandleItemDragOverAsync(e);
deferral.Complete();
}

private async void SidebarControl_ItemDropped(object sender, ItemDroppedEventArgs e)
{
await SidebarAdaptiveViewModel.HandleItemDroppedAsync(e);
}
Comment on lines +482 to +485
Copy link

@sentry sentry bot Dec 19, 2025

Choose a reason for hiding this comment

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

Bug: The async event handler SidebarControl_ItemDropped is missing a required deferral, which can cause the drag operation's data to become invalid before async work finishes.
Severity: HIGH | Confidence: High

🔍 Detailed Analysis

The SidebarControl_ItemDropped event handler is an async method that performs significant asynchronous work, such as file operations and database updates, via SidebarAdaptiveViewModel.HandleItemDroppedAsync. However, it fails to acquire a deferral from the ItemDroppedEventArgs. In the WinUI drag-and-drop API, a deferral is necessary to prevent the operating system from cleaning up the drag operation's data context before the asynchronous work completes. Without it, the DataPackageView could be invalidated mid-operation, leading to failed drop actions.

💡 Suggested Fix

In the SidebarControl_ItemDropped method, acquire a deferral from the event arguments before the await call and complete it after the operation is finished. Example: var deferral = e.RawEvent.GetDeferral(); await SidebarAdaptiveViewModel.HandleItemDroppedAsync(e); deferral.Complete();

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/Files.App/Views/MainPage.xaml.cs#L482-L485

Potential issue: The `SidebarControl_ItemDropped` event handler is an `async` method
that performs significant asynchronous work, such as file operations and database
updates, via `SidebarAdaptiveViewModel.HandleItemDroppedAsync`. However, it fails to
acquire a deferral from the `ItemDroppedEventArgs`. In the WinUI drag-and-drop API, a
deferral is necessary to prevent the operating system from cleaning up the drag
operation's data context before the asynchronous work completes. Without it, the
`DataPackageView` could be invalidated mid-operation, leading to failed drop actions.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7750491

Copy link
Member Author

@0x5bfa 0x5bfa Dec 19, 2025

Choose a reason for hiding this comment

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

I moved this code from here, where there is no deferral code.

image

Copy link
Contributor

@marcelwgn marcelwgn Dec 20, 2025

Choose a reason for hiding this comment

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

I would argue its a bug that we didnt do that before then (though its unlikely) it might cause an issue, since we have no idea what might be done in the event handler.


private void SidebarControl_ItemInvoked(object sender, ItemInvokedEventArgs e)
{
SidebarAdaptiveViewModel.HandleItemInvokedAsync(((SidebarItem)sender).Item, e.PointerUpdateKind);
}
}
}
30 changes: 0 additions & 30 deletions tests/Files.App.UITests/TestData/TestSidebarViewModel.cs
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions tests/Files.App.UITests/Views/SidebarViewPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ public SidebarViewPage()
sidebarModels.Add(new TestSidebarModel { Text = "Test 1" });
sidebarModels.Add(new TestSidebarModel { Text = "Test 2" });
sidebarModels.Add(new TestSidebarModel { Text = "Test 3" });

Sidebar.ViewModel = new TestSidebarViewModel();
}
}
}
Loading