Skip to content

Add Unread Divider#60

Open
thgoebel wants to merge 3 commits into
binwiederhier:mainfrom
thgoebel:unread-divider
Open

Add Unread Divider#60
thgoebel wants to merge 3 commits into
binwiederhier:mainfrom
thgoebel:unread-divider

Conversation

@thgoebel

Copy link
Copy Markdown
Contributor

This PR adds a divider after the unread messages. Just like in messaging apps.

The exact string ("You are all caught up!") is open for discussion. Maybe something more translation-friendly. E.g. Slack on iOS has "You're up-to-date 🎉" when you scroll up. Signal has "1 Unread Message".

Also, the string is hardcoded in XML right now because I don't know how your string/translation workflow works. I'll leave it to you to properly integrate it into the string setup.

thgoebel added 3 commits June 16, 2023 15:57
The only real content changes are:

1. The companion object
2. Formatting the two constructors to be multiline instead of one looong line

Otherwise it's copy and paste of the DetailViewHolder and the imports that it needs.
@binwiederhier

Copy link
Copy Markdown
Owner

Wow. This is from June and I haven't commented or merged. My apologies. I haven't worked on the Android app in a while. When I do, I'll certainly look at this and/or merge it. Looks very neat. Thank you for your contribution!!

@thgoebel

thgoebel commented Sep 1, 2023

Copy link
Copy Markdown
Contributor Author

No problem! I’m running my fork, so I already have the unread divider and am happy :)

@binwiederhier

Copy link
Copy Markdown
Owner

I like the divider, though I don't understand the implementation and/or why it has to be so involved. Are there other things that you are doing in this that aren't strictly necessary? I didn't look too closely -- maybe I should re-read the PR ..

@thgoebel

Copy link
Copy Markdown
Contributor Author

The problem is that RecyclerViews/Adapters/ViewHolders are nice as long as you only have a single view type, but as soon as you have multiple view types, things get messy.

An alternative would be to rewrite the screen in Jetpack Compose, where lists with mixed item types result in a lot cleaner/simpler/shorter code. But I didn't want to go there when I originally authored the PR.

Explanation

To explain the changes:

  • The first two commits are just refactoring, to move the classes for the details UI code into its own package.
  • The third commit is where the new unread divider logic is. It adds another ItemViewType for the divider. The adapter list can then hold multiple DetailItems, either of type NotificationItem or of type UnreadDividerItem. Each of these item types has its own ViewHolder that is responsible for rendering the item.

Going forward

The radical option is to drop this PR entirely, and rewrite the screen in Jetpack Compose.

If we want to keep the RecyclerView and this PR:

  • Most of the conflicts are in the first commit (a12eb78).
  • I think it is easiest to just redo the same changes from this commit manually on top of the latest main, instead of trying to rebase.
    • That is, move class DetailViewHolder out of DetailAdapter.kt and into DetailViewHolder.kt.
    • Then the other two commits should cherry-pick more cleanly.

I agree that reviewing the change that moves class DetailViewHolder into its own file is unwieldly. The diff is not easy. So I propose that you do this refactor yourself. Then you can be convinced that it is "correct", and I didn't sneak in something fishy.
Then once DetailViewHolder is in its own file, I can do the rest (rebase the actual logic, and fix up conflicts).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants