pleroma/pleroma-fe
Watch
2
2
Fork
You've already forked pleroma-fe
 10
Code Issues 351 Pull requests 26 Projects Releases 5 Packages 1 Wiki Activity

Properly detect thread-muted posts and set with_muted when fetching notifications #2208

Closed
rinpatch wants to merge 0 commits from gitlab-mr-iid-941 into develop
pull from: gitlab-mr-iid-941
merge into: pleroma:develop
Conversation 23 Commits - Files changed -
rinpatch commented 2019-09-04 09:21:14 +00:00
Member
Copy link
No description provided.
hj commented 2019-09-04 10:19:20 +00:00
Member
Copy link

this should always be true otherwise you'll have to reload UI to get all the notifications you've been missing if you toggle that option off

this should always be true otherwise you'll have to reload UI to get all the notifications you've been missing if you toggle that option off
hj commented 2019-09-04 10:19:30 +00:00
Member
Copy link

why not both?

why not both?
hj commented 2019-09-04 10:20:17 +00:00
Member
Copy link

again, this will make old user's posts unaffected by toggling the setting, only newly fetched ones

again, this will make old user's posts unaffected by toggling the setting, only newly fetched ones
rinpatch commented 2019-09-04 10:29:52 +00:00
Author
Member
Copy link

Well, timelines have the same logic. I don't think it should always be true because it would lead to inconsistent behavior of "Load older notifications" (e.g be returns 20 notifications but fe can't show any of them because they are from a muted thread) and would cause additional be load for no good reason. Instead the notifications should be reloaded when the option is changed, though I have no idea how to do that.

Well, timelines have the same logic. I don't think it should always be true because it would lead to inconsistent behavior of "Load older notifications" (e.g be returns 20 notifications but fe can't show any of them because they are from a muted thread) and would cause additional be load for no good reason. Instead the notifications should be reloaded when the option is changed, though I have no idea how to do that.
rinpatch commented 2019-09-04 10:33:17 +00:00
Author
Member
Copy link

right, I will change that. Also note that old thread muted posts are not affected as well, ideally we would update the state of all posts with the same context after muting/unmuting but I have no idea where to put it.

right, I will change that. Also note that old thread muted posts are not affected as well, ideally we would update the state of all posts with the same context after muting/unmuting but I have no idea where to put it.
hj commented 2019-09-04 10:45:16 +00:00
Member
Copy link

if we had something like actual thread_muted we could probably keep a Set of muted ConversationIDs.

another option would be re-fetching the context of a post after muting and doing addNewStatuses with them.

assuming that BE will update muted property on them in timely fashion.

if we had something like actual `thread_muted` we could probably keep a Set of muted ConversationIDs. another option would be re-fetching the `context` of a post after muting and doing `addNewStatuses` with them. assuming that BE will update `muted` property on them in timely fashion.
hj commented 2019-09-04 10:47:23 +00:00
Member
Copy link

weird, that also be always true if you ask me. Loading next 20 notifications should also be accounted for hidden notifications.

if anything, such things should probably be controlled by some (new) option like "Use less data at cost of timeline consistency"

weird, that also be always true if you ask me. Loading next 20 notifications should also be accounted for hidden notifications. if anything, such things should probably be controlled by some (new) option like "Use less data at cost of timeline consistency"
rinpatch commented 2019-09-04 10:53:30 +00:00
Author
Member
Copy link

another option would be re-fetching the context of a post after muting and doing addNewStatuses with them.

Yeah, let's do this one.

assuming that BE will update muted property on them in timely fashion.

It will, the bug that caused it not to do so is fixed and merged in master.

> another option would be re-fetching the `context` of a post after muting and doing `addNewStatuses` with them. Yeah, let's do this one. > assuming that BE will update `muted` property on them in timely fashion. It will, the bug that caused it not to do so is fixed and merged in master.
rinpatch commented 2019-09-04 10:55:57 +00:00
Author
Member
Copy link

Actually no, the first one would probably be better. People usually mute hellthreads that have 5mb responses, so the delay will be noticeable, especially on a bad connection

Actually no, the first one would probably be better. People usually mute hellthreads that have 5mb responses, so the delay will be noticeable, especially on a bad connection
rinpatch commented 2019-09-04 10:58:18 +00:00
Author
Member
Copy link

so, for now, should I set it to true for timelines as well, set it to true only in notifs, or leave it as is?

so, for now, should I set it to true for timelines as well, set it to true only in notifs, or leave it as is?
hj commented 2019-09-04 10:59:04 +00:00
Member
Copy link

i'd say leave it as is for consistency sake

i'd say leave it as is for consistency sake
hj commented 2019-09-04 11:00:04 +00:00
Member
Copy link

indeed.

Keeping a mutedConversationIds set should be easy, IIRC there is thread_muted in API, so when adding a post to vuex store, if post has thread_muted: true - add its conversation id to the set, if it's false - remove from it (if there's any).

in this very same line, check if that set has this conversationid or not.

indeed. Keeping a `mutedConversationIds` set should be easy, IIRC there is `thread_muted` in API, so when adding a post to vuex store, if post has `thread_muted: true` - add its conversation id to the set, if it's `false` - remove from it (if there's any). in this very same line, check if that set has this conversationid or not.
rinpatch commented 2019-09-04 11:04:46 +00:00
Author
Member
Copy link

IIRC there is thread_muted in API

there isn't, but I will add one.

> IIRC there is `thread_muted` in API there isn't, but I will add one.
rinpatch commented 2019-09-04 16:56:33 +00:00
Author
Member
Copy link

@hj I did just that, but because the computed properties are cached, only posts of the user who made the post on which the mute button was clicked are updated. Should this be fixed by dispatching an action to update muted in setMuted?

@hj I did just that, but because the computed properties are cached, only posts of the user who made the post on which the mute button was clicked are updated. Should this be fixed by dispatching an action to update muted in `setMuted`?
hj commented 2019-09-04 17:07:57 +00:00
Member
Copy link

to answer the question in other thread, i originally suggested using a Set, not array, since using Set (or Map or plain object with keys) would be O(1) (Set.get(key) / Map.get(key) / object[key]) instead of O(n) (findIndex(key))

Instead, since there's a problem with computed property not updating, I would suggest using conversationsObject which i forgot already exists and iterate over it like so:

newStatus.thread_muted = status.thread_muted

conversationsObject[newStatus.statusnet_conversation_id].forEach(status => status.thread_muted = newStatus.thread_muted)

or something along those lines.

to answer the question in other thread, i originally suggested using a `Set`, not array, since using `Set` (or `Map` or plain object with keys) would be O(1) (`Set.get(key)` / `Map.get(key)` / `object[key]`) instead of O(n) (`findIndex(key)`) Instead, since there's a problem with computed property not updating, I would suggest using `conversationsObject` which i forgot already exists and iterate over it like so: ```js newStatus.thread_muted = status.thread_muted conversationsObject[newStatus.statusnet_conversation_id].forEach(status => status.thread_muted = newStatus.thread_muted) ``` or something along those lines.
rinpatch commented 2019-09-04 18:12:29 +00:00
Author
Member
Copy link

Done

Done
hj commented 2019-09-04 18:26:51 +00:00
Member
Copy link

nitpick: i'd check newStatus.thread_muted === undefined to avoid iterating if BE doesn't support thread_muted yet

nitpick: i'd check `newStatus.thread_muted === undefined` to avoid iterating if BE doesn't support thread_muted yet
hj commented 2019-09-04 18:29:49 +00:00
Member
Copy link

codewise looks good, just needs some testing with BE that supports it...

codewise looks good, just needs some testing with BE that supports it...
rinpatch commented 2019-09-04 19:07:44 +00:00
Author
Member
Copy link

patch.cx has the extension deployed

patch.cx has the extension deployed
shpuld commented 2019-09-06 04:12:29 +00:00
Member
Copy link

lgtm

lgtm
shpuld commented 2019-09-16 09:31:47 +00:00
Member
Copy link

@rinpatch you've had this running for a while, ready to merge?

@rinpatch you've had this running for a while, ready to merge?
rinpatch commented 2019-09-16 11:37:57 +00:00
Author
Member
Copy link

yes

yes
rinpatch commented 2019-09-16 11:38:51 +00:00
Author
Member
Copy link

@shpuld resolved the merge conflicts.

@shpuld resolved the merge conflicts.

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
BE Request

   
Backlog

   
Better behavior

   
Blocked by deps

   
Bug

   
Code Review

   
Discussion

   
Doing

   
Feature Request / Enhancement

   
In the shining bright future maybe

   
It's complicated

   
MS Edge

   
Mememoon (or other niche browsers)

   
Missing API

   
Missing feature

   
Missing l10n/i18n

   
NL1
Instance Admin Dashboard

  
NL2
Improved Mobile Experience

  
NL4
Improved instance customization and QoL options

  
Need to verify on develop

   
RELEASE BLOCKER

   
Reassign or Close

   
Refactor

   
Regression

   
Safari

   
To Do

   
User story

   
Waiting on godot

   
accessibility
https://git.pleroma.social/pleroma/pleroma-fe/-/snippets/7746

  
better documentation

   
chore

   
confirmed

   
easy ticket

   
incident
Denotes a disruption to IT services and the associated issues require immediate attention

  
l10n update

   
mastoapi

   
mobile

   
needs design

   
needs-info

   
needs-review

   
stupid
No labels
BE Request
Backlog
Better behavior
Blocked by deps
Bug
Code Review
Discussion
Doing
Feature Request / Enhancement
In the shining bright future maybe
It's complicated
MS Edge
Mememoon (or other niche browsers)
Missing API
Missing feature
Missing l10n/i18n
NL1
NL2
NL4
Need to verify on develop
RELEASE BLOCKER
Reassign or Close
Refactor
Regression
Safari
To Do
User story
Waiting on godot
accessibility
better documentation
chore
confirmed
easy ticket
incident
l10n update
mastoapi
mobile
needs design
needs-info
needs-review
stupid
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
pleroma/pleroma-fe!2208
No description provided.