Feature/multiple selection 407#740
Conversation
srwi
left a comment
srwi
left a comment
There was a problem hiding this comment.
Hi! Thank you for the pull request. You definitely picked one of the more complicated ones. 🙂
I think this is a very good start and it will be a great addition to EverythingToolbar, especially for touch screen users. I did some testing and it mostly works. However, there are a few improvements I suggested in the comments. I think it also needs a lot of testing, so things might pop up later. The feature itself seems so simple, but even while reviewing it I was reminded why I had been postponing it for so long. 😅
By the way I may not always respond immediately as I'm a little more busy with the start of summer.
| Content=".*" | ||
| Margin="1" /> | ||
|
|
||
| <ToggleButton Style="{DynamicResource QuickSettingsToggleButton}" |
There was a problem hiding this comment.
I think placing the toggle button in the optional quick actions bar is not a good idea. Its visibility depends on whether the quick action are shown or not.
In general I am not sure about having a toggle button for this. Isn't the typical flow in mobile apps to long press any item to switch to a multi-select mode where the user can tap multiple items to select or unselect and then press the back button to cancel the selection?
My suggestion would be to switch to the multi-selection mode by long pressing the first item on touch devices. While this mode is active, show a "Unselect all" button in the main toolbar to the left of the sort by button which unselects all items and switches off the multi-select mode. For regular desktop mode I don't think any UI changes are needed and regular multiselect can be used.
What do you think?
| <Style TargetType="{x:Type ListView}"> | ||
| <Setter Property="SelectionMode" Value="Extended" /> | ||
| <Style.Triggers> | ||
| <DataTrigger Binding="{Binding Source={x:Static local:ToolbarSettings.User}, Path=IsSelectionModeEnabled}" Value="True"> |
There was a problem hiding this comment.
I think semantically it would make more sense to make this a property of the SearchResultsView instead of a setting since this is not a user setting but rather a runtime value. You should still be able to bind to it.
|
|
||
| private void OnPreviewLeftMouseButtonDown(object sender, MouseButtonEventArgs e) | ||
| { | ||
| // Prevents deselecting an item when Ctrl is held down and clicking on an already selected item |
There was a problem hiding this comment.
These comments were probably removed by accident? It happened at multiple places throughout the file.
| if (first == null) return; | ||
|
|
||
| SearchResultProvider.OpenSearchInEverything(SearchState.Instance, SelectedItem.FullPathAndFileName); | ||
| SearchResultProvider.OpenSearchInEverything(SearchState.Instance, first.FullPathAndFileName); |
There was a problem hiding this comment.
I am not sure Everything can be launched with multiple files selected and launching with the first selected file seems a little arbitrary. I think the best approach would be to disable this feature while multiple files are selected, since it doesn't seem to be a supported feature of Everything. This would require disabling the context menu entry as well so it is consistent in the UI.
| } | ||
| else if (Keyboard.Modifiers == (ModifierKeys.Control | ModifierKeys.Shift) && e.Key == Key.C) | ||
| { | ||
| SelectedItem?.CopyPathToClipboard(); |
There was a problem hiding this comment.
The two methods CopyPathToClipboard and CopyToClipboard are now not used anymore and their implementations can be removed.
| SelectedItem?.CopyToClipboard(); | ||
| var paths = new StringCollection(); | ||
| foreach (var item in GetSelectedItems()) paths.Add(item.FullPathAndFileName); | ||
| if (paths.Count > 0) Clipboard.SetFileDropList(paths); |
| if (ToolbarSettings.User.IsSelectionModeEnabled) | ||
| return; | ||
|
|
||
| if (Keyboard.Modifiers.HasFlag(ModifierKeys.Control) || Keyboard.Modifiers.HasFlag(ModifierKeys.Shift)) |
There was a problem hiding this comment.
Why is the check for modifiers needed here?
| SelectedItem?.ShowProperties(); | ||
| SearchWindow.Instance.Hide(); | ||
| break; | ||
| case ModifierKeys.Control: |
| @@ -286,9 +286,22 @@ public void ShowProperties() | |||
|
|
|||
| public void ShowWindowsContextMenu() | |||
There was a problem hiding this comment.
This method is not used anymore and can be removed.
| <ColumnDefinition Width="Auto" /> | ||
| </Grid.ColumnDefinitions> | ||
|
|
||
| <Grid x:Name="CheckBoxColumn" Width="0" ClipToBounds="True"> |
There was a problem hiding this comment.
The styling of the checkboxes currently doesn't match the light/dark themes and Windows 10/11 themes.
In ui settings the template uses the correct WinUI themes (although cut off):
I remember it was a little tricky to use WinUI in the search window without a lot of rework. Therefore it may be necessary to create own styles for this checkbox that can work nicely with the different item templates under Windows 10 and 11. Here is a screenshot of windows settings:
2 participants
Added multiple selection via CTRL and via button for touch displays.
Context window for actions available only for files in the same parent directory due to WIN API limitations.