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

Undoing archival of a pinned chat doesn't re-pin it #6968

Closed
2 tasks done
Vinnl opened this issue Aug 7, 2024 · 3 comments · Fixed by #6969
Closed
2 tasks done

Undoing archival of a pinned chat doesn't re-pin it #6968

Vinnl opened this issue Aug 7, 2024 · 3 comments · Fixed by #6969
Labels

Comments

@Vinnl
Copy link
Contributor

Vinnl commented Aug 7, 2024

Using a supported version?

  • I have searched searched open and closed issues for duplicates.
  • I am using Signal-Desktop as provided by the Signal team, not a 3rd-party package.

Overall summary

When I accidentally archive a chat (because I mix up the keyboard shortcut with that of another app), I can click "Undo" in the toast. But if that chat was pinned, it will no longer be pinned after hitting Undo.

Steps to reproduce

  1. Got to a pinned chat
  2. Archive that chat (e.g. Ctrl+Shift+A)
  3. Click "Undo" in the toast in the bottom left-hand corner

Expected result

The chat to be unarchived, and pinned.

Actual result

The chat is unarchived, but no longer pinned.

Screenshots

No response

Signal version

7.19.0

Operating system

Fedora Silverblue Linux 40 (via Flatpak - apologies for reporting here, but this seems very unlikely to be a packaging issue)

Version of Signal on your phone

7.12.3 (Android)

Link to debug log

No response

@Vinnl
Copy link
Contributor Author

Vinnl commented Aug 7, 2024

I was also looking to see if I could submit a PR myself to fix this. I got as far as finding that clicking Undo triggers this Redux action, where the archived status is restored:

conversation.setArchived(false);

Which calls out to the setArchived action, which unpins it on archival:

But I'm not sure if there's a preferred/good pattern for remembering that status, so it can be restored when undoing, so for now I gave up there. Alternatively, maybe that action shouldn't unpin it in the first place, and the list of pinned chats should just not show archived chats. But I'm weeding into architectural decisions too much for a drive-by PR, so I'll leave t at that - but happy to take another look if so desired, if you have suggestions on where to go next.

@jamiebuilds-signal
Copy link
Member

jamiebuilds-signal commented Aug 7, 2024

I think for the Undo operation specifically we should also undo the unpin() that's called in conversation.setArchived. So in the onArchived() redux action we should probably save the previous isPinned state and pass that to the toast that it opens, so that when it calls onUndoArchive() it could pass the previous isPinned value in.

@Vinnl
Copy link
Contributor Author

Vinnl commented Aug 8, 2024

Perfect, thanks. PR here: #6969

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants