-
-
Notifications
You must be signed in to change notification settings - Fork 390
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
Support the mastodon 4 filter api #3188
Conversation
First visual impressions:
Or, display them as filter chips (https://m3.material.io/components/chips/guidelines#8d453d50-8d8e-43aa-9ae3-87ed134d2e64).
I'll mock up the UI changes directly so you can see what I mean, and send them as a patch. |
https://github.com/nikclayton/Tusky/commit/4a8c267ee88ab1ae70fd8eb3e6f0eb7db33f50c3 is a WIP that adjusts the padding and uses chips. There's still some layout stuff to do, and I haven't tried my other proposals yet because it's late here. Here's a screenshot with 2 "whole word" keywords and one that isn't. |
Another thought -- the screen before this one (i.e., the one that lists all the filter names) is currently a list of single-line items. That could probably be a list of two-line items, where the second line displays some key information about the filter, like what it does, duration, what it affects. Or maybe three lines, and show some of the things it's filtering too? Quick layout mock: Not sure, to be honest, that might be overloading the user with too much information on that screen. |
WRT the labels, I based the layout and labels on the web UI - we probably shouldn't diverge too much from them on naming, or people will get confused about what thing in Tusky corresponds to what thing in the web UI. WRT the list, the web UI does have a richer list, but it also uses a lot of vertical space for each item, as well as assuming the availability of more horizontal space 🤷 Honestly, I'm not married to any of the UI, but the thing I couldn't actually get working was scrolling the edit activity |
I think the scrolling problem is because nothing was constrained to the bottom of the screen, so the layout is free to overflow the screen without needing to scroll. I'll have a fix for that and some more layout stuff shortly. |
https://github.com/nikclayton/Tusky/commits/filter-chips has a new commit that does scrolling and a few other things. There's full notes in the commit message. Latest screenshot: In this example, the screen is scrollable, so the user can scroll up to set the rest of the contexts: |
Overall, this is excellent, thank you! I've been looking forward to this ever since Mastodon merged "filter as CW" into upstream.
Though it could be useful for some, I'd discourage implementing that. Folks may set up filters to manage topics that they are currently negatively overwhelmed by (e.g. hate speech, politics, ex-friends/partners, etc), and if Tusky shows some of the actual filtered words in the "here's all your filters" list view, there's no way to view or edit any other filters without risking being reminded. Instead, I'd suggest sticking with the two line summary. If a third line is needed, perhaps filter expiration (if set) and/or creation/edit date if available. |
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.
Very nice 😍
android:layout_weight="1" | ||
android:fillViewport="true"> | ||
|
||
<androidx.appcompat.widget.LinearLayoutCompat |
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.
LinearLayout
everywhere please
(LinearLayoutCompat
was relevant to access newer features on really old Androids, like 4.something)
@@ -59,7 +59,7 @@ class TimelinePagingAdapter( | |||
} | |||
else -> { | |||
val view = LayoutInflater.from(viewGroup.context) | |||
.inflate(R.layout.item_status, viewGroup, false) | |||
.inflate(R.layout.item_status_wrapper, viewGroup, 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.
Wouldn't it be better to have another viewtype "VIEW_TYPE_FILTERED_STATUS" instead? (less stuff to inflate, less layouts hanging around at runtime)
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 we do that, will we still be able to dynamically show the status when the user clicks "show anyway" ?
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 think so
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.
Oh, now I see what you mean - I completely misinterpreted what you meant before 🤦
import com.keylesspalace.tusky.util.getRelativeTimeSpanString | ||
|
||
class FiltersAdapter(context: Context, val listener: FiltersListener, filters: List<Filter>) : | ||
ArrayAdapter<Filter>(context, R.layout.item_removable, filters) { |
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.
Can we use RecyclerView and RecyclerViewAdapter instead please? ListView/ArrayAdapter are really old apis and with RecyclerView we would also get nice animations when an element gets deleted.
@@ -203,6 +203,9 @@ AND timelineUserId = :accountId | |||
) | |||
abstract suspend fun deleteAllFromInstance(accountId: Long, instanceDomain: String) | |||
|
|||
@Query("UPDATE TimelineStatusEntity SET filtered = NULL WHERE (serverId = :statusId OR reblogServerId = :statusId)") | |||
abstract suspend fun clearWarning(statusId: String): Int |
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.
Needs to also check for the correct accountId as to not accidentally affect statuses from othr accounts with the same id.
import java.util.Date | ||
import javax.inject.Inject | ||
|
||
class EditFilterActivity : BaseActivity() { |
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.
Would be nice to have ViewModels for this and FiltersActivity so they can handle screen rotation without reloading
@@ -0,0 +1,54 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<androidx.constraintlayout.widget.ConstraintLayout |
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.
weird filename - maybe better "item_filter"?
} | ||
|
||
companion object { | ||
private const val TAG = "EditFilterActivity" |
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.
unused
|
||
// Mastodon *stores* the absolute date in the filter, | ||
// but create/edit take a number of seconds (relative to the time the operation is posted) | ||
fun getSecondsForDurationIndex(index: Int, context: Context?, default: Date? = null): Int? { |
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.
default
is never set anywhere? Except in tests.
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.
please resolve the conflicts
Nested scrolling views (e.g., an activity that scrolls with an embedded list that also scrolls) can be difficult UI. Since the list of contexts is fixed, replace it with a fixed collection of switches, so there's no need to scroll the list. Since the list of actions is only two (warn, hide), and are mutually exclusive, replace the spinner with two radio buttons. Use the accent colour and title styles on the different heading titles in the layout, to match the presentation in Preferences. Add an explicit "Cancel" button. The layout is a straightforward LinearLayout, so use that instead of ConstraintLayout, and remove some unncessary IDs. Update EditFilterActivity to handle the new layout.
What's done:
warn
filter actionOutstanding:
The edit filter activity needs to be made scrollableCleanup in the timeline codeProbably UI tweaking