#644, Recursive filtering

[K1rakishou]

Closes #644.

Added two new menu items - "Hide" and "Hide whole chain". "Hide" is for hiding separate posts/threads whereas "Hide whole chain" is for hiding posts with all replies to those posts. OP post cannot be hidden from a thread (only from the catalog and in this case the whole thread will be hidden).

[Adamantcheese]

This has LOTS of issues with crashing, especially when scrolling fast through a thread or reaching the last post in a thread (in which case it crashes immediately when a thread has only a few posts and can't scroll yet). As a note, you've removed a lot of the synchronized checks in the database methods which may be causing it.

[K1rakishou]

@Adamantcheese Alright, I will look into it today.

[K1rakishou]

When I tested this I had crashes in this place so I kinda closed it up with that check. Maybe it is related.
https://github.com/Floens/Clover/pull/654/files#diff-c43fe9f6e874c2b42ffaf3822bf843edR107

[Adamantcheese]

It will also crash here with the opposite error as the one you fixed. https://github.com/K1rakishou/Clover/blob/(%23644)-recursive-filtering/Clover/app/src/main/java/org/floens/chan/ui/adapter/PostAdapter.java#L129
i.e.

if (!(holder instanceof StatusViewHolder)) {
                    // to avoid "PostViewHolder cannot be cast to StatusViewHolder" exception
                    return;
}

will also "solve" a problem, but the crashes will continue. It's likely that these two checks aren't needed and there's something else wrong with your code.

[K1rakishou]

@Adamantcheese I think I figured out what is the problem.

https://github.com/K1rakishou/Clover/blob/(%23644)-recursive-filtering/Clover/app/src/main/java/org/floens/chan/ui/adapter/PostAdapter.java#L198

Here displayList is being accessed on the background thread and then there is the notifyDataSetChanged which will trigger onBindViewHolder to be called and it will access the displayList on the main thread. So, to fix this displayList should be synchronized. We can probably completely remove sourceList since it's not being used anywhere besides this place (and kinda pointless). The loop before notifyDataSetChanged call should be synchronized as well. This check (https://github.com/K1rakishou/Clover/blob/(%23644)-recursive-filtering/Clover/app/src/main/java/org/floens/chan/ui/adapter/PostAdapter.java#L107) is not needed anymore and should be removed as well. Other places that access displayList should be synchronized as well. I will push the fixes a little bit later today, have some stuff to do. Meanwhile you can just use this patch.

#654_fix.zip

[Adamantcheese]

It's about 2AM for me, so whenever you get to it, I'll test it and make sure it's good before putting it in.

Although why does this need to be done in a background thread? The current version works fine with it running on the main thread and the synchronization is going to be a pain in the ass to make work correctly (as I figured out when trying to resolve this).

If I remember right, most of the crashes were due to IndexOutOfBounds exceptions. It also looks like another reason it might fail is here where the background thread is resetting the indicator position before calculating it, instead of using a thread-safe local variable to calculate it before setting it. That -1 being set for (relatively) quite a while, considering the holder stuff seems to wait in nanoseconds before failing, would explain those exceptions. If you do choose to continue using a background thread, you should probably double check that because it might cause crashes still.

[K1rakishou]

@Adamantcheese

Although why does this need to be done in a background thread?

Before this task you could only hide threads. Hidden threads were stored in the database as well as in a cache variable for fast look-ups (https://github.com/Floens/Clover/pull/654/files#diff-d4c6b656f34af7234a6390b559ec3688L21). That's why it could work safely on the main thread. But it could only hold 250 threads at a time. After I've implemented the ability to hide posts as well as threads the number of 250 hidden posts became kinda small. So I've removed those caches and instead filtering is now reading hidden posts directly from the database. And now it can't run on the main thread since it will just crush (or just hung for a while if there is an ability to disable throwing an exception when accessing database from the main thread in the orm lite).

I think you are right that lastSeenIndicatorPosition may cause different crashes so what I think to do is to completely extract this code from the adapter (it shouldn't be there in the first place) to a different place and then just assign the filtering result into the adapter on the main thread. I'm gonna work on it later today.

[K1rakishou]

Alright. This should fix it.

[ghost]

Hey @K1rakishou , some threads hide some don't.
The message "Thread hidden" appears but the threads remains and is accesible.

DISREGARD WHAT I WROTE ABOVE.
I guess there was something wrong with the database because I deleted it and now everything is working as intended.

Also, could you make it that instead of removing the replies they would just hide?
Dashchan does it like you hide them but they stay in place and if you click them they appear again.

[Astridchan]

Wouldn't it make sense to add options for "remove whole chain" and "hide whole chain" in the filter settings? As currently the recursive filtering feature is only available when manually hiding posts.

Also, Muyli above has a point regarding hiding vs. removing posts. Currently the options in filter settings are "hide" and "remove", where "hide" will gray out the post and "remove" will completely remove it. But when you click the three button menu on a post, the "hide" option doesn't "hide" the post, it removes it. So the same term "hide" is being used for two different actions, which I think can lead to confusion. My suggestion would be to add both "remove" and "hide" in the post menu, and to keep the menu from getting cluttered, instead of having a "hide whole chain" and "remove whole chain" option separately, you would get a Y/N prompt "hide/remove whole chain?" when pressing to hide or remove a post with replies.

[K1rakishou]

@MyuLi @Astridchan
So, to sum it up. There have to be two menu items - "Hide" which will grey out a post and "Remove" which will completely remove a post like it is implemented right now. Upon clicking a post if it has replies to it a dialog window should pop up suggesting the ability to hide/remove the whole chain. Clicking on hidden (not removed) post should bring it back up. Is this correct?

Wouldn't it make sense to add options for "remove whole chain" and "hide whole chain" in the filter
settings? As currently the recursive filtering feature is only available when manually hiding posts.

Wouldn't people start complaining that posts are disappearing without any reason because they forgot this option is turned on?

[Astridchan]

@K1rakishou

So, to sum it up. There have to be two menu items - "Hide" which will grey out a post and "Remove" which will completely remove a post like it is implemented right now. Upon clicking a post if it has replies to it a dialog window should pop up suggesting the ability to hide/remove the whole chain. Clicking on hidden (not removed) post should bring it back up. Is this correct?

Exactly.

Wouldn't people start complaining that posts are disappearing without any reason because they forgot this option is turned on?

Well that's on them, isn't it?

I'll try to explain why I think this feature would be useful, and why I would want to see it added. Basically, in a few of the generals I frequent, there are these copy-pasta shitposts that get posted in every new thread for (You)s. I've got those posts filtered so that they're automatically removed, but the replies remain, so I still end up seeing the shitposts when I follow back the reply chains. A "remove whole chain" would make it so both the shitpost and the replies to it get removed.

EDIT:
One thing I forgot to mention is that currently, if I choose "remove whole chain" on a post, new replies to that post will still show up. To me it would make sense that any subsequent reply to a post that has been removed/hidden should also be removed/hidden.

EDIT2: I'll give an example for this too. Say someone posts one of those "Roll for X" images that tend to get a lot of "roll" replies. Currently, if you chose "remove whole chain" you'd only remove the post and the replies made up to that point, but all future "roll" posts will still show up. What you'd want to happen is that all current and future replies to the post get removed.

[K1rakishou]

The last commit is Work In Progress but you can already test it if you would like (I would really appreciate it). But do not merge it yet.

It "should" work.
For now all posts that have replies to a post that was matched by a filter will be hidden (greyed out). I'm gonna need to store the filter id along with the posts to figure out what to do with such a post (hide/remove or highlight) when filtering them.

[Adamantcheese]

I'll take a look at what changed in the db and do what I need to for import/export. I have an exported file for the last version that I can test with.

[Adamantcheese]

@K1rakishou looks like from the last build of mine to the most recent one, no import/export changes needed to be made so the import/export version stayed the same. I've also already replaced all the database variable names in my build with their respective strings.

[K1rakishou]

@Adamantcheese I see you have removed them from everywhere. I actually meant to just remove them from this class (https://github.com/Adamantcheese/Clover/blob/92a7e6d05778c6f304b4118be9c9de40d519a9e7/Clover/app/src/main/java/org/floens/chan/core/database/DatabaseHelper.java) because they should never change in migrations to avoid crashes, but keep them everywhere else to avoid mistakes when typing them by hand every time. And it is really helpful when you want to rename some column because you will only have to rename it in one place.

[Adamantcheese]

I figured if everything was hardcoded it would be better and these names shouldn't really ever change.

[K1rakishou]

@Adamantcheese Well normally yes, they should not change. Well I guess it's fine.

[K1rakishou]

Regarding the importing/exporting I guess it is fine too because we have just merged the importing/exporting so there is just nothing to import from.

[Adamantcheese]

The import/export was merged before this last set of changes you made, and the GSON class maker thing defaults the field to false if it didn't exist, so there's no upgrade needed.

[Adamantcheese]

Filter has an applyToReplies variable that is annotated as a database field, but the database column name specified in DatabaseHelper is apply_to_replies. If the actual variable name doesn't match the column name, you MUST specify the column name in the @DatabaseField annotation, otherwise the database engine will take the literal variable name as the column name. See this commit for the fix.

Not only did this crash my build several times with weird, confusing errors, but also you'd never even be reading from the right table!

[Adamantcheese]

I meant to point to this line of code, sorry.

[K1rakishou]

Done.

[K1rakishou]

@Adamantcheese Damn, I guess I forgot to specify the name in the annotation. I wonder why it didn't crash for me when I was testing it.

[ghost]

Yo @K1rakishou can you make it that hidden threads or replies unhide when you click on them?

[K1rakishou]

@MyuLi Sure, but only for the posts that you hid by yourself and not by the filters. That's because of how filters work right now. I guess it should also be implemented for removed threads/posts.

[K1rakishou]

Alright. You can now unhide threads/posts by clicking on them.
There is also a new menu item that will show all posts that were removed in a thread.

[ghost]

@K1rakishou Thanks, man.
@Adamantcheese Can you add this?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.