Display room's parent in room switcher #1732
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ | |
|
|
||
| #include <nlohmann/json.hpp> | ||
|
|
||
| #include <mtx/events/spaces.hpp> | ||
| #include <mtx/responses/common.hpp> | ||
| #include <mtx/responses/messages.hpp> | ||
|
|
||
|
|
@@ -2945,6 +2946,26 @@ Cache::roomNamesAndAliases() | |
| { | ||
| auto txn = ro_txn(env_); | ||
|
|
||
| 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; | ||
| }; | ||
|
Comment on lines
+2949
to
+2961
There was a problem hiding this comment. 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 auto parent = roomList->getRoomById();
if (parent) {
auto desc = parent->parentSpace();
if (desc) return desc->roomName();
}There was a problem hiding this comment. I'm not sure why, a room's parentSpace is empty. I tried to read off 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{};
}There was a problem hiding this comment. Did that room possibly not have a canonical parent? Can you maybe log the space parent event for that room? There was a problem hiding this comment. I don't think these rooms have any parents.
There was a problem hiding this comment. I think The matrix spec also says
I think m.space.child should be the primary sources of truth There was a problem hiding this comment. All 3 options are valid:
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. There was a problem hiding this comment. 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 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. There was a problem hiding this comment. I think we really should stick to only showing the canonical parent as the "official" parent. There are other features that rely on that like custom stickers and emojis. Maybe mautrix could change their spaces to use canonical parents? In any case, if there is a canonical parent, we should really pick that one! |
||
|
|
||
| auto getRoomName = [&](const std::string &roomId) { | ||
| auto spaceDb = getStatesDb(txn, roomId); | ||
| auto membersDb = getMembersDb(txn, roomId); | ||
| return Cache::getRoomName(txn, spaceDb, membersDb).toStdString(); | ||
| }; | ||
|
|
||
| std::vector<RoomNameAlias> result; | ||
| result.reserve(roomsDb_.size(txn)); | ||
|
|
||
|
|
@@ -2962,13 +2983,21 @@ Cache::roomNamesAndAliases() | |
| alias = aliases->content.alias; | ||
| } | ||
|
|
||
| auto parentId = getParentRoomIdsWithTxn(room_id_str); | ||
| auto parentName = std::string{}; | ||
|
|
||
| if (parentId) { | ||
| parentName = getRoomName(*parentId); | ||
| } | ||
|
|
||
| result.push_back(RoomNameAlias{ | ||
| .id = std::move(room_id_str), | ||
| .name = std::move(info.name), | ||
| .alias = std::move(alias), | ||
| .recent_activity = info.approximate_last_modification_ts, | ||
| .is_tombstoned = info.is_tombstoned, | ||
| .is_space = info.is_space, | ||
| .parent = std::move(parentName), | ||
|
There was a problem hiding this comment. Can we load this information lazily when it is accessed first in the RoomsModel instead? Looking up this data seems to add quite a lot of overhead to constructing the fuzzy search structure, but we only need it when displaying specific results,so we can really look it up only when showing a certain entry. There was a problem hiding this comment. I have a few deadlines coming up. I'll make the fixups in a few weeks :) |
||
| }); | ||
| } catch (std::exception &e) { | ||
| nhlog::db()->warn("Failed to add room {} to result: {}", room_id, e.what()); | ||
|
|
||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea