signalapp / Signal-Desktop

A private messenger for Windows, macOS, and Linux.
https://signal.org/download
GNU Affero General Public License v3.0
14.68k stars 2.68k forks source link

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

Closed Vinnl closed 3 months ago

Vinnl commented 3 months ago

Using a supported version?

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 commented 3 months ago

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:

https://github.com/signalapp/Signal-Desktop/blob/02e7a9e1a5ba4a0e1d82ed30eeab235f3b35a42d/ts/state/ducks/conversations.ts#L1237

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

https://github.com/signalapp/Signal-Desktop/blob/02e7a9e1a5ba4a0e1d82ed30eeab235f3b35a42d/ts/models/conversations.ts#L4236

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 commented 3 months ago

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 commented 3 months ago

Perfect, thanks. PR here: https://github.com/signalapp/Signal-Desktop/pull/6969