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

Prevent re-rendering of account sidebar when switching account #3789

Conversation

Simon-Laux
Copy link
Member

@Simon-Laux Simon-Laux commented Apr 24, 2024

Closes #3776

Alternative to #3787 (which showed caching issues when switching accounts)

The only thing that is a bit annoying in terms of flickering now are the gallery tabs (when the experimental feature location streaming is enabled), though that is only minor, we could solve that by splitting the setting store up in one that caches account setting and another one that caches config.json/DesktopSettings, the latter stores the experimental option for location streaming and does not need to be reloaded everytime the account id changes.

@adzialocha
Copy link
Member

adzialocha commented Apr 24, 2024

This might cause the chat state to be temporarily invalid, right? I can see how switching the account id will not reset the chat id to undefined, leaving the chat state with an chat not matching the account. Probably most components do not see that invalid state because they are too slow, but it might be a bug cause for the future. Ideally we would call unselectChat whenever we unselect an account (similar to how I did it in my other PR #3787)

@Simon-Laux
Copy link
Member Author

I used a ref instead of refactoring and moving the component up.

@Simon-Laux Simon-Laux force-pushed the simon/i3776-prevent-re-rendering-of-sidebar-when-switching-account branch from 75eb957 to bc6c55b Compare April 24, 2024 18:06
Copy link
Collaborator

@WofWca WofWca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ehh, looks a little fragile. We reset all state of ChatProvider with unselectChat, but <ContextMenuProvider>, <DialogContextProvider> etc also have their state that could become invalid, don't they?

@WofWca
Copy link
Collaborator

WofWca commented Apr 25, 2024

Maybe just keeping the scroll position is good enough for now? With getSnapshotBeforeUpdate

Though if you're sure it's all good, I won't insist. I'm just voicing my thoughts.

@@ -129,6 +130,9 @@ export default class ScreenController extends Component {
if (this.selectedAccountId === undefined) {
return
}

this.unselectChatRef.current?.()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean here? Looks weird to me :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is set from inside ChatContext. Read it as chatContext.unselectChat()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to doing it like that is to introduce the unselectChat as a prop to the classic class-style React component:

https://github.com/deltachat/deltachat-desktop/blob/adz/i3776-fix-rerendering/src/renderer/ScreenController.tsx#L330-L337

@Simon-Laux
Copy link
Member Author

<ContextMenuProvider>, <DialogContextProvider> etc also have their state that could become invalid, don't they?

I guess yes in theory: a dialog like settings could still be open when you switch account from a notification that was for another account.

Maybe just keeping the scroll position is good enough for now? With getSnapshotBeforeUpdate

no, because:

  1. the sidebar is a functional component not a class component
  2. the react "key" hack forces complete re-rendering, so this won't help as the component does not exist anymore
  3. it would still flicker which is annoying.

@Simon-Laux
Copy link
Member Author

The only way to switch accounts without clicking in the interface (which means no context menu / dialog can be open at the same time), is to click on a notification that is for another account (I just added a commit to close all dialogs in that case).

(At least to according to my current knowledge.)

@Simon-Laux Simon-Laux force-pushed the simon/i3776-prevent-re-rendering-of-sidebar-when-switching-account branch from f37baf1 to 58fac03 Compare April 30, 2024 22:19
@Simon-Laux Simon-Laux merged commit e7fc9e2 into master Apr 30, 2024
4 of 6 checks passed
@Simon-Laux Simon-Laux deleted the simon/i3776-prevent-re-rendering-of-sidebar-when-switching-account branch April 30, 2024 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switching accounts re-renders everything visibly including sidebar
4 participants