Fix context menu text being different based on where right click occurred for emojis

khuddite commented 2 weeks ago

First time contributor checklist:

[X] I have read the README and Contributor Guidelines
[X] I have signed the Contributor Licence Agreement

Contributor checklist:

[X] My contribution is not related to translations.
[X] My commits are in nice logical chunks with good commit messages
[X] My changes are rebased on the latest main branch
[X] A npm run ready run passes successfully (more about tests here)
[X] My changes are ready to be shipped to users

Description

Fixes #7058

The fix was to update the logic for isImage flag for the context menu. Currently, isImage flag is set to true for emojis, which should not be. Thus I added an extra check to filter out emojis, so isImage results in false for emojis.

What kind of manual testing did you do? I used my macOS to test the changes. I sent some messages(with/without emojis) and images to myself for testing. I tried to cover all combinations of texts, emojis, and images I could think of and ensure the changes behaved as expected. I made sure that Copy menu shows up as Copy(not Copy Image) regardless of where I right-click when it has text and emojis.

CleanShot 2024-11-05 at 09 59 52

When I selected the emoji in the editable text input and right-clicked, there was no option for Paste because it was perceived as an image. However, I believe that was also a bug. So, I fixed it as part of my changes in this PR.

CleanShot 2024-11-05 at 10 01 10

Did you write any new tests? No, I did not
What operating systems did you test with? macOS 10.15.7
What other devices did you test with? No other devices because I believe this bug/fix is not platform-specific.

scottnonnenberg-signal commented 2 weeks ago

Thanks for your work on this - it will be released soon in an upcoming beta release!

khuddite commented 2 weeks ago

Thanks for your work on this - it will be released soon in an upcoming beta release!

Thanks for your help and review, appreciated!

scottnonnenberg-signal commented 2 weeks ago

This change is now available in this release! https://github.com/signalapp/Signal-Desktop/releases/tag/v7.33.0-beta.1

khuddite commented 2 weeks ago

@scottnonnenberg-signal Can you let me know where I can find my commit? It looks like my commit was not merged.

khuddite commented 2 weeks ago

Nvm, I saw it's in the beta branch. Thanks!

khuddite commented 1 week ago

@scottnonnenberg-signal I am sorry to bother you, but do you know why my commits are not showing up here - https://github.com/signalapp/Signal-Desktop/commits/main/?author=khuddite?

I see my commit co-authored with you in the list without the filter, though.

I'd love to contribute more to Signal projects (desktop, android, and iOS) moving forward, but at the same time, I'd like my contributions to be publicly visible.

scottnonnenberg-signal commented 1 week ago

You are indeed an author on the commit, as you can see here:

Hopefully Github improves their interface soon! In the meantime, you can always point people at the release notes!

khuddite commented 1 week ago

Alright, thanks!

signalapp / Signal-Desktop