Skip to content

591: Add groups to user info, with sections and using RecyclerView for future expandability#16769

Open
penguin86 wants to merge 11 commits intonextcloud:masterfrom
penguin86:feature/591-groups-in-user-info-rv
Open

591: Add groups to user info, with sections and using RecyclerView for future expandability#16769
penguin86 wants to merge 11 commits intonextcloud:masterfrom
penguin86:feature/591-groups-in-user-info-rv

Conversation

@penguin86
Copy link
Copy Markdown

Implementation of #591

Added groups and implemented sectioning in RecyclerView. Meant for adding further groups if needed in the future.
Based on the suggestions by Andy in #16767 (comment)

🖼️ Screenshots

🏚️ Before 🏡 After
before after_new

🏁 Checklist

  • Tests written, or not not needed

Copy link
Copy Markdown
Collaborator

@alperozturk96 alperozturk96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello

Thank you for the PR.

import com.owncloud.android.databinding.UserInfoDetailsTableItemTitleBinding
import com.owncloud.android.utils.theme.ViewThemeUtils

class UserInfoAdapter(val context: Context, val mDisplayList: Map<Int, MutableList<UserInfoDetailsItem>>, val viewThemeUtils: ViewThemeUtils) :
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why mDisplayList needs to have mutable list? Adapter should just display the list.

Also since this is a new class; mDisplayList can we rename it to the displayList?

const val SECTION_GROUPS = 1
}

class UserInfoDetailsItem(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UserInfoDetailsItem this can be a data class. For item of the adapter better (so that we can compare easily if necessary) to use data class but it's a small thing.

}

override fun onBindViewHolder(
holder: SectionedViewHolder?,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SectionedViewHolder? we can make this non-nullable.

relativePosition: Int,
absolutePosition: Int
) {
val item = mDisplayList[section]?.get(relativePosition)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the extract item position logic to internal enum class.

private enum class ItemPosition {
  SINGLE, FIRST, MIDDLE, LAST;

  fun backgroundRes(): Int = when (this) {
    SINGLE -> R.drawable.rounded_corners_listitem_single_background
    FIRST -> R.drawable.rounded_corners_listitem_first_background
    LAST -> R.drawable.rounded_corners_listitem_last_background
    MIDDLE -> R.drawable.rounded_corners_listitem_center_background
  }
}

private fun resolvePosition(position: Int, count: Int): ItemPosition {
  val isFirst = (position == 0)
  val isLast = (position == count - 1)

  return when {
    isFirst && isLast -> ItemPosition.SINGLE
    isFirst -> ItemPosition.FIRST
    isLast -> ItemPosition.LAST
    else -> ItemPosition.MIDDLE
  }
}

So that we can have simpler onBindViewHolder

Usage:

val count = getItemCount(section)
val positionType = resolvePosition(relativePosition, count)
val uiHolder = holder as UserInfoSectionedViewHolder
val item = mDisplayList[section]?.get(relativePosition)

// Set background
uiHolder.binding.root.setBackgroundResource(
  positionType.backgroundRes()
)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @alperozturk96 , thank you for spotting the first 3 and thank you for the very sleek item type refactoring!
I committed them! Can I add your name on the contributor list in the adapter?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sure :)

Copy link
Copy Markdown
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! :)

This is probably a follow-up and should be done separately, but it would be nice to also move it to be in line with how @AndyScherzinger did it for the Talk Android app: nextcloud/talk-android#5943

@penguin86
Copy link
Copy Markdown
Author

Hey @jancborchardt , yes I think it's the final intent: in fact, this isn't my first implementation unfortunately, at first I completely missed the big picture 🤦‍♂️ : see comment #16767 (comment) in the, now closed, original PR.

Signed-off-by: Daniele Verducci <penguin86@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants