Add Support for Emoji Reactions #1980
Conversation
|
While I'm not opposed to the idea, it gets spammy real quick. Also with multiple reactions, clicking on a reaction and adding a different one removes another reaction instead of the clicked one. Also there should not be a new notification send when a reaction was changed: |
|
Thanks for the feedback!
That would make sense, although I'm not sure that's really feasible to enforce in the backend due to federation. I'm not very familiar with ActivityPub tbh but I can imagine it might cause inconsistencies between instances if they receive reactions in different orders. It's probably easiest to just make the frontend handle this by sorting reactions by count and only display the first couple ones.
That's really strange and to be quite honest I have no idea why it happens, but I'll definitely look into it.
Are you referring to an already existing emoji just having its counter incremented, like from 2 to 3? Yeah, I can see how that might get quite noisy. That should be fixable by simply checking the reaction count before sending out the notification though. I'll do my best to solve all of these issues and will report back when everything is ready. |
| const visibleReactions = reactions | ||
| .filter(x => x.get('count') > 0) | ||
| .sort((a, b) => b.get('count') - a.get('count')) | ||
| .filter((_, i) => i < maxReactions); |
There was a problem hiding this comment.
The limit for how many reactions are displayed should probably be different from the amount of reaction one user can set.
Let's say an admin limits reaction per user to 1, now only the most used reaction is displayed on a post.
I propose a new variable like: MAX_VISIBLE_REACTIONS or have it as a settings (more convenient than env vars)
And change the default of max visible to 6, which would be 1 row. Looks cleaner imo.
There was a problem hiding this comment.
Good point, I'll throw that into the app settings modal. And now that I think about it, it might also be useful to have a button that quickly expands all reactions for a single toot. What do you think?
There was a problem hiding this comment.
I'm not sure about a button, since I think it would look weird.
Thinking again, a plus button which shows all reactions might actually be good.
But maybe a column setting or user preference to show all reactions and/or show all in the detailed status view?
There was a problem hiding this comment.
Quick mock-up for a button (missing proper styling, but good enough to visualise):
I used the plus icon because it was the quickest I could find which fits.
Might be confused with "add reaction" though
There was a problem hiding this comment.
I think using a plus button would be kind of confusing because that's already used for adding reactions. Also, announcement reactions have their plus button right next to the existing emojis, just where that expand button would be. The way I'm currently doing it is only limiting in the timeline view, and always display all reactions when you click on the status to open the detailed view.
edit: speaking of things that are confusing, shoutout to GitHub for dynamically loading new comments in the regular discussion but not for commits
:3
Good point.
I was referring to there being 4 notifications but only 3 reactions, because "admin" changed a reaction.
Thanks! |
|
The way Misskey handles emoji reactions, a user can only react exactly once to a post, and it replaces the "like" feature. ( + federates to instances that don't support reactions as just a like) Maybe, for better cross-fediverse compatibility, it'd be good to implement it the same way? |
One reaction per user is probably enough, yeah. Cross-federation is a very good point though, but out of my expertise. |
|
As for fedi compatibility, Misskey handles multiple reactions from the same account by updating the emoji rather than adding a new one. Hitting fav is treated the same way as reacting with ⭐. I can see how this might be a problem, so I agree it's a good idea to at least make Nevertheless, I still think admins should be able to raise that limit if they wish, as other fedi software like Akkoma doesn't have this restriction either. At least my personal fedi bubble is overwhelmingly in favour of multi reactions. |
| number | ||
| > | ||
| <FormattedMessage id='settings.num_visible_reactions' defaultMessage='Number of visible reaction badges:' /> | ||
| </LocalSettingsPageItem> |
There was a problem hiding this comment.
Not sure about making this a glitch flavour only setting or feature in general.
While I don't see upstream implementing it anytime soon, I think it would be worth it to have reactions in the vanilla flavour. (Which would require this being a preference instead of local setting)
There was a problem hiding this comment.
Fair point, I'll move it. Having reactions in the vanilla flavour also requires copying all changes in /app/javascript/flavours/glitch over to /app/javascript/mastodon, right?
| @@ -60,7 +60,7 @@ export default class StatusPrepend extends React.PureComponent { | |||
| return ( | |||
| <FormattedMessage | |||
| id='notification.reaction' | |||
| defaultMessage='{name} reacted to your post' | |||
| defaultMessage='{name} reacted to your status' | |||
There was a problem hiding this comment.
Minor nitpick: Why changing the default message to say "status" instead of "post"?
Was this accidental?
There was a problem hiding this comment.
Oh that's just to make it consistent with the other default messages, I originally called it "post" without realizing that the other default messages use "status" here.
There was a problem hiding this comment.
Maybe they just weren't updated then. Okay.
|
Just realized I totally forgot to reply to the other issue:
Ah, now I get what you mean. I've looked into it, and as it turns out this also happens with favs:
Steps to reproduce: Favourite something, wait for the notification, then remove the fav and add it again. Also works for boosts. I agree this is an issue that should be resolved, but Mastodon's backend notification code is mostly agnostic to the particular kind of notification. I believe it would actually be easier to remove duplicates for all kinds of notifications at once rather than do so only for reactions. So, depending on how you look at it, this might kinda fall out of scope. I assume this won't take too much effort to fix though, so I could also include it in this PR if you want.
100% agreed, I might actually work on that after I'm done with reactions. |
| @@ -21,6 +22,7 @@ import PictureInPicturePlaceholder from 'mastodon/components/picture_in_picture_ | |||
| // We use the component (and not the container) since we do not want | |||
| // to use the progress bar to show download progress | |||
| import Bundle from '../features/ui/components/bundle'; | |||
| import { visibleReactions } from '../../flavours/glitch/initial_state'; | |||
There was a problem hiding this comment.
This should import mastodons initial state, not glitch's
| @@ -9,6 +9,8 @@ import ImmutablePureComponent from 'react-immutable-pure-component'; | |||
| import { me } from '../initial_state'; | |||
| import classNames from 'classnames'; | |||
| import { PERMISSION_MANAGE_USERS } from 'mastodon/permissions'; | |||
| import EmojiPickerDropdown from '../features/compose/containers/emoji_picker_dropdown_container'; | |||
| import { maxReactions } from '../../flavours/glitch/initial_state'; | |||
There was a problem hiding this comment.
maxReactions should be added to the import in line 9
| @@ -6,6 +6,7 @@ import ClearColumnButton from './clear_column_button'; | |||
| import GrantPermissionButton from './grant_permission_button'; | |||
| import SettingToggle from './setting_toggle'; | |||
| import { PERMISSION_MANAGE_USERS, PERMISSION_MANAGE_REPORTS } from 'mastodon/permissions'; | |||
| import PillBarButton from '../../../../flavours/glitch/features/notifications/components/pill_bar_button'; | |||
There was a problem hiding this comment.
PillBarButton is unused in vanilla
I agree it would make sense to remove duplicates of all notifications in that case. (Also I probably should've created one review instead of creating multiple comments. Sorry) |
It would probably be best to solve this issue directly in vanilla Mastodon.
Not a problem! |
|
So, if I haven't lost track of anything, the strange reaction removal bug is the last major obstacle standing in the way of getting this merged. I noticed that it only occurs if you're doing it fast enough, which lets me suspect it might have something to do with Sidekiq. I wanna be totally honest here, though: I'm not familiar with the Ruby ecosystem at all and this is the most Ruby code I've written in my life, so my guess is basically as good as anyone's. It's probably gonna take some time to get to the bottom of this, but I will give updates if something noteworthy happens. |
|
Well, this is either good or bad news: I can't reproduce the bug anymore. I tried everything, including switching between Firefox and Chrome and using the vanilla and glitch flavour, even going as far as using the exact same emojis that you originally chose. No matter how fast or slow I perform the steps, it just works. |
I can pin it down a bit: It works as long as the user which reacted to a post doesn't reload. Edit: I'll look deeper into it once I'm awake again. |
|
Well that was certainly something. Turns out I just forgot to pass the reaction's actual name to Anyway, the bug should definitely be gone for good now. By the way, do you know what's up with the CircleCI build? I noticed it succeeded the very first time it ran when I originally opened the PR, but it's been failing ever since for no apparent reason. |
That seems to have fixed it. Can't reproduce it anymore.
Looks like it uses a openssl3 environment and fails during precompiling assets due to missing |
There was a problem hiding this comment.
Oof, making a review was way more exhausting than I expected.
Someone else would have to review the backend, as I'm not familiar enough with it.
I still have some functional concerns, but most of them fall out of scope.
(For example that it's not possible to know which user reacted with which emoji, but looks to me that this is a limitation of Mastodon with how notifications work)
Thanks for putting the work into this btw!
| onClick={this.handleClick} | ||
| onMouseEnter={this.handleMouseEnter} | ||
| onMouseLeave={this.handleMouseLeave} | ||
| title={`:${shortCode}:`} |
There was a problem hiding this comment.
I think this could be changed to a translatable string {count} people reacted with {shortcode} for screenreader users, but I don't have one to test what it currently says.
Maybe the title attribute is unneeded though, as likes and boosts also have no title attribute and this might interfere with screenreaders, telling them to say the ":shortcode:" (which is already there than hovering the emoji) instead of the count.
There was a problem hiding this comment.
I have never used a screenreader to be honest, but I imagine making this a whole sentence could quickly turn reading all reactions out loud into a lengthy process. It's probably best to ask people who actually use a screenreader for feedback on this.
app/javascript/flavours/glitch/features/local_settings/page/index.js
Outdated
Show resolved
Hide resolved
app/javascript/flavours/glitch/features/local_settings/page/item/index.js
Outdated
Show resolved
Hide resolved
| onClick={this.handleClick} | ||
| onMouseEnter={this.handleMouseEnter} | ||
| onMouseLeave={this.handleMouseLeave} | ||
| title={`:${shortCode}:`} |
There was a problem hiding this comment.
| title={`:${shortCode}:`} |
Again, maybe messes with screenreaders and is already shown when hovering the emoji.
app/javascript/mastodon/features/status/components/detailed_status.js
Outdated
Show resolved
Hide resolved
|
I've checked the Ruby part of this and it all seems fine to me after the most recent three commits. |
|
It'd be nice if the client api for this was closer to Pleroma/Akkoma's implementation, it's very similar as is but there are a few differences:
it'd be nice to not need to handle the two separately in my client tho again idk how you'd feel about including a pleroma-prefixed api. |
|
@easrng tbh, I agree that the key in the list of reactions could be called emoji_reactions. However, I think the rest is really out of place on something that is not Pleroma or, unfortunately, Akkoma, and the API of reactions was designed with the rest of the existing Mastodon API in mind (see also c8ab12a14a76ac0b73134d8ce39a17a899501b07). |
|
yeah i guess, and it's minimal effort for me especially since i'm only implementing emoji reactions now, but i assume there are clients out there that already support reactions on pleroma/akkoma and it'd be nice if they worked as is with this. |
Do you mean the attribute of the JSON object returned by |
I was thinking opening up multiple PRs as this currently changes 80 files. It would make the review process easier, as I completely loose track every time I look at it. |
Oh okay, I didn't know vanilla also loads from glitch. Will do that!
I understand, although I don't know whether it's even possible to separate the trees now without basically rewriting the entire commit history. Especially the frontend side has a lot of overlapping commits between the vanilla and glitch flavours, because I always edited them simultaneously ever since I ported the feature to vanilla. So, while I'm not opposed to the idea as I find it increasingly hard to keep track of everything myself, I'm not sure whether it's feasible to do so at this point. |
Yeah, I probably should've mentioned that glitch locales basically extend vanilla locales.
Yeah, it probably requires basically starting from scratch on new branches and manually copying changes from this PR :/ |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
|
This causes the action bar to be too long in search results, thus cutting off the timestamp. |
Must be introduced by #2038 as it worked fine before. |
No? Please familiarize yourself with the codebase more before making such accusations. The smallest a status in the main columns can be is somewhere around 320px, whereas the smallest a status can be in the left sidebar is closer to 285px. #2038 had nothing to do with changing the widths of anything. You didn't even test #2038 yourself, did you? |
Nope, as I'm busy with other stuff. Before: After: Default for comparison: After: |
|
this PR comment history is very long and I'm not sure if this was attempted a fix or not, but this issue seems to be still present: #1980 (comment) tested with Calckey, for reference. |
| def call(account, status, name) | ||
| reaction = StatusReaction.find_by(account: account, status: status, name: name) |
There was a problem hiding this comment.
| def call(account, status, name) | |
| reaction = StatusReaction.find_by(account: account, status: status, name: name) | |
| def call(account, status, emoji) | |
| name, domain = emoji.split('@') | |
| custom_emoji = CustomEmoji.find_by(shortcode: name, domain: domain) | |
| reaction = StatusReaction.find_by(account: account, status: status, name: name, custom_emoji: custom_emoji) |
As of now this looks for shortcode@domain.tld in the database for remote emojis, making this fail as such entry cannot be found.
This suggestion basically does the same as react_service.rb, splitting the name and domain and also look for the custom emoji ID, making it possible to remove a reaction with a remote emoji.
|
I have rebased this branch to latest, applied the suggested fix above, and submitted a pull request to the author's repo, in the hopes of getting this merged sooner :) I've started using the feature on our instance and it's great 👍 |
|
This is another confirmation that the rebase and fixes @neatchee fixes and they are working on the instance I run. Also, confirmed working on Mastodon Glitch 4.1.0 as well. |
|
@Plastikmensch It's been a month with no response from the initial PR creator. Would you like to switch to a new PR from my repo? We're looking to squash one more bug (emoji picker does not stay in screen bounds and can be inaccessible off-screen) |
|
Oh well. I don't really have an explanation for staying away for so long, but I'm still more than willing to get this finished. After all, I'll have to do it either way for my own Mastodon instance. For starters, this is merely keepalive comment to let you know I am in fact not dead. I'll catch up with the conversation history when I get up in around 9 hours. My sincere apologies for the long silence. |
|
Life happens :) I'm glad you're still around. Let me know if I can help.
…On Fri, Feb 10, 2023, 10:30 PM anna ***@***.***> wrote:
Oh well. I don't really have an explanation for staying away for so long,
but I'm still more than willing to get this finished. After all, I'll have
to do it either way for my own Mastodon instance. For starters, this is
merely keepalive comment to let you know I am in fact not dead. I'll catch
up with the conversation history when I get up in around 9 hours. My
sincere apologies for the long silence.
—
Reply to this email directly, view it on GitHub
<#1980 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABA3WHZR3ZCZO2IYCTB3FY3WW4WXFANCNFSM6AAAAAASOJESE4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hi, this is not a review, but I just wanted to say I'm potentially interested in this change, but unfortunately do not currently have the mental bandwidth to review such a large PR. Hopefully this will improve sooner than later, but thank you for your contribution either way! |
|
Absolutely understandable. I know how that is.
It might be nice to get this into a branch under glitch-soc's project so
interested parties can pull and test while you're otherwise occupied. It
would improve visibility - I think fewer people check PRs and do the work
of setting up a new upstream compared to just switching branches.
What do you think?
…On Sun, Feb 12, 2023 at 9:13 AM Claire ***@***.***> wrote:
Hi, this is not a review, but I just wanted to say I'm potentially
interested in this change, but unfortunately do not currently have the
mental bandwidth to review such a large PR. Hopefully this will improve
sooner than later, but thank you for your contribution either way!
—
Reply to this email directly, view it on GitHub
<#1980 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABA3WH3MI7WEIASNRKBLR43WXEK27ANCNFSM6AAAAAASOJESE4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Brian "Neatchee" Resnik
|
|
@neatchee Any chance you could open a new PR with the updated code? This one seems stale/dead and I'd love to test/help fix bugs on the front-end if possible. Cheers |
|
Daily usage has actually exposed some bugs that I would definitely like to
see fixed before integration. Neither I nor @deanveloper have had time to
dig into it further but you can see them on the issue tracker at
GitHub.com/neatchee/mastodon
…On Wed, Feb 22, 2023, 7:45 PM Pedro ***@***.***> wrote:
@neatchee <https://github.com/neatchee> Any chance you could open a new
PR with the updated code? This one seems stale/dead and I'd love to
test/help fix bugs on the front-end if possible.
Cheers
—
Reply to this email directly, view it on GitHub
<#1980 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABA3WH5M2XKCW36UWPJ24U3WY3MNZANCNFSM6AAAAAASOJESE4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Is there any chance that this PR will start working again? |
|
I'm planning to submit a new PR for this since the original author seems to
not have time, but not until we've fixed one issue: the emoji picker is not
bound to the screen dimensions so it often can't be used as it shows up
off-screen, especially on mobile.
Once we've had a chance to fix and test that (and maybe iterate a little on
how the exact 'remain on screen' behavior is positioned, etc) I'll get it
all bundled up and submitted :)
|
|
New pull request is #2127 |
|
Highly suggest this e4c411e to be added. |
|
Ah, I messed that commit up. Apply 9c4e060 afterwards. |
|
Replaced by #2221 |
I'm running a custom fork of glitch-soc on my instance and recently implemented Akkoma-style emoji reactions for it. It has been running without issues so far, so I'm offering to merge the code with upstream because I believe it's quite a cool feature (and it would also make my diffs smaller :3).
Some details of how this works:
MAX_REACTIONSenvironment variable)The whole thing is based on announcement reactions, and I also took some architectural hints from the Fedibird fork.
I'm sorry if this description lacks some important details; I don't really do pull requests very often and never have for such a large commit. Feel free to ask for clarification.