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

Rewrite SelectionModel #4533

Merged
merged 33 commits into from
Aug 26, 2020
Merged

Rewrite SelectionModel #4533

merged 33 commits into from
Aug 26, 2020

Conversation

grokys
Copy link
Member

@grokys grokys commented Aug 20, 2020

What does the pull request do?

SelectionModel as ported from WinUI has a number of problems:

  • Didn't handle binding SelectedIndex (Selection Model Issues #4496)
  • Combining flat and nested selection led to unergonomic API (IndexPath for flat selection)
  • Nested selection didn't work very well anyway due to the way TreeView works (don't know what the child items are until the TreeViewItem is materialized, so can't select non-expanded nodes)
  • API was complex with lots of convenience methods so extracted interface was large
  • Was not generic so lots of casts needed in ViewModel layer
  • Did not preserve SelectedIndex/SelectedItem; instead returned first selection
  • AnchorIndex didn't behave like in normal controls
  • Didn't allow setting SelectedItem
  • SelectRange/SelectAll selected multiple items even when SingleSelect = true
  • Didn't work well when making selections before Source assigned, so BeginInit/EndInit logic had to be hacked into control (needs to be possible to set SelectedIndex/SelectedItem before Items is assigned in XAML)

This PR first reverts the changes introduced by the previous SelectionModel attempt and then rewrites SelectionModel to:

  • Only handle list (flat) selections, so indexes are plain ints
  • Slims down the interface
  • Makes SelectionModel generic so that it can be used in ViewModels without casts everywhere
  • Preserves SelectedIndex
  • Makes AnchorIndex work as expected
  • Allows setting SelectedItem
  • Disable range selection when SingleSelect = true
  • Makes setting selection before Source is assigned work better, so BeginInit can simply delay the Source assigment until initialization is finished

Note that after this change, TreeView no longer has a selection model: it will remain reverted to its pre-SelectionModel v1 state.

It's still a WIP, left to do:

  • Use the list of selected objects in SelectedItems to repopulate selection after Reset (needs to be a way to turn it off for large collections)
  • Correctly select newly added items with IsSelected set
  • Re-apply use of Selection to samples (were changed back to SelectedItems when I reverted the previous SelectionModel)
  • Various other little bugs

Breaking changes

Yep, it's a breaking change :) Not so much from 0.9.x but the SelectionModel API has changed from the previous version shipped in the 0.10 previews.

Fixed issues

Fixes #4496

@robloo
Copy link
Contributor

robloo commented Aug 21, 2020

A lot of these changes make sense. Removing IndexPath; however, cripples the usefulness of SelectionModel and as you stated, doesn't allow it to work in a hierarchy (TreeView). Removing IndexPath doesn't seem like a good idea to future-proof this API. IndexPath with a single int index isn't that complicated.

@grokys
Copy link
Member Author

grokys commented Aug 25, 2020

Removing IndexPath; however, cripples the usefulness of SelectionModel and as you stated, doesn't allow it to work in a hierarchy (TreeView)

The idea will be to add a TreeSelectionModel at a later date: I'm working on it over at https://github.com/grokys/AvaloniaSelectionModel but it's a bit more involved so might take a while.

IndexPath with a single int index isn't that complicated.

Heh, it's a lot more complicated than it first seems!

`IEnumerable<T>` is not covariant for value types, so we need to use the non-generic `IEnumerable` everywhere under the hood to ensure we can work with both value and reference types.
@grokys grokys marked this pull request as ready for review August 26, 2020 14:04
@grokys grokys changed the title WIP: Rewrite SelectionModel Rewrite SelectionModel Aug 26, 2020
Copy link
Member

@jmacato jmacato left a comment

Choose a reason for hiding this comment

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

code style 👍 tested controlcatalog and found nothing wrong 👍 would need a deeper dive testing @danwalmsley

@danwalmsley danwalmsley merged commit 2a89ba9 into master Aug 26, 2020
@danwalmsley danwalmsley deleted the refactor/selectionmodel-rewrite branch August 26, 2020 15:55
@Kermalis
Copy link
Contributor

Does this fix #4472 or #4048? More unlikely to fix #4461

@grokys
Copy link
Member Author

grokys commented Aug 26, 2020

@Kermalis I wasn't targeting those issues specifically, as this PR was holding up a release, however it's possible it may have fixed some things. I plan to look into those issues again when I have a moment.

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.

Selection Model Issues
5 participants