Render display names of users and rooms according to the spec - take 2 #4
Conversation
|
Revisiting the topic of user renaming and moving the signal to Connection:
Therefore I'll remove the last commit (about deprecating User::nameChanged), connect the two signals and add Connection::avatarChanged "mediation signal" in a similar fashion - in the same PR, if you don't mind. |
|
Just a notice: because I force-pushed the branch, some commits shifted later than comments referring to them. In case you already started reviewing - I amended 8d56fcf to only emit one signal. |
| } | ||
|
|
||
| Room* room = roomMap.value(id, nullptr); |
There was a problem hiding this comment.
I think it would be easier to read this way:
if( roomMap.contains(id) )
return roomMap.value(id);
It might be a bit slower than your version, but I don't think it matters much (it's a hashmap, so we have O(1) complexity)
What do you think?
There was a problem hiding this comment.
This is just a matter of personal taste, as for me. Since you drive the car, I willingly comply :)
|
FIrst: Thanks for the PR and sorry it took so long! I'm not quite sure about moving the nameChanged and avatarChanged signals to the Connection class. Imho that belongs to the User class and I'd like to make sure Connection doesn't evolve into a god class (it is already quite big). |
|
Regarding signal connections - basically, it's memory that we are wasting. Since connections are stored in hash map, access time doesn't really change when this map grows. As for memory - each connection is at least four pointers big, which (with 32 bit pointers) brings at least 4x4x(number of connections) bytes needed to store the map (not counting the map's own overhead). Probably, 8Mb+ is not so much these days - but still looks quite a waste. |
|
I rebased and amended the branch according to the minor comments. Regarding connections and Connection (pun intended :) ) - let's discuss separately (it might be a good idea to separate this part into another PR). |
In particular, this will be needed to render room names according to the CS spec.
…CS spec. According to section 11.2.2.3 of the CS spec, clients SHOULD follow a certain algorithm of making a non-ambiguous display name of a user in the room context. This algorithm implies checking whether other room members have the same display name. This commit prepares for implementation of the algorithm: 1. Use a hash map instead of a list to store room members. The external Room::users() API is kept intact. 2. Convenience CRUD methods are implemented to deal with the hash map. 3. An additional slot for user renaming is introduced (because renaming affects the hash map). Binding of actual signals is left for the next commit. 4. nullptr is the recommended representation of a null pointer since C++11. Use that and mandate compiler support of that.
…red on a displayname update. This changes the way displayname is supplied to a client application - instead of calculating immediately, displayname becomes a separate stored value that is refreshed with every change of the list of members, or the name, or the canonical alias. displaynameChanged signal is supplied to subscribe to these updates: in case of displaying a room in the roomlist a client should use this new signal instead of Room::namesChanged. The displaname calculation algorithm is described in section 11.2.2.5 of the CS spec: https://matrix.org/docs/spec/r0.0.1/client_server.html#calculating-the-display-name-for-a-room
(as requested at PR review)
|
Reworked the branch once more to remove two commits about Connection:: signals and connections to them. A PR for organising signals for user events will come separately, and probably not soon - I'll think how to consolidate signals through another mediator than Connection. Therefore, the current PR should be clear to merge with respect to all your comments. |
…named signal, which clients should use in the room context. Processing changes of user displaynames is tricky: we have to not only deal with the currently renamed user but also with its past and new namesakes which might change representation due to that renaming. So in the worst case a single User::nameChanged signal may lead to three Room::memberRenamed references (and 3 user displaynames updated in the UI, respectively). And the newly added users should be taken care of in a similar manner, of course.
|
Here's one more commit to take care of user renamings. Turned out to be quite non-trivial; the good news though is that solving this problem also solved the question regarding signals mediator. As it turns out, every room has to process each renaming anyway because it causes potential disambiguations (even for users that did not rename themselves). So the best thing to do here for consolidation is to throw all user renaming signals into all rooms which is quite a bit of signal-slot spam (and it's worse than having additional megabytes of memory consumed instead). So, there you have it - no consolidated signals. The updated PR for the Quaternion part will come later. |
|
Looks all good now :) Thanks for your work! |
Though it's yet incomplete (room names are on the way), I think you can start reviewing this. As you asked, I have split one big commit into smaller ones (and this is only the lib part, naturally). The code is backwards-compatible; however, I had to lay some leftovers for that - namely, User::displayname() and User::nameChanged(). Probably, they should be marked as deprecated in the /** docs */. I'll submit a separate PR on Quaternion later to reflect the newly introduced functionality in the UI.