Unity-Technologies / EditorXR

Author XR in XR
Other
925 stars 167 forks source link

ListView Core #551

Closed mtschoen-unity closed 5 years ago

mtschoen-unity commented 5 years ago

Purpose of this PR

Updated ListView code to reflect the latest state of the ListView framework repo here https://bitbucket.org/Unity-Technologies/list-view-framework/

This mostly involves the introduction of IListViewItem, IListViewData, INestedListViewItem, and INestedListViewData. This frees us from needing to extend a particular base class for list view item and data classes. As a result, all of the list view implementation code in EditorXR has changed to some degree.

Testing status

Tested all workspaces with lists in edit and play mode. Unit tests pass (though none of the tests cover list views)

Technical / Halo risk

Tech Risk: 2 -- The underlying principles are the same, but types have been shuffled around, and the base controller classes have changed significantly Halo Risk: 1 -- Many features rely on lists, but this is all view code.

Comments to reviewers

It's good that we now have a simple interface pattern for ListViewItem and ListViewData. Naming has gotten a little more consistent, too. ListViewNestedData seemed wrong in retrospect.

Unfortunately, git didn't recognize the file move/rename as such, and the core list view classes are shown as deleted and added. The diff is pretty complex, anyway. You might just want to review that class as new code.

One thing I wish I had a way around was the duplicated code in EditorXRListViewController and EditorXRNestedListViewController. We can't just add extension methods, because we want our lists to have new fields. Another option is to just make the haptics code another component that subscribes to events on the list, or otherwise hooks into the list without the need to extend ListViewController. As it stands, I don't hate it.

Another change that came with this refactor was to remove GetListItem as a delegate on IListViewItem and instead implement it specifically for HierarchyListViewController. This was to remove the need to cast from the specific item type (TItem) down to IListViewItem, and then inevitably back to the item type (TItem or in the one case we use it, HierarchyListItem). Adding a class restraint to the generic constraints on the list view controller also got rid of some potential boxing casts when working with list items and data in the base class.

A few misc refactors:

I wavered on whether the GrabRows feature should live in the core, or not. It's actually not too hard to extract it into the EditorXRListViewController classes, but it seemed generally useful to be able to temporarily remove list items.

mtschoen-unity commented 5 years ago

👋 Hi @stella3d and @amydigiov! I need a pair of eyes on this PR before I merge it in. I've tested it pretty thoroughly but I could always use a confirmation that it doesn't just work on my machine. The changes are pretty extensive, but a fair bit of it is from @amydigiov's refactor. I just need one review, so whoever picks this up first, please let everyone know.