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

Display room's parent in room switcher #1732

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

Conversation

nishanthkarthik
Copy link

@nishanthkarthik nishanthkarthik commented May 18, 2024

When rooms in different spaces have the same name #general, it is hard to distinguish them. This PR adds a user-configurable option to display parent room names by the side

image

(ignore my theme!)


Also removed a hack for QTBUG-93281

@nishanthkarthik
Copy link
Author

@deepbluev7 Would you be interested in reviewing this PR?

@deepbluev7
Copy link
Member

Sorry, but I don't know what that command means.

Thanks for asking 🥰

@redsky17
Copy link
Member

That was a bot Nico uses to help do some administrivia for PRs and such, not an actual response from the person that is Nico. I let him know about your comment @nishanthkarthik :).

Copy link
Member

@redsky17 redsky17 left a comment

Choose a reason for hiding this comment

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

This looks good to me, but we can wait for Nico to review if you want :)

@nishanthkarthik
Copy link
Author

Rebased and fixed linter feedback. Let's wait for Nico too

Comment on lines +2949 to +2961
auto getParentRoomIdsWithTxn = [&](const std::string &id) -> std::optional<std::string> {
auto cursor = lmdb::cursor::open(txn, spacesParentsDb_);
std::string_view sp = id, space_parent;
if (cursor.get(sp, space_parent, MDB_SET)) {
while (cursor.get(sp, space_parent, MDB_FIRST_DUP)) {
if (!space_parent.empty())
return std::make_optional(static_cast<std::string>(space_parent));
}
}
cursor.close();

return std::nullopt;
};
Copy link
Member

Choose a reason for hiding this comment

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

This actually gets a random room parent. I would suggest you actually query the canonical room parent. Otherwise it might appear quite random, which room is actually listed here. I would suggest you check the parentSpace function in the TimelineModel.

Actually since you don't need the parentSpace for anything but displaying the name of the filtered entries, I would suggest you might want to do (in RoomsModel::roleNames()):

auto parent = roomList->getRoomById();
if (parent) {
  auto desc = parent->parentSpace();
  if (desc) return desc->roomName();
}

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why, a room's parentSpace is empty. I tried to read off mtx::events::state::space::Parent events manually from a room and I did not find a parent. That's why I think I gave up and used spacesParentsDb.

This is the diff I used to test your approach

case Roles::RoomParent: {
            const auto roomPtr = roomListModel_.getRoomById(QString::fromStdString(rooms[index.row()].id));
            if (auto &room = *roomPtr; roomPtr) {
                if (const auto &parent = room.parentSpace(); parent) {
                    qInfo() << "Parent has name" << parent->roomName();
                    return parent->roomName();
                } else {
                    qWarning() << "No parent for room" << "expected" << rooms[index.row()].parent;
                }
            }
            else {
                qWarning() << "No room with ID";
            }
            return QString{};
        }

Copy link
Member

Choose a reason for hiding this comment

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

Did that room possibly not have a canonical parent? Can you maybe log the space parent event for that room?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think these rooms have any parents.

cache::client()->getStateEventsWithType<mtx::events::state::space::Parent>(this->room_id_.toStdString()).size() is 0.

Copy link
Author

@nishanthkarthik nishanthkarthik Jun 22, 2024

Choose a reason for hiding this comment

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

I think spacesParentsDb_ builds this relation using mtx::events::state::space::Child. MSC2946 describes walking the spaces tree using m.space.child.

The matrix spec also says

Spaces form a hierarchy of rooms which clients can use to structure their room list into a tree-like view. The parent/child relationship can be defined in two ways: with m.space.child state events in the space-room, or with m.space.parent state events in the child room.

In most cases, both the child and parent relationship should be defined to aid discovery of the space and its rooms. When only a m.space.child is used, the space is effectively a curated list of rooms which the rooms themselves might not be aware of. When only a m.space.parent is used, the rooms are “secretly” added to spaces with the effect of not being advertised directly by the space.

...

To avoid situations where a room falsely claims it is part of a given space, m.space.parent events should be ignored unless one of the following is true: A corresponding m.space.child event can be found in the supposed parent space. ...

I think m.space.child should be the primary sources of truth

Copy link
Member

Choose a reason for hiding this comment

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

All 3 options are valid:

  • only space.child
  • only space.parent
  • both

We generally only accept canonicalParents as something added to the room name in Nheko, because this means the room admin accepted that relationship. Otherwise a room can have multiple parents or someone could maliciously craft space.child events to make a room belong to a community it doesn't want to. So I think we really should only accept canonical parent events.

Copy link
Author

Choose a reason for hiding this comment

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

In case the canonical parent events are not available for a room, can this be an opt-in config? This is the structure that mautrix - matrix bridges use, so I'd really like to use the parent-child relationship from spacesParentsDb_. I think I can manually add the parent event to my ~50 matrix rooms but I'd prefer not to.

Alternatively, I can make an issue on mautrix about this but I'm not aware of the technical decisions behind not doing this by default.

Comment on lines +252 to +259
Loader {
active: Settings.displayParentInSwitcher && model.roomParent !== ""
sourceComponent: Label {
color: model.index == popup.currentIndex ? palette.highlightedText : palette.text
text: "[" + model.roomParent + "]"
font.pixelSize: popup.avatarHeight * 0.5
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you make this font somehow special, so that it doesn't just look like a suffix to the roomname? I.e. make it smaller, bold, different color or whatever?

Copy link
Author

Choose a reason for hiding this comment

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

Good idea

@nishanthkarthik
Copy link
Author

Ready for next round of feedback :)

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.

3 participants