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

Add initial spec of IndexPath class #81

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

marcelwgn
Copy link
Contributor

@marcelwgn marcelwgn commented Apr 10, 2020

This PR adds the initial API spec for the IndexPath class.
@anawishnoff @ranjeshj FYI

Background

To easily track selection in nested collections, we need an object that we can use to uniquely identify an item in nested collections.
This can be done with the IndexPath.

Description

The IndexPath class provides indexing of nested collections, for example in a TreeView.

Examples

In the tree below, using the following IndexPath

var indexPath = IndexPath.CreateFromIndices(new List<int>{ 1, 0, 3 });

would select the item "Kitchen cabinet style", since it is in the second group of first nesting level, first group in the second level of nesting and is the fourth element in the last nesting level.

Sample tree selection

Creating IndexPath objects

New IndexPaths objects can be created using the static IndexPath.CreateFrom and IndexPath.CreateFromIndices methods:

// Points to the third element in a flat collection
var simplePath = IndexPath.CreateFrom(2);

// This is the same IndexPath as "simplePath"
var simplePathIndices = IndexPath.CreateFromIndices(new List<int>(){ 2 });

// This will point to the third child of the second element in a nested collection
var groupedPath = IndexPath.CreateFrom(1,2);

// The following IndexPath is equivalent to "groupedPath"
var groupedPathIndices = IndexPath.CreateFromIndices(new List<int>(){ 1, 2 });

Comparing two IndexPaths

Comparing two IndexPath objects can be done using the CompareTo function:

var baseIndexPath = IndexPath.CreateFromIndices(new List<int> { 1, 4, 2 });

var shorterPath = IndexPath.CreateFrom(2, 0);

// Since shorterPath takes second element of first nesting, it is "greater" than baseIndexPath
Assert.AreEqual(1, shorterPath.CompareTo(baseIndexPath));


var sameLengthPath = IndexPath.CreateFromIndices(new List<int> { 1, 0, 1 });

// Since sameLength path enters before baseIndexPath on the second level, it is smaller than basePath
Assert.AreEqual(-1, sameLengthPath.CompareTo(baseIndexPath));


var baseIndexPathCopy = IndexPath.CreateFromIndices(new List<int> { 1, 4, 2 });
// Those two are the same, and thus CompareTo returns 0
Assert.AreEqual(0, baseIndexPathCopy.CompareTo(baseIndexPath));

API Notes

Function Description
Int32 GetSize() Returns the size of the IndexPath.
Int32 GetAt(Int32 index) Returns the index for a given nesting level.
Int32 CompareTo(IndexPath other) Compares the IndexPath to a different IndexPath. If the other IndexPath is behind the current IndexPath, the method returns -1, if the other IndexPath is before the current IndexPath it returns 1. In case of equality this method returns 0.
static IndexPath CreateFrom(Int32 index) Creates an IndexPath that only consists of the index provided.
static IndexPath CreateFrom(Int32 groupIndex, Int32 itemIndex) Creates an IndexPath with the given group index and item index for the given group.
static IndexPath CreateFromIndices(IVector indices) Creates an IndexPath from the given list of integers.

API Details

[WUXC_VERSION_MUXONLY]
[webhosthidden]
runtimeclass IndexPath : Windows.Foundation.IStringable
{
    Int32 GetSize();
    Int32 GetAt(Int32 index);
    Int32 CompareTo(IndexPath other);

    [default_overload] [method_name("CreateFrom")]
    static IndexPath CreateFrom(Int32 index);
    [method_name("CreateFromGroupAndItemIndex")]
    static IndexPath CreateFrom(Int32 groupIndex, Int32 itemIndex);
    static IndexPath CreateFromIndices(Windows.Foundation.Collections.IVector<Int32> indices);
}

Appendix

Currently we use a Vector<int> to store those indices. Since we don't have any operations that would modify the size of an IndexPath, that is how many nesting levels it works with, shouldn't we switch to an array to increase performance?

@marcelwgn marcelwgn requested a review from a team as a code owner April 10, 2020 18:40
```
would select the item "Kitchen cabinet style", since it is in the second group of first nesting level, first group in the second level of nesting and is the fourth element in the last nesting level.

![Sample tree selection](./sample-tree-selection.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might just be on my end, but the image isn't rendering for me.

Copy link
Contributor Author

@marcelwgn marcelwgn Apr 13, 2020

Choose a reason for hiding this comment

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

I think the webview is not really aware of the relative paths here. Downloading them should work correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird, it's there, but it doesn't show up. I can vouch that the picture's accurate :)

active/IndexPath/IndexPath.md Outdated Show resolved Hide resolved
active/IndexPath/IndexPath.md Outdated Show resolved Hide resolved
@anawishnoff
Copy link
Contributor

Currently we use a Vector to store those indices. Since we don't have any operations that would modify the size of an IndexPath, that is how many nesting levels it works with, shouldn't we switch to an array to increase performance?

Are there for sure no situations in which you'd need to modify an IndexPath after it's been created? I feel like there might be some possible use cases for this. 🤔

@marcelwgn
Copy link
Contributor Author

I am not sure if there are scenarios where one would benefit from it. If the collection changes, e.g. an item suddenly becomes a collection I think it is better to create new IndexPath objects to adress the new items instead of modifying an existing one. SelectionModel just keeps overriding the path objects instead of modifying them.

The big design question at hand I think is whether IndexPath should be mutable or immutable. Should one be able to change the index they are pointing to or should they return/create a new IndexPath if they want to point to something different?

@ranjeshj
Copy link

Currently we use a Vector to store those indices. Since we don't have any operations that would modify the size of an IndexPath, that is how many nesting levels it works with, shouldn't we switch to an array to increase performance?

Are there for sure no situations in which you'd need to modify an IndexPath after it's been created? I feel like there might be some possible use cases for this. 🤔

I think it was initially mutable and we changed it to immutable. Array sounds like a good idea to reduce the footprint.


# Description

The `IndexPath` class provides indexing of nested collections, for example in a TreeView.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a corresponding update to TreeView to accept an IndexPath?

Copy link

@ranjeshj ranjeshj Apr 14, 2020

Choose a reason for hiding this comment

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

No. TreeView does not use this yet. This is used by SelectionModel, which still needs an API spec. I would make this public along with SelectionModel. We internally use this for NavigationView.

Copy link
Contributor

Choose a reason for hiding this comment

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

For a feature that cuts across multiple APIs, we likely want a single API spec.

Copy link
Contributor Author

@marcelwgn marcelwgn Apr 15, 2020

Choose a reason for hiding this comment

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

Should we/I add the SelectionModel to this API spec since it is the main API this will interact with (and the only one so far)?

```c++
[WUXC_VERSION_MUXONLY]
[webhosthidden]
runtimeclass IndexPath : Windows.Foundation.IStringable
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it need IStringable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not really need it, it's just better for debugging and representation. Should we remove it?

Choose a reason for hiding this comment

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

It is a debugging aid. Perhaps there is a different way to achieve the debug visualization

Copy link
Contributor

Choose a reason for hiding this comment

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

I think debugging is fine. What does it return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the IndexPath IndexPath.CreateFrom(1,3) it would return "R.1.3"

active/IndexPath/IndexPath.md Outdated Show resolved Hide resolved
[default_overload] [method_name("CreateFrom")]
static IndexPath CreateFrom(Int32 index);
[method_name("CreateFromGroupAndItemIndex")]
static IndexPath CreateFrom(Int32 groupIndex, Int32 itemIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's obvious enough that two parameters means the grouping case, it would be more clear with a name like CreateForGroup, or some such with "group" in its name.

But how is the grouped case different from the indices case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only difference is that it's a bit faster and easier to use in the two index case. Updated the name to CreateFromGroupAndItemIndex.

Choose a reason for hiding this comment

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

Yes. These are convenience constructors. The most common are one and two indices(one level grouping) in which case from a usage perspective we don't have to require creating a list of one or list of two items.

```
would select the item "Kitchen cabinet style", since it is in the second group of first nesting level, first group in the second level of nesting and is the fourth element in the last nesting level.

![Sample tree selection](./sample-tree-selection.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird, it's there, but it doesn't show up. I can vouch that the picture's accurate :)

@marcelwgn
Copy link
Contributor Author

I think it was initially mutable and we changed it to immutable. Array sounds like a good idea to reduce the footprint.

Should I create a tracking issue for that over at WinUI so we update it to array and adjust the API once API review has gone through?


# Description

The `IndexPath` class provides indexing of nested collections, for example in a TreeView.
Copy link
Contributor

Choose a reason for hiding this comment

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

For a feature that cuts across multiple APIs, we likely want a single API spec.

```c++
[WUXC_VERSION_MUXONLY]
[webhosthidden]
runtimeclass IndexPath : Windows.Foundation.IStringable
Copy link
Contributor

Choose a reason for hiding this comment

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

I think debugging is fine. What does it return?

Comment on lines +82 to +83
Int32 GetSize();
Int32 GetAt(Int32 index);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an IVectorView?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding IndexOf doesn't make much sense I think.

@marcelwgn
Copy link
Contributor Author

Are there any points that I need to follow up on or I should address here @anawishnoff @MikeHillberg @ranjeshj?

@anawishnoff
Copy link
Contributor

anawishnoff commented Jun 30, 2020

Everything looks good here to me, but I'll leave it to @ranjeshj and @MikeHillberg for any final approval. Thanks Marcel!

I believe we'll need to publish this alongside the SelectionModel spec, which will be in progress for a while until we decide between using a flexible container or attached behaviors to implement this in ItemsRepeater. So, lots of things linked together here :) I'll make sure to keep you updated and in the loop about progress on SelectionModel.

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.

4 participants