-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Allow joining several hashtags in a single column #8904
Conversation
c8d5ace
to
8b2bdcf
Compare
Excellent video explainer... does this limit to hashtags filtered from the home feed and/or local feed and/or federated feed? this feature is worth it on it's own, but I'd like to see hashtags being pulled from the fediverse to get access to toots in and out of my social circle. if possible, I'd be nice to be able to filter in or out either locally or from the firehose. |
I have to admit to still be wrapping my head around the federation features of Mastodon, but this PR always respects 'local' variable that's passed through on the backend. So I believe the answer is 'If your normal hashtag column displays remote tweets, then the modified AND/OR/NOT column will as well', but there's nothing in here at the moment around going and fetching statuses from other instances, which would likely be out of scope for this particular PR. |
Totally understandable... thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, reviewed it again and found some issues. Most importantly, the streaming API is untouched, so even with all the column params, new statuses will only appear from the main hashtag.
Changing the streaming API to support such filtering is not straightforward and I don't have advice ready. Each streaming API consumer subscribes to one redis pubsub channel (well, the channels are multiplexed actually). But the the thing is, you can't really subscribe to a union of two channels. They aren't published in the same server tick either, there might be any delay.
Perhaps your only option is to subscribe to all individual streaming API channels in the web UI, and then filter on ingress based on the tags included in the JSON.
@@ -86,6 +87,10 @@ const deleteStatus = (state, id, accountId, references) => { | |||
return state; | |||
}; | |||
|
|||
const clearTimeline = (state, timeline) => { | |||
return state.updateIn([timeline, 'items'], list => list.filter(() => false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list => list.clear()
@@ -79,6 +79,17 @@ class Status < ApplicationRecord | |||
scope :including_silenced_accounts, -> { left_outer_joins(:account).where(accounts: { silenced: true }) } | |||
scope :not_excluded_by_account, ->(account) { where.not(account_id: account.excluded_from_timeline_account_ids) } | |||
scope :not_domain_blocked_by_account, ->(account) { account.excluded_from_timeline_domains.blank? ? left_outer_joins(:account) : left_outer_joins(:account).where('accounts.domain IS NULL OR accounts.domain NOT IN (?)', account.excluded_from_timeline_domains) } | |||
scope :tagged_with_all, ->(tags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess stylistically if you need a multiline lambda for a scope, the scope is better off being a class method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a subtle difference here in that in a class method, self
refers to the class itself (ie, Status, which would map to Status.all), whereas within a scope self
refers to the existing relation that the scope's being called on.
So a straight port to a class method doesn't quite work because instead of reducing the existing relation, we start reducing from all statuses
app/controllers/tags_controller.rb
Outdated
@@ -16,14 +16,16 @@ def show | |||
end | |||
|
|||
format.rss do | |||
@statuses = Status.as_tag_timeline(@tag).limit(PAGE_SIZE) | |||
@statuses = HashtagQueryService.new.call(@tag, params.slice(:any, :all, :none)).distinct.limit(PAGE_SIZE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If distinct
is called on all uses of the service, why not put it inside the service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it was because rails 5's or
method is verrrry picky about what kinds of clauses you can join via or (and distinct was blowing it up), but I believe we can pull it into the service by calling distinct
on both the original query and the 'or' one.
Okay great, I've patched up a couple little things you mentioned and will have a poke into the streaming stuff over the next while. An initial glance makes me think there are two possible solutions:
|
The screenshot you posted looks better, but I'm still wondering what would happen if you wanted to remove the "cats" hashtag while keeping all the other tags. Would you have to make an entirely new column? This would be inconvenient if you had many "additional" tags but only wanted to change the "primary". |
@trwnh Hm, I'm not really sure I'm following you here; there's not currently a function that I'm aware of to update the hashtag of a column (I suppose the workflow is 'make a new column, pin it, and unpin the old one'?), and I'm not sure it's within the scope of this PR to add it, since this is simply trying to add options to an existing column. You could actually do some odd stuff with these settings like 'cats OR dogs WITHOUT cats', which would end up being a slightly-less-performant 'dogs' column, but I wouldn't really want to encourage that behaviour. |
I mean, tagId is just a prop passed from column settings in the settings reducer, so it could be changed on the fly, I think |
Alright, I've updated this to do the second option here, which is
|
Great work on this feature, James. In addition, I like the inclusion of the video to help introduce the use case and initial implementation. Bravo! 🚀 |
Hey @Gargron , is there anything else I can do here to shore this one up? |
Thanks for the support on this @Gargron; I had a great first PR experience with Mastodon. Feel free to ping me if there are any issues or enhancements that come up around this and I'll be happy to take a look. |
It takes more than 4 seconds to load "fediplay OR np" so something about these queries isn't quite right. |
Oh and it seems to reload the column every time you navigate anywhere. I should open an issue for this. |
@gdpelican With reference to the issue here I was keen to watch the video that you are referring to in your post here. On clicking the video link, I get: |
Ref #6359
Watch this video for more details on intended functionality :)