Fix slow chat switching 2: Electric Boogaloo #16279
Conversation
jrainville
requested review from
igor-sirotin,
osmaczko,
micieslak and
caybro
Jenkins BuildsClick to see older builds (20)
|
| @@ -117,6 +119,15 @@ QtObject: | |||
| result = newQVariant(item.pubKey) | |||
| of ModelRole.DisplayName: | |||
| result = newQVariant(item.displayName) | |||
| of ModelRole.PreferredDisplayName: | |||
There was a problem hiding this comment.
nitpick: I'm wondering if there's a reason why it's not a member in the item object? 😄
We'll still need to take care of the dataChanged events for this new role and add this logic in all functions updating any of the user names. So IMO adding it as a member in the item object would be a nice way to go here.
There was a problem hiding this comment.
I prefer adding the PreferredDisplayName in dataChanged in the model, as it's way simpler than modifying all the modules that use this model.
Also, this way, the logic for the order which name is more important is only in one place.
| } | ||
|
|
||
| rightPanel: Component { | ||
| id: userListComponent | ||
| UserListPanel { | ||
| id: userListPanel | ||
| readonly property bool isFullCommunityList: !root.chatContentModule || !root.chatContentModule.chatDetails || !root.chatContentModule.chatDetails.requiresPermissions |
There was a problem hiding this comment.
both isFullCommunityList and the Connections below have a dependency on the onChatContentModuleChanged signal. The implementation assumes that isFullCommunityList will always receive the signal first and its value is updated before handling the connection signal.
I'd redo this a little bit to remove any dependency on the signal order.
| onChatContentModuleChanged: { | ||
| if (!root.chatContentModule || | ||
| (userListPanel.usersModel != null && root.chatContentModule.chatDetails.belongsToCommunity && userListPanel.wasFullCommunityList && userListPanel.isFullCommunityList)) { | ||
| // If the previous channel had the full community list already, and we still need the whole list, just keep the old model |
There was a problem hiding this comment.
I'm wondering if this isn't a bit dangerous..What if we delete the previous channel and the model is still cached here? I think it would lead to a crash since there are cases where the usersModel won't have a binding anymore on the proper chatContentModule, but the model reference assigned below.
There was a problem hiding this comment.
I tested it and it didn't crash, but I totally get your worry. I also think it can be dangerous.
I'm not sure how to make it safe though (apart from just removing this).
I know back in the day when we switched from one UserList for the whole community to having a specific lists per channel, I suggested having one model for all the members of the community that we can show for unpermissioned channels, and then permissioned channels could have their own model, but I know it wasn't done because it was too complex with the current architecture.
I'll try to figure out a way to do it now.
It would free a lot of memory and also fix the worry that it might crash.
I'll open a new issue for it though and work on it in another PR on top
There was a problem hiding this comment.
Issue opened here #16288
There was a problem hiding this comment.
PR opened here: #16301
We can wait until that new PR is merged before merging this one, that way we don't have to merge the possibly dangerous code.
There was a problem hiding this comment.
Alright, I removed the dangerous code. I was tired of rebasing lal my PRs on this. I want to merge it hahaha
jrainville
force-pushed
the
fix/slow-chat-switching-2
branch
from
3314d35
to
76f88c8
Compare
jrainville
force-pushed
the
fix/slow-chat-switching-2
branch
from
76f88c8
to
0d2faa9
Compare
| self.beginInsertRows(modelIndex, first, last) | ||
| self.items.add(items) | ||
| self.endInsertRows() | ||
| self.countChanged() |
There was a problem hiding this comment.
As we have ModelCount attached property, there is no need to expose count from nim. Ideally we should just remove it from the model. Not necessary in this PR, just pointing for the future.
jrainville
force-pushed
the
fix/slow-chat-switching-2
branch
from
0d2faa9
to
955d95e
Compare
Redo of #16178
Fixes #16132
In the previous PR, I was improving the slow chat switching by delaying the loading of the member list and the nkeeping it in memory.
After discussing with @micieslak , we agreed that it was making the code harder to understand and also didn't fix the root cause.
Turns out, the root cause is the SortFilterProxyModel and the
preferredDisplayNameproperty.@caybro already has a PR improving it, but I pushed it one step further (sorry)
In my PR, I moved the
preferredDisplayNameproperty to the Nim side (this is the biggest speed up).I also made it so that we populate the models in one go instead of one entry at a time (saves just a little bit on start, but it's something)
Finally, I made it so that we don't update the user model if not needed.
Basically, when a community channel is not encrypted is when we put all the members of the community in the member list. Those are the channels that are the slowest to switch to.
To help that, I made it so that if we are switching from an unencrypted channel to another unencrypted channel, we just don't update the member list.
This makes the channel switching instantaneous.
The only caveat of that approach is that we are putting a little bit of app logic in the QML. If that's a problem, I can move it to a property in the model.
fast-switching.webm