feat: configure notification actions

[harshad1]

This PR addresses #3530

• Added a drag UI to select available actions and their order
• The Existing delete option has been updated to only be for summary notifications

[harshad1]

In the future we can have additional configurations for

1. Summary messages (only some actions make sense - delete, archive, spam, mark read)
2. Other surfaces (android wear) where the number of actions can be > 3

[wilcooo]

Great work! I hope to see this included in the app. I've some opinions about the details here.

I would remove the ability to drag the separation line. It might be slightly confusing since the line itself isn't an element on the notification.

Maybe even replace the grab icons with + and - depending on whether the option is included at the moment, to make it even more clear which options will be included. (clicking plus/minus moves it to the other side of the line).

The main objective here is including/excluding options and should be visually obvious. Reordering is secondary and can be explained in text (as you have already).

To make using the + and - buttons more consistent, you can remember the complete order of all 5 items. So that removing one and adding it again returns it to the same spot. e.g. the order can be EBACD, but only B and D are shown so what's visible in the UI is BD/EAC, and dragging will then be the only way to change the underlying order.

[ckardaris]

@wilcooo I will have to disagree.

I would remove the ability to drag the separation line. It might be slightly confusing since the line itself isn't an element on the notification.

Maybe even replace the grab icons with + and - depending on whether the option is included at the moment, to make it even more clear which options will be included. (clicking plus/minus moves it to the other side of the line).

While first reading your suggestion I was thinking that your suggestions make sense and would probably improve the UX. Less moving parts, so it would be clean. But only if the order of elements is fixed and non-configurable by the user. If re-ordering is allowed, then that would mean that there would be a "hidden" functionality when holding and sliding a line, which maybe not be very intuitive unless clearly explained in the top description.

In my opinion, the current UI with the "dimmed" color for the hidden options and normal color for the shown is intuitive enough. Taking your suggestions into consideration maybe we could have the separator not movable at all on its own and instead it could span the whole width of the screen with no margin left/right. This would mean that we would not be able to enable/disable multiple elements with one slide, but I don't consider this use case very useful anyway.

Having said that, @harshad1 have you considered how your solution behaves with regards to accessibility?

[harshad1]

Some options:

1. Keep the current UI, but remove the dragability of the line
2. Move to a checkbox centric approach - remove the line, add a checkbox per item and enforce a maximum of n (= 3) checked items.

Option 2 is similar to this UI I implemented in Markor (an open source text editor I contribute to)

@ckardaris I have not considered accessibility. Thanks for bringing that up. I'll investigate.

[harshad1]

I have made a few changes:

1. The bar behaves similarly but doesn't have a drag handle (should be clear that this isn't an item)
2. Added Accessibility helpers
3. Added star as an option
4. cleaned up the code (single source of truth)

The bar remains draggable. This makes it easier to control the number of actions, makes the code simpler etc

IMO this is now quite clear and usable

[harshad1]

Is there anything I can do to help move this forward?

This is my first Thunderbird-android contribution and I am unfamiliar with the SOP here.

ty!

[dani-zilla]

Thanks for putting this together! Just giving you an update, we’re having the UX team go over the new design. They’ll potentially have some feedback, so you should hold off on any changes until they review this.

My first consideration is that the new UI should be built in compose for any new screens. Just hold off on starting it until we have design feedback.

I do want to raise concerns over the accessibility of a drag & drop interface. Drag & drop looks intuitive, and creates a visually pleasing interface (which I think this one is), but users with vision and mobility impairments find it difficult to interact with drag & drop. TalkBack would have to be set up to read what’s above and below the line on any changes to the ordering or inclusion. Likely with a debouncer so we wait to make sure they’re not still editing quickly between read-outs. Also we’d want to consider using accessibility actions to make the drag and drop feature work as smoothly for someone using TalkBack or pointing aids as it would for the rest of our users.

My recommendation might be to go with something more akin to this Microsoft example. By using arrows to re-order items, we can make it easier to make individual changes to ordering one at a time, and also drop the requirement for a consistent touch to drag and reorder items. We could also consider making the reorder buttons only available above the line, as they’re the only ones that it would matter for reordering, by using the previously suggested checkbox design. If an item is checked off, it appears above the line, with the other checkboxes disabled after 3 are selected.

However, I strongly recommend waiting until UX gets back to us with what they think. I’d hate for you to have to make more changes again. Just wanted to plant some ideas on what I was thinking here along with the update. Thanks again for this!

[harshad1]

@dani-zilla thanks!

For reference I did add the following

• Each action row and the cutoff divider exposes TalkBack actions “Move up” and “Move down” via ViewCompat.replaceAccessibilityAction, so reorder works without drag.
• The cutoff row has a contentDescription, and the drag handle has its own description.
• Actions trigger the same move logic as drag, respecting the max cutoff position.

Thanks for the example. I'll look into it when UX gets back.

[laurelterlesky]

Hi all, Design Team chiming in here. I don't have anything more to add on top of what's been said already. Let's get this out there and then see if we need to further iterate. Thanks for the contributions!

[harshad1]

@dani-zilla is there anything I need to do to enable workflows? Ty

[dani-zilla]

Enabled the workflow. Looks like there are a few issues with Spotless, Detekt, and tests.

We also aren't introducing new features with XML views, opting instead for Jetpack Compose for views. Now that we know the design is good, I think it's safe to tackle that.

[harshad1]

@dani-zilla It appears that workflows need to be approved again.

I have moved the UI to compose and fixed the CI issues (I believe)

[dani-zilla]

Thanks for continuing to work on this! Some small nits as well as a suggestion to convert more of the view to compose. Thanks!

[dani-zilla]

These should use the constants defined above so the string doesn't have to be changed if the keys change or are added to

[dani-zilla]

Would this potentially be better as a List rather than a comma-separated string? My guess is that it's to make it easier to write to preferences as a string. Though it might be easier to only do the parsing on read/write, rather than when passing the data around and making changes.

[dani-zilla]

Noted this above with the default order constant, but what is the purpose of doing this as a string instead of keeping it as a list?

[dani-zilla]

This does seem to mean we define the default order in two places. Seeing as this class already depends on the NotificationActionTokens, maybe it could just parse the string or expose the list here?

[dani-zilla]

Not a fan of setting constants in the code like this. I'd say either define it as a constant or we could set the minimum list item height a different way

[dani-zilla]

Constants for the items should be either made as constant somewhere or not used and let the relation to the other elements control the size.

You could consider breaking this out into its own composable and using it that way. We do use an atomic structure, where we break down screens into atoms, the smallest reusable parts, and build up from there. It's described here. This could allow the constants to live in their atomic components, and will shrink the implementation of those components in a screen as well.

[dani-zilla]

I definitely would prefer to see the components split out and templated for easier re-use.

[dani-zilla]

I was hoping to see the entire view move to compose. Replacing inflated views and recycler views with lazy columns, that sort of thing. That would reduce the complexity and file size as well.

[harshad1]

just FYI for any watching - I'm still working on this and do plan to address comments and push things through as time permits

[harshad1]

Single source of truth for default order

[harshad1]

• Order tokens now come from NotificationActionTokens, and parsing is centralized.

  • messageActionsOrder is now a list in the preference model; we only serialize at the storage boundary.
  • messageActionsOrder is a List in NotificationPreference. Parsing happens only in DefaultNotificationPreferenceManager

• Min height uses MainTheme.sizes.iconAvatar inside composables.

• Tried full Compose; drag was janky and I couldn't get it to animate / move smoothly (but I am not familiar with compose)

  • Reverted to RecyclerView + ItemTouchHelper

[dani-zilla]

* Tried full Compose; drag was janky and I couldn't get it to animate / move smoothly (but I am not familiar with compose)
  
  * Reverted to RecyclerView + ItemTouchHelper

Thanks for giving it a shot. So, we don’t want to merge anything using old xml views. Obviously doing small modifications to existing views is fine, and we wouldn’t currently ask anyone submitting changes to a view to update it to Jetpack Compose unless it was a large change, however, for all new views, we’re using Compose with an "atomic" design system. More info on that is here.

I think we could look at two options here. The first is to do it in compose, jankiness and all, and work to smooth it out before merging. We can figure out what’s causing the stuttering behavior and see if we can fix it. Most likely just a mutable data object or some other item causing frequent recomposing. I can’t tell what the issue may have been because it doesn’t look like it was committed, but we can look into it if you want to give it another go.

The second option is to change the way we're displaying this list of options entirely, if the drag & drop itself is the issue. We could go back to the older idea of using arrow buttons. This is a more static event, with each item only being reordered by one place at a time, and could likely reduce the jank. It might also work better for people using TalkBack.