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

feat(message)_: add PinnedBy to common.Message #6226

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jrainville
Copy link
Member

Needed for status-im/status-desktop#16896

Having PinnedBy directly in the Message object makes it way simpler in the client to know if a message is pinned. This saves us from having to keep a cache of the pinned messages and comparing all new messages.

@jrainville jrainville requested review from ilmotta, Cuteivist and igor-sirotin and removed request for ilmotta and Cuteivist December 18, 2024 19:03
@status-im-auto
Copy link
Member

status-im-auto commented Dec 18, 2024

Jenkins Builds

Click to see older builds (8)
Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ 300545f #1 2024-12-18 19:08:01 ~3 min tests 📄log
✔️ 300545f #1 2024-12-18 19:08:36 ~4 min macos 📦zip
✔️ 300545f #1 2024-12-18 19:08:43 ~4 min ios 📦zip
✔️ 300545f #1 2024-12-18 19:09:23 ~5 min macos 📦zip
✔️ 300545f #1 2024-12-18 19:09:33 ~5 min linux 📦zip
✔️ 300545f #1 2024-12-18 19:09:53 ~5 min android 📦aar
✔️ 300545f #1 2024-12-18 19:10:18 ~6 min windows 📦zip
✔️ 300545f #1 2024-12-18 19:11:09 ~7 min tests-rpc 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ f048766 #2 2024-12-18 19:12:24 ~2 min tests 📄log
✔️ f048766 #2 2024-12-18 19:14:35 ~4 min windows 📦zip
✔️ f048766 #2 2024-12-18 19:14:42 ~4 min macos 📦zip
✔️ f048766 #2 2024-12-18 19:15:08 ~4 min ios 📦zip
✔️ f048766 #2 2024-12-18 19:15:45 ~5 min linux 📦zip
✔️ f048766 #2 2024-12-18 19:16:00 ~5 min android 📦aar
✔️ f048766 #2 2024-12-18 19:16:16 ~5 min macos 📦zip
✔️ f048766 #2 2024-12-18 19:17:44 ~6 min tests-rpc 📄log
✖️ 98176d9 #3 2024-12-20 15:53:44 ~2 min tests 📄log
✔️ 98176d9 #3 2024-12-20 15:55:37 ~4 min windows 📦zip
✔️ 98176d9 #3 2024-12-20 15:55:43 ~4 min ios 📦zip
✔️ 98176d9 #3 2024-12-20 15:55:54 ~4 min macos 📦zip
✔️ 98176d9 #3 2024-12-20 15:56:44 ~5 min macos 📦zip
✔️ 98176d9 #3 2024-12-20 15:56:48 ~5 min linux 📦zip
✔️ 98176d9 #3 2024-12-20 15:56:58 ~5 min android 📦aar
✔️ 98176d9 #3 2024-12-20 15:58:09 ~6 min tests-rpc 📄log

@jrainville jrainville force-pushed the feat/add-pinned-info-in-messages branch from 300545f to f048766 Compare December 18, 2024 19:09
@jrainville jrainville changed the title feat(message): add PinnedBy to common.Message feat(message)_: add PinnedBy to common.Message Dec 18, 2024
Copy link
Collaborator

@igor-sirotin igor-sirotin left a comment

Choose a reason for hiding this comment

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

Looks noice! 🙌
Just one question: when someone else pinned/unpinned a message, does status-go publish a signal with the updated pinnedBy property?

@jrainville
Copy link
Member Author

Looks noice! 🙌 Just one question: when someone else pinned/unpinned a message, does status-go publish a signal with the updated pinnedBy property?

Yes, that already worked before.

I posted a video here: status-im/status-desktop#16897 (comment)

Copy link
Contributor

@ilmotta ilmotta left a comment

Choose a reason for hiding this comment

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

This saves us from having to keep a cache of the pinned messages and comparing all new messages.

@jrainville In status-mobile we keep the pinned messages state in sync with status-go via signal pinMessages. Pinned messages in mobile are indexed and stored in memory by chat ID & message ID, which makes checking if any message is pinned cheap.

The currently pinned messages per chat will continue to be stored in memory even with the changes from this PR (in mobile).

Maybe on desktop things are different? Could you elaborate a bit more? Maybe then I'll understand what/if something needs to change or be optimized on the mobile side.

@jrainville
Copy link
Member Author

jrainville commented Dec 20, 2024

Maybe on desktop things are different? Could you elaborate a bit more? Maybe then I'll understand what/if something needs to change or be optimized on the mobile side.

@ilmotta We used to only have the pinned messages in a model made to be accessed in the PinnedMessagesPopup. This popup worked flawlessly. When the user clicked the button to see the pinned messages, they all appeared correctly.

We didn't store them in a "cache" though, so when we scrolled up in a channel, the old messages didn't appear as pinned

I did implement a cache solution in the chat module that would keep the pinned messages like you do and then apply the pinned status to new messages by looping them when they arrived.

However, we found that if we did it using the way I just did here in status-go, it removes a lot of logic needed in the middle layer (Nim).

So like you said, you can keep your current method and it will work perfectly. We just prefer to write as little Nim code as possible, so this solution solves it 😄

Needed for status-im/status-desktop#16896

Having `PinnedBy`  directly in the Message object makes it way simpler in the client to know if a message is pinned. This saves us from having to keep a cache of the pinned messages and comparing all new messages.
@jrainville jrainville force-pushed the feat/add-pinned-info-in-messages branch from f048766 to 98176d9 Compare December 20, 2024 15:51
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.

5 participants