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

Tree view multiple selection support #2347

Merged
merged 7 commits into from
Mar 8, 2019

Conversation

MarchingCube
Copy link
Collaborator

@MarchingCube MarchingCube commented Mar 5, 2019

What does the pull request do?

Implements multiple selection behavior for TreeView using SelectingItemsControl implementation as a baseline.

What is the current behavior?

TreeView does not support multiple selection at all. Also operations like toggle are not supported, so it was impossible to deselect an item using Control modifier.

What is the updated/expected behavior with this PR?

Enabled the multiple selection on TreeViewPage, so user can now select multiple elements.

tree-view-simple

How was the solution implemented (if it's not obvious)?

I couldn't really use SelectingItemsControl due to index requirements (and TreeView does not use contiguous item collection), so I had to port it to work for this case.
I did not implement AlwaysSelected yet, can add in followup if needed. Also similar to SelectingItemsControl switching SelectionMode at runtime does not change current selection.

Checklist

Breaking changes

It replaces SelectedItemChanged event with SelectionChanged event, which uses the same event args, but semantically means that there could be multiple items that got selected/deselected.

Fixed issues

cc: @grokys

@MarchingCube MarchingCube marked this pull request as ready for review March 6, 2019 13:46
Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

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

Seems to work great in my testing! Just a few nits really.

/// <summary>
/// Node search info.
/// </summary>
public readonly struct SearchInfo
Copy link
Member

Choose a reason for hiding this comment

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

Why encapsulate these two TreeViewItem references and the GetMatch method in a struct? I would seem to be that you could just pass them as parameters to the relevant methods.

src/Avalonia.Controls/TreeViewHelper.cs Outdated Show resolved Hide resolved
@@ -639,7 +639,7 @@ private static IEnumerable<object> GetRange(IEnumerable items, int first, int la
/// </summary>
/// <param name="items">The items collection.</param>
/// <param name="desired">The desired items.</param>
private static void SynchronizeItems(IList items, IEnumerable<object> desired)
internal static void SynchronizeItems(IList items, IEnumerable<object> desired)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: now that this is internal, could you move it up above the private methods?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was not sure where to put it, do you follow any file layout guidelines in Avalonia?

Copy link
Member

@grokys grokys Mar 7, 2019

Choose a reason for hiding this comment

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

It's not a strict ordering, but I usually use the order public, protected, internal, private, i.e. from most public to least public.

I know there are many places where we don't follow that, mostly because of edits. However, if we can keep it tidy i'd prefer it and an internal modifier between private methods just sets my senses tingling ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see, at work I use the R# file layout that is automatically applied on their cleanup reformat. Makes life a bit easier when you don't have to think about it :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I really wish there was a gofmt for csharp :/

src/Avalonia.Controls/TreeView.cs Outdated Show resolved Hide resolved
samples/ControlCatalog/Pages/TreeViewPage.xaml Outdated Show resolved Hide resolved
@MarchingCube
Copy link
Collaborator Author

@grokys Implemented your feedback. Let me know if you need something else from me.

@grokys grokys merged commit c5e4089 into AvaloniaUI:master Mar 8, 2019
@grokys grokys added this to the 0.8.0 milestone Apr 3, 2019
@MarchingCube MarchingCube deleted the feature/tree-view-selection branch October 26, 2019 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants