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

Added lab feature to pin/unpin messages #7762

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

cintek
Copy link
Contributor

@cintek cintek commented Dec 10, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

This PR adds a new feature so users can (un)pin messages and open a timeline where only the pinned messages are visible. The feature can be found and activated in the "Labs" settings.

Details of this feature:

  • It is possible to (un)pin messages
  • Users can open a timeline with all pinned messages
  • Users can jump to a specific messages from the pinned messages timeline to the "normal" timeline
  • Users have to have the necessary role to (un)pin messages

Little background:
Currently, if pinned messages are missing the client will load chunks of older messages like it does in the "normal" timeline until all pinned messages are available. This is not very efficient but it works for now.

Motivation and context

Solves #5933 and #357

Screenshots / GIFs

Here you can see the context menu which includes a button to pin a message. When a message is pinned already a button to do the opposite will be shown.
context_menu

When there are pinned messages available a button to open a timeline with these messages appears at the top right corner.
open_pinned_messages

Tests

  • (Un)Pinned messages and compared result with web client

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 12

Checklist

Signed-off-by: Christoph Klassen christoph.klassen@intevation.de

@cintek cintek marked this pull request as ready for review December 10, 2022 19:15
@bmarty bmarty added the Z-Community-PR Issue is solved by a community member's PR label Dec 12, 2022
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your Pull Request!

I have made a bunch of first remarks.

@cintek
Copy link
Contributor Author

cintek commented Dec 16, 2022

@bmarty thanks for your review! I edited the things you marked and also did some additional refactoring (renamed files and variables to have more consistency).
Since I was already working on the edits I also made the icons (to pin/unpin events) in the context menu bigger so their size fits better to the other ones.

@cintek
Copy link
Contributor Author

cintek commented Dec 16, 2022

In case that someone wants to know where the icons which are used in this PR come from : They are from Microsoft's Fluent Icons.

@cintek
Copy link
Contributor Author

cintek commented Dec 16, 2022

@bmarty I removed the property -XX:MaxPermSize=2048m in gradle.properties (diff) because I'm using a newer version of Java JDK which was complaining that this property is deprecated.
If you want I will add it again. I just forgot to add it again before commiting.

@catfromplan9
Copy link

Waiting for this to be merged, it's a very useful feature. Right now I have to explore the room data and copy out the pin IDs, then check them one by one. It isnt very convenient at all

@sschuberth
Copy link

@bmarty I removed the property -XX:MaxPermSize=2048m in gradle.properties

I'd also vote for removing the MaxPermSize property (which has been deprecated since Java 1.8), as the source / target compatibility is at 11 anyway:

https://github.com/vector-im/element-android/blob/591b08f1ff426d5d7a5ee6e60599d809f45bbcc4/dependencies.gradle#L5-L6

@artempas
Copy link

@bmarty any updates?

@grvn-ht
Copy link

grvn-ht commented Apr 26, 2024

nice feature here! Does it need more changes before merging it @bmarty ?

@cintek
Copy link
Contributor Author

cintek commented Sep 12, 2024

BTW: In Element X for Android this feature is already included and since today enabled as default: https://github.com/element-hq/element-x-android/releases/tag/v0.6.0.

@catfromplan9
Copy link

BTW: In Element X for Android this feature is already included and since today enabled as default: https://github.com/element-hq/element-x-android/releases/tag/v0.6.0.

I'd rather not use element X, can you update this PR with the necessary changes and get it merged?

@cintek
Copy link
Contributor Author

cintek commented Sep 16, 2024

I'd rather not use element X, can you update this PR with the necessary changes and get it merged?

I implemented all requested changes already and don't know if there is anything from my side that has to be done for a merge.

@catfromplan9
Copy link

catfromplan9 commented Sep 16, 2024

I'd rather not use element X, can you update this PR with the necessary changes and get it merged?

I implemented all requested changes already and don't know if there is anything from my side that has to be done for a merge.

It says reviews are pending with changes requested, @bmarty any updates on this? I really like this client and want to continue using it for the forseeable future

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants