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

MSC3230: Spaces top level order #3230

Open
wants to merge 10 commits into
base: old_master
Choose a base branch
from

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Jun 3, 2021

BillCarsonFr and others added 2 commits June 3, 2021 12:12
Co-authored-by: Michael Telatynski <7t3chguy@googlemail.com>
proposals/3230-spaces_top_level_order.md Outdated Show resolved Hide resolved
proposals/3230-spaces_top_level_order.md Outdated Show resolved Hide resolved
@turt2live turt2live changed the title Spaces top level order MSC3230: Spaces top level order Jun 3, 2021
@turt2live turt2live added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review client-server Client-Server API labels Jun 3, 2021
proposals/3230-spaces_top_level_order.md Outdated Show resolved Hide resolved
Co-authored-by: Erkin Alp Güney <erkinalp9035@gmail.com>
proposals/3230-spaces_top_level_order.md Outdated Show resolved Hide resolved
proposals/3230-spaces_top_level_order.md Outdated Show resolved Hide resolved
proposals/3230-spaces_top_level_order.md Outdated Show resolved Hide resolved
proposals/3230-spaces_top_level_order.md Outdated Show resolved Hide resolved
proposals/3230-spaces_top_level_order.md Outdated Show resolved Hide resolved
proposals/3230-spaces_top_level_order.md Outdated Show resolved Hide resolved
proposals/3230-spaces_top_level_order.md Outdated Show resolved Hide resolved

As defined per `room tagging`ordering information is given under the order key as a number between 0 and 1. The numbers are compared such that 0 is displayed first. Therefore a room with an order of 0.2 would be displayed before a room with an order of 0.7. If a room has a tag without an order key then it should appear after the rooms with that tag that have an order key, fallbacking then to roomID lexical order.

This alternative has been discarded becaused perceived as confusing in regards of tags intentions.
Copy link
Member

Choose a reason for hiding this comment

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

confusing how? It seems like a natural fit for reusing existing structures in Matrix, which is ideal.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also much easier to work with than lexicographical strings.

Copy link
Member Author

@BillCarsonFr BillCarsonFr Jan 28, 2022

Choose a reason for hiding this comment

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

Caused infinite problems when we first did it due to truncation and rounding and ieee representation quirks. It didn't proove much easier

Copy link
Member Author

Choose a reason for hiding this comment

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

@turt2live looks like this has an implementation vector-im/element-android#3502

Also in web matrix-org/matrix-react-sdk@21fc386

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the spec explicitly calls out tags without a u prefix as reserved for application internal uses, which in this case the tag would be used for ordering spaces instead of as a tag.

proposals/3230-spaces_top_level_order.md Outdated Show resolved Hide resolved
proposals/3230-spaces_top_level_order.md Outdated Show resolved Hide resolved
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@bb010g
Copy link

bb010g commented Jun 12, 2021

The "space folder" future consideration overlaps with spaces themselves? A folder could be a space tagged with "folder display" account data.

A similar approach could be taken for the "space pinning" future consideration. Pinned spaces could be children of a space tagged with "pinned spaces" account data.

should appear after the rooms with that tag that have an order key, fallbacking then to roomID lexical order.

This alternative has been discarded becaused perceived as confusing in regards of tags intentions.

Choose a reason for hiding this comment

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

Suggested change
## Alternatives
Have space child room pointers in MSC1769 profile rooms, hence have the ordering per-profile,
rather than per-account.


In order to find mid points between two orders strings, the `order` string can be considered as
a base N number where N is the length of the allowed alphabet. So the string can be converted
to a base 10 number for computation and mid point computation, then converted back to base N.
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 this algorithm works well enough to go into the spec as a recommended algorithm.

First issue is for an alphabet "A-Z" it considers "B" and "BA" to be the same thing, which they're not. Collisions would become an issue.

For A..B and limit 4
     0
A    8
AA   12
AAA  14
AAAA 15
AAAB 16
AAB  16
AABA 17
AABB 18
AB   16
ABA  18
ABAA 19
ABAB 20
ABB  20
ABBA 21
ABBB 22
B    16
BA   20
BAA  22
BAAA 23
BAAB 24
BAB  24
BABA 25
BABB 26
BB   24
BBA  26
BBAA 27
BBAB 28
BBB  28
BBBA 29
BBBB 30

One could fix this by treating the string as a base N + 1 and padding the end of the string with the new character.
But you get gaps in that case.

For A..B and limit 4
     0
A    27
AA   36
AAA  39
AAAA 40
AAAB 41
AAB  42
AABA 43
AABB 44
AB   45
ABA  48
ABAA 49
ABAB 50
ABB  51
ABBA 52
ABBB 53
B    54
BA   63
BAA  66
BAAA 67
BAAB 68
BAB  69
BABA 70
BABB 71
BB   72
BBA  75
BBAA 76
BBAB 77
BBB  78
BBBA 79
BBBB 80

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I just noticed that the example also highlights this.

"a" = 65*(95^0) 
"z" = 90*(95^0) 
"az" = 65*(95^1) + 90*(95^0) 

Simplified, the calculated values are.

"a" = 65
"z" = 90
"az" = 6265

Sorting them lexicographically, "az" is between "a" and "z", but the numbers don't reflect this.
Worse yet, the number of possible strings between "a" and "z" should be massive! Much bigger than 2^32.
But this algo says the available strings is "90 - 65", which is just 25.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is that we need a representation that allows to easily compute midpoints / interval in between to orders.
Nonetheless it's true that string to base representation is not respecting lexicographic order (as we are padding strings to be same length).
I might update MSC to reflect the need for padding or state to use stringToBase to compare

@aaronraimist
Copy link
Contributor

@turt2live looks like this has an implementation element-hq/element-android#3502

@anoadragon453
Copy link
Member

I've added the known implementations to the PR's description. These appear to constitute appropriate implementations of the MSC, do you agree @BillCarsonFr?

Comment on lines +109 to +110
This alternative has been discarded as it won't scale, could reach event content size limit, and is
less flexible as a way to define order compared to [0,1].
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no size limit for account data events (otherwise m.direct would break). A bigger concern is that clients often break the m.direct event when updating the list in it, because it is racy, which would apply here too, but I very much doubt users will be in so many top level spaces, that this will ever matter here.

On tap would expand the pannel.

## Alternatives

Copy link
Contributor

Choose a reason for hiding this comment

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

Not that I think it is a particularly good idea, but one could also create a private, invisible top level space, that orders its children by the normal rules. Only difference would be that that top level space is not shown in clients and is only used for ordering.


## Future considerations

__Space Pinning__: The room `m.space_order` content could be extended by adding categories like `pinned`.
Copy link

Choose a reason for hiding this comment

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

Is this about pinning non-top level spaces?

Comment on lines +82 to +84
__Space Folder__: In order to save vertical space, content could be extended to define folders
and space with same folder could be represented as a single entry in the space pannel.
On tap would expand the pannel.
Copy link

Choose a reason for hiding this comment

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

if defining folders and spaces with same folder key as folder id will be represented as single entry, how the folder entry will be ordered around other top-level spaces or folders? does folder key is also a lexicographical order key?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet