Unify community members lists into one #16508
Conversation
jrainville
requested review from
micieslak,
caybro,
alexjba and
a team
as code owners
| sectionItemCopy.pendingMemberRequests = pendingMemberRequests | ||
| sectionItemCopy.declinedMemberRequests = declinedMemberRequests | ||
| return sectionItemCopy | ||
| } |
There was a problem hiding this comment.
This little trick makes it so that I don,t have to modify every place we used members. I basically put the right model inside the right property name.
Let me know if there is a cleaner way to do it with some sort of SFPM.
There was a problem hiding this comment.
I don't think there's an easy way; that said, I think this is overkill. For most common properties, I think you can continue using sectionItemModel and only where you need a specific member model, you should pass one of the filtered SFPM models instead of the members
Jenkins BuildsClick to see older builds (7)
|
| sectionItemCopy.pendingMemberRequests = pendingMemberRequests | ||
| sectionItemCopy.declinedMemberRequests = declinedMemberRequests | ||
| return sectionItemCopy | ||
| } |
There was a problem hiding this comment.
I don't think there's an easy way; that said, I think this is overkill. For most common properties, I think you can continue using sectionItemModel and only where you need a specific member model, you should pass one of the filtered SFPM models instead of the members
|
@caybro thanks for the review. Indeed, passing the members models when needed was simpler than expected. I thought it was used way more, but it's not that bad and it's way cleaner that way. |
| @@ -44,6 +44,7 @@ Item { | |||
| required property CurrenciesStore currencyStore | |||
| property bool hasAddedContacts: false | |||
| property var communityData | |||
| property var joinedMembers | |||
There was a problem hiding this comment.
You could really just pass the count instead of the whole model :)
| @@ -109,7 +113,7 @@ StatusSectionLayout { | |||
| id: communityHeader | |||
|
|
|||
| title: community.name | |||
| subTitle: qsTr("%n member(s)", "", community.members.count || 0) | |||
| subTitle: qsTr("%n member(s)", "", root.joinedMembers.count || 0) | |||
There was a problem hiding this comment.
This is relying on the existence of the count property in the model; better use the ModelCount attached property
There was a problem hiding this comment.
Dumb question, but what is that? I tried googling and I can't find what you mean.
| @@ -959,7 +959,7 @@ QtObject { | |||
| id: importControlNodePopup | |||
| ImportControlNodePopup { | |||
| onClosed: destroy() | |||
| onImportControlNode: root.rootStore.promoteSelfToControlNode(community.id) | |||
| onImportControlNode: root.rootStore.promoteSelfToControlNode(communityId) | |||
What does the PR do
Fixes #16433
In the section model, we used to have 4 models for all the different types of members (joined, request pending, request declined and banned)
Now they are united in one model (
allMembers) and only in QML we filter them in four different models.I also fixed some bugs that flew under the radar, like the fact that we never update the member's state when it changed, we used to just move them from one model to another.
Known issue
The number of members in the profile showcase will be wrong for communities where you are an admin.
This is because since we united all members in one model, the count of the members is higher than the actual joined members.
I tried doing the same trick I did in ChatLayout where I create a SFPM to get the joined members and use that for the count, but it doesn't seem to work with the way we use those models. It would either break or always show
0.I can open an issue later, but it's a very minor issue IMO.
Affected areas
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Impact on end user
Nothing, the UX is all the same, this is just a refactor to simplify the handling of members.
How to test
Risk
Tick one:
Worst case scenario would be one of the list of members in the admin panel to be erroneous until a restart