status-desktop icon indicating copy to clipboard operation
status-desktop copied to clipboard

[Epic] General approach for displaying contact details

Open MishkaRogachev opened this issue 2 years ago • 13 comments

The Problem

In general, we display model-based contact details using the standard QAbstractItemModel mechanisms. However, in a few In a few places we need to display contact details based on a specific contactId. Here are just a few of them:

We use the Utils.getContactDetailsAsJson function to retrieve the data, but we also need to know about updates to this contact's and for this we subscribe to the signal from the model in contactsStore. More precisely, to 3 different models because contacts are stored in myContactsModel, receivedContactRequestsModel, sentContactRequestsModel. And we don't receive signals about users who are not in contacts, and we won't see until restart that the user has changed profile pic or alias.

Another problem is that requesting contact data by id is synchronous and can lead to frezzes on UI

TODO:

  • [x] #14962
  • [x] #14963
  • [ ] #14964
  • [ ] #14965
  • [ ] #14966
  • [ ] #14967

MishkaRogachev avatar Aug 22 '23 12:08 MishkaRogachev

I'd like to mention another related problem.

Intro details

When working on https://github.com/status-im/status-desktop/issues/11854 I discovered this property: https://github.com/status-im/status-desktop/blob/123ed272c20a3c1d49f29c69c6a508fe319dedee/ui/app/AppLayouts/Chat/stores/RootStore.qml#L732 which is updated with many Connections/Binding components below, e.g. this one: https://github.com/status-im/status-desktop/blob/10a7b385d548a725ff7cddba29a5e775b9b41c09/ui/app/AppLayouts/Chat/stores/RootStore.qml#L785-L790

The problem

I found out that the when condition doesn't seem to work correctly here, we make a lot of getContactDetailsAsJson calls with _d.activeChatType === 6, which is a community channel.

This leads to this an error in logs:

DBG 2023-08-24 19:19:02.087+03:00 id doesn't have expected length            topics="contacts-service" tid=34034906 file=service.nim:400 pubkey=0x039fe1dbdd83e6c903f00da15a36ba5279699090aac77f87859bf6da2705f149ee36639205-3654-4bd3-aaad-2ddc4522fc1e parsedChars=76 expectedLength=132

And what?

I don't think that this call is a big deal, but I'm pretty sure that this is only the top of an iceberg. There might be other problems related to this behaviour.

Creating a general approach should fix this as well.


cc @alexjba @MishkaRogachev

igor-sirotin avatar Aug 28 '23 19:08 igor-sirotin

I found out that the when condition doesn't seem to work correctly here, we make a lot of getContactDetailsAsJson calls with _d.activeChatType === 6, which is a community channel.

@igor-sirotin Basically, the when probably works correctly. when evaluated to false prevents from assigning value to the target, but not from evaluating value binding itself. Those are two different things.

To prevent unwanted calls we would have to use sth like that:

Binding on oneToOneChatContact { 
     when: _d.activeChatId && _d.activeChatType === Constants.chatType.oneToOne  
     value: {
                if ( _d.activeChatId && _d.activeChatType === Constants.chatType.oneToOne  )
                     return Utils.getContactDetailsAsJson(_d.activeChatId, false) 
               else
                    return {}
     }
     restoreMode: Binding.RestoreBindingOrValue 
 } 

micieslak avatar Feb 08 '24 16:02 micieslak

cc @noeliaSD

alexjba avatar May 15 '24 18:05 alexjba

Here's my plan for this task:

  1. Create a nim model that proxies the service cache. This model will be exposed to qml and could replace the current models (MyMutualContacts, BlockedContacts, IncomingPendingContactRequests, OutgoingPendingContactRequests) in a follow-up task. These models can be replaced with a SFPM where they're needed in the UI. The model will proxy the service cache in as similar way as implemented here https://github.com/status-im/status-desktop/blob/master/src/app/modules/main/wallet_section/assets/balances_model.nim

  2. Create a new generic c++ component SingleModelItemData that can provide a live object from a model. This component will receive the key roleName and the value and will provide a modelItem variable that will be a live QObject holding all the model roleNames as properties. I've already started working on this component and it's almost complete.

  3. Create a qml component ContactDetails that will use the new model and the SingleModelItemData. Its responsibility is to provide default values and handle other cases (requesting contacts whenever the contact is not in the model, requesting extra data if needed)

This is a usage example:

ContactDetails.qml

required property var contactsModel
required property string publicKey

[other optional input args if needed]

readonly property string name: data.modelItem.available ? data.modelItem.name : ""
readonly property OnlineStatus onlineStatus: data.modelItem.available ? data.modelItem.onlineStatus : OnlineStatus.Available

[other readonly properties]

SingleModelItemData {
    id: data
    sourceModel: root.contactsModel
    key: "pubKey"
    value: root. publicKey
}

[other optional logic like request extra data if needed]

@jrainville @osmaczko @MishkaRogachev @igor-sirotin I'd appreciate some thoughts on this. Do you see any major blockers?

alexjba avatar May 22 '24 15:05 alexjba

I'd appreciate some thoughts on this. Do you see any major blockers?

@alexjba I can mostly talk for the Nim part, but it should be pretty straightforward.

Do note that you can reuse user_model as is and just create a new generic model in contacts/view that will contain . That way, you can keep the old models in the meantime and do the refactor step by step.

jrainville avatar May 22 '24 17:05 jrainville

I like the approach and can't think of any blockers off the top of my head. The only thing I would be careful about is the case when the contact is removed from the underlying model at the moment we interact with it.

osmaczko avatar May 24 '24 10:05 osmaczko

I have some ideas on this, will share next week when I'm back from vacation.

igor-sirotin avatar May 24 '24 11:05 igor-sirotin

The only thing I would be careful about is the case when the contact is removed from the underlying model at the moment we interact with it.

It's a good point! I'll provide a config option in the SingleModelItemData to allow a model item snapshot on removal and add a flag to the item saying that it was removed from the model. We can cache model items containing basic types, models and property-value pairs from QObjects. I think it will cover 99% of use cases.

alexjba avatar May 27 '24 06:05 alexjba

@alexjba your idea described above is more or less the same thing I wanted to propose 👍

Just one thing to note. It would be nice for ContactDetails to support fetching contacts from store nodes, e.g. to be used in SendContactRequestModal, which does that itself now: https://github.com/status-im/status-desktop/blob/16926511841da84f72c1333c6350aef43a1bc1b7/ui/imports/shared/popups/SendContactRequestModal.qml#L29-L33

This includes API:

  • a method to start store node request
  • loading property
  • some error property to see if the contact wasn't found, or any other fetch error

This logic is currently kept in SendContactRequestModal (and I believe in some other places), but we should extract and reuse it.

igor-sirotin avatar May 28 '24 11:05 igor-sirotin

This task is promoted to Epic and split into smaller chunks.

alexjba avatar May 29 '24 08:05 alexjba

@noeliaSD We'll need to re-plan this. Based on current priorities, we can't deliver it in 2.30

alexjba avatar May 29 '24 08:05 alexjba

Closed by mistake

alexjba avatar Jun 04 '24 14:06 alexjba

We can take some subtasks again on the next stabilization phase! Changed the target to 2.31. However, probably the complete epic cannot be done in just 1 stabilization phase but just some subtasks.

noeliaSD avatar Jun 05 '24 15:06 noeliaSD