-
Notifications
You must be signed in to change notification settings - Fork 79
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
ModelEntryChangeTracker
introduced, removed dependency on contact model's onItemChanged
#16758
Conversation
Jenkins Builds
|
…model's onItemChanged signal Closes: #16754
6b1f0e5
to
447a7d0
Compare
readonly property var myContactsModelConnection: Connections { | ||
target: root.contactsStore.myContactsModel ?? null | ||
enabled: d.activeChatType === Constants.chatType.oneToOne | ||
StatusQUtils.ModelEntryChangeTracker { |
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.
I'm wondering if we need the ModelEntryChangeTracker
. In this case we can use something like this:
property ContactDetails oneToOneChatContact: ContactDetails {
publicKey: d.activeChatId
contactsStore: root.contactsStore
profileStore: root.profileStore
}
Have you found other usages as well?
If this is the only usage, I'd advise against ModelEntryChangeTracker
usage. The complete solution should be coming from the ContactDetails
component.
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.
There are two usages right now: Chat/RootStore
and ActivityNotificationMessage
.
I fully agree it's not a final solution. Right now it's the least invasive method to unblock #16667 which is providing significant simplifications related to contacts models. In the next step we will remove ModelEntryChangeTracker
which is only temporary replacement for nim's model onItemChanged
signal, releasing dependency on that custom feature of backend's model. The target is to use ContactDetails
or join models at early stage to provide all necessary details via models.
Regarding the ContactDetails
in this particular context I see also some other problems we should consider:
ContactDetails
depends onProfileStore
so it would introduce unnecessary dependency to thisRootStore
even though its totally not needed hereContactDetails
should not be used in store as it's breaking contract that stores are just a thin layer over backend's functionality
So I'm still assuming it's temporary solution on the way towards further simplifications/cleanup. Just trying to fix everything at once triggers too many changes.
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.
Totally agree! Glad it's another step forward and not the final solution 😄
ContactDetails
dependencies should be simplified as well at some point if we can extract the user profile from the model and we don't need the profileStore anymore.
Ideally it would work only with a model. The purpose of the component is just to provide some default values on top of ModelEntry
.
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.
LGTM
What does the PR do
ModelUtils
: optimizeindexOf
by moving implementation to C++ModelUtils
: expose methodpersistentIndex
giving possibility to obtain persistent index in QMLonItemChanged
, unblocking Refactor contacts models to have a single model, remove useless properties and improve updating #16667Closes: #16754
Affected areas
ModelUtils
,ActivityNotificationMessage
with subclassesArchitecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide