Skip to content
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

App needs to be restarted to see the new displayname set by other device #3624

Closed
iequidoo opened this issue Jan 7, 2024 · 11 comments
Closed
Assignees
Labels
bug Something isn't working new-core involves or requires an dc-node/core update

Comments

@iequidoo
Copy link

iequidoo commented Jan 7, 2024

  • Operating System (Linux/Mac/Windows/iOS/Android): Debian GNU/Linux 12
  • Delta Chat Version: 1.42.2 (git: v1.33.0-555-g9da03dd2)
  • Expected behavior: The new displayname ("Your Name") is displayed w/o an app restart
  • Actual behavior: Need to restart the app
  • Steps to reproduce the problem:
    • Change the displayname aka "Your Name" on other device
    • Check if the new value is seen in the app

Notes: in DC Android it works as expected. In DC cli, too. Do we have any cache in Desktop?

@iequidoo iequidoo added the bug Something isn't working label Jan 7, 2024
@farooqkz farooqkz self-assigned this Jan 9, 2024
@Simon-Laux
Copy link
Member

we have a cache/store for some settings in desktop, is there an event when it changes?

the cache is there to get core values faster, because it is sync, while the core api is async, though we could probably change the code to have less places that rely on it with the cost that we may have to add some loading delays.

@iequidoo
Copy link
Author

Unfortunately, no special event is emitted for this. Only IncomingMsgBunch is emitted for sync messages, can it be used to update the displayname and other things (e.g. the avatar) that can be synchronised?

@Simon-Laux
Copy link
Member

relying on that event would be rather hacky (naming has only weak link to meaning), but could work, but an event that indicates that some config changed would be even better.

@Simon-Laux
Copy link
Member

Simon-Laux commented Jan 10, 2024

so @farooqkz listen for IncomingMsgBunch in settings store and reload the whole message store on that event, don't forget to add a comment that explains that there is no dedicated event for this yet and maybe link to this issue.

Was considering to do it myself, but I don't want to steal your issue this time ;)

@iequidoo
Copy link
Author

iequidoo commented Jan 10, 2024

Yes, relying on IncomingMsgBunch isn't a good idea, because it only means that some message is received, but it might not be yet applied. So, we can have a race.

EDIT: Maybe indeed add some ConfigChanged event instead of adding temporary hacks? @r10s would it be useful in mobile apps?

EDIT: Though, for self-avatar we already have SelfavatarChanged. Thinking about DisplaynameChanged then, but not sure we need so fine-grained events...

@r10s
Copy link
Member

r10s commented Jan 10, 2024

@r10s would it be useful in mobile apps?

no. we do not cache config values on mobile, this is not needed, things are fast enough. iirc, not caching is no option as all calls are async on desktop? or did that change meanwhile?

@iequidoo
Copy link
Author

iequidoo commented Jan 10, 2024

So, i'm going to add some event like

ConfigChanged {
    key: Config,
    val: Option<String>,
}

It will also replace SelfavatarChanged. But for some time there will be both for compatibility.

EDIT: But then we're going to have some useless events for e.g. LastHousekeeping. So, i'd rather add an event only for synced config keys, but do that on both sides (sender and receiver) for uniformity.

EDIT: How about ConfigSynced (with the same params)?

@Simon-Laux
Copy link
Member

Simon-Laux commented Jan 10, 2024

EDIT: How about ConfigSynced (with the same params)?

I agree, that would be nice, though value is probably not needed, we can always fetch the newest version of it with a core call.

iirc, not caching is no option as all calls are async on desktop? or did that change meanwhile?

if we do loading states (or react suspense rendering) for places where it is needed we could avoid caching those, but would need some refactoring and maybe not applicable for all cases where the settings store is used.

@iequidoo
Copy link
Author

I agree, that would be nice, though value is probably not needed, we can always fetch the newest version of it with a core call.

Decided not to add a value, otherwise it would be logged which might not be good for privacy when sharing logs

@iequidoo
Copy link
Author

Merged the core PR

@iequidoo
Copy link
Author

iequidoo commented Feb 9, 2024

Merged another core PR fixing the previous one, now should work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working new-core involves or requires an dc-node/core update
Projects
None yet
Development

No branches or pull requests

4 participants