Skip to content
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

The redesign of comment details content #20798

Merged
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ import android.os.Bundle
import androidx.core.view.isGone
import org.wordpress.android.R
import org.wordpress.android.datasets.NotificationsTable
import org.wordpress.android.fluxc.tools.FormattableRangeType
import org.wordpress.android.ui.comments.unified.CommentIdentifier
import org.wordpress.android.ui.comments.unified.CommentSource
import org.wordpress.android.ui.engagement.BottomSheetUiState
import org.wordpress.android.ui.reader.tracker.ReaderTracker.Companion.SOURCE_NOTIF_COMMENT_USER_PROFILE
import org.wordpress.android.util.AppLog
import org.wordpress.android.util.ToastUtils

Expand All @@ -26,6 +30,27 @@ class NotificationCommentDetailFragment : CommentDetailFragment() {
}
}

override fun getUserProfileUiState(): BottomSheetUiState.UserProfileUiState {
val user = mContentMapper.mapToFormattableContentList(mNote!!.body.toString())
.find { FormattableRangeType.fromString(it.type) == FormattableRangeType.USER }


return BottomSheetUiState.UserProfileUiState(
userAvatarUrl = mNote!!.iconURL,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it's safe to use the not-null assertion for mNote. Shouldn't we use the same approach as for the mComment?

Copy link
Contributor Author

@jarvislin jarvislin May 20, 2024

Choose a reason for hiding this comment

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

In NotificationCommentDetailFragment, mNote should be non-null after onCreate().

I replaced with note here.

  private val note: Note // note will be non-null after onCreate
        get() = mNote!!

blavatarUrl = "",
userName = user?.text ?: getString(R.string.anonymous),
userLogin = mComment?.authorEmail ?: "",
jarvislin marked this conversation as resolved.
Show resolved Hide resolved
userBio = "",
siteTitle = user?.meta?.titles?.home ?: getString(R.string.user_profile_untitled_site),
siteUrl = user?.ranges?.firstOrNull()?.url ?: "",
siteId = user?.meta?.ids?.site ?: 0L,
blogPreviewSource = SOURCE_NOTIF_COMMENT_USER_PROFILE
)
}

override fun getCommentIdentifier(): CommentIdentifier =
CommentIdentifier.NotificationCommentIdentifier(mNote!!.id, mNote!!.commentId);

override fun handleHeaderVisibility() {
mBinding?.headerView?.isGone = true
}
Expand All @@ -44,8 +69,8 @@ class NotificationCommentDetailFragment : CommentDetailFragment() {
// This should not exist, we should clean that screen so a note without a site/comment can be displayed
mSite = createDummyWordPressComSite(mNote!!.siteId.toLong())
}
if (mBinding != null && mReplyBinding != null && mActionBinding != null) {
showComment(mBinding!!, mReplyBinding!!, mActionBinding!!, mSite!!, mComment, mNote)
if (mBinding != null && mReplyBinding != null) {
showComment(mBinding!!, mReplyBinding!!, mSite!!, mComment, mNote)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,17 @@ package org.wordpress.android.ui.comments

import android.os.Bundle
import androidx.core.view.isVisible
import com.gravatar.AvatarQueryOptions
import com.gravatar.AvatarUrl
import com.gravatar.types.Email
import org.wordpress.android.R
import org.wordpress.android.fluxc.model.CommentModel
import org.wordpress.android.fluxc.model.SiteModel
import org.wordpress.android.ui.comments.unified.CommentIdentifier
import org.wordpress.android.ui.comments.unified.CommentSource
import org.wordpress.android.ui.engagement.BottomSheetUiState
import org.wordpress.android.ui.reader.tracker.ReaderTracker
import org.wordpress.android.util.WPAvatarUtils

/**
* Used when called from comment list
Expand All @@ -23,6 +31,28 @@ class SiteCommentDetailFragment : CommentDetailFragment() {
}
}

override fun getUserProfileUiState(): BottomSheetUiState.UserProfileUiState {
return BottomSheetUiState.UserProfileUiState(
userAvatarUrl = CommentExtension.getAvatarUrl(
mComment!!,
Copy link
Contributor

@antonis antonis May 22, 2024

Choose a reason for hiding this comment

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

Nit pick: I wonder if we should avoid the non-null assertion operator for mComment in this file since it is assigned a nullable value. Wdyt of using ?. when possible with a default value?

Copy link
Contributor Author

@jarvislin jarvislin May 24, 2024

Choose a reason for hiding this comment

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

Hmmm, I just reviewed the mComment, I have a strong sense that mComment should be non-null in SiteCommentDetailFragment, but the parent class CommentDetailFragment handles mComment with defensive programming and make it like nullable.

I plan to replace it with

 private val comment: CommentModel
        get() = mComment!!

resources.getDimensionPixelSize(R.dimen.avatar_sz_large)
),
blavatarUrl = "",
userName = mComment!!.authorName ?: getString(R.string.anonymous),
userLogin = mComment!!.authorEmail ?: "",
// keep them empty because there's no data for displaying on UI
userBio = "",
siteTitle = "",
siteUrl = "",
siteId = 0L,
blogPreviewSource = ReaderTracker.SOURCE_SITE_COMMENTS_USER_PROFILE
)
}

override fun getCommentIdentifier(): CommentIdentifier =
CommentIdentifier.SiteCommentIdentifier(mComment!!.id, mComment!!.remoteCommentId)


override fun handleHeaderVisibility() {
mBinding?.headerView?.isVisible = true
}
Expand All @@ -48,3 +78,19 @@ class SiteCommentDetailFragment : CommentDetailFragment() {
}
}
}

object CommentExtension {
fun getAvatarUrl(comment: CommentModel, size: Int): String = when {
comment.authorProfileImageUrl != null -> WPAvatarUtils.rewriteAvatarUrl(
comment.authorProfileImageUrl!!,
size
)

comment.authorEmail != null -> AvatarUrl(
Email(comment.authorEmail!!),
AvatarQueryOptions(size, null, null, null)
).url().toString()

else -> ""
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
@file:Suppress("DEPRECATION")

package org.wordpress.android.ui.comments.unified

import android.view.LayoutInflater
import android.view.View
import android.widget.PopupWindow
import org.wordpress.android.R
import org.wordpress.android.databinding.CommentActionsBinding
import org.wordpress.android.ui.comments.CommentDetailFragment
import org.wordpress.android.util.ToastUtils

object CommentActionPopupHandler {
@JvmStatic
fun show(anchorView: View, listener: CommentDetailFragment.OnActionClickListener?) {
val popupWindow = PopupWindow(anchorView.context, null, R.style.WordPress)
popupWindow.isOutsideTouchable = true
popupWindow.elevation = anchorView.context.resources.getDimension(R.dimen.popup_over_toolbar_elevation)
popupWindow.contentView = CommentActionsBinding
.inflate(LayoutInflater.from(anchorView.context))
.apply {
textUserInfo.setOnClickListener {
listener?.onUserInfoClicked()
popupWindow.dismiss()
}
textShare.setOnClickListener {
ToastUtils.showToast(it.context, "not yet implemented")
popupWindow.dismiss()
}
textEditComment.setOnClickListener {
listener?.onEditCommentClicked()
popupWindow.dismiss()
}
textChangeStatus.setOnClickListener {
ToastUtils.showToast(it.context, "not yet implemented")
popupWindow.dismiss()
}
}.root
popupWindow.showAsDropDown(anchorView)
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package org.wordpress.android.ui.engagement

import java.io.Serializable

sealed class BottomSheetUiState {
@Suppress("SerialVersionUIDInSerializableClass")
data class UserProfileUiState(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we shouldn't mark some fields as nullable because not all are always available for all the cases. For example, if the user does not have an avatar URL, it should be null but not an empty string that has less straight meaning. When using the state in different places with not enough context, it looks like the state has to contain all the data (except for onSiteClickListener.)
Just leaving this note for your consideration :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I 100% agree with you, straight meaning is important for us to handle those fields correctly. I find many implementations use default values, but it'd be better if they are defined as nullable.

I suspect that when handling this data with Java in the past, there was a tendency to use defensive programming to avoid crashes. However, now with Kotlin, handling nullables has become much easier. I think we can find time to refactor this.

val userAvatarUrl: String,
val blavatarUrl: String,
Expand All @@ -10,9 +13,8 @@ sealed class BottomSheetUiState {
val siteTitle: String,
val siteUrl: String,
val siteId: Long,
val onSiteClickListener: ((siteId: Long, siteUrl: String, source: String) -> Unit)? = null,
val blogPreviewSource: String
) : BottomSheetUiState() {
) : BottomSheetUiState(), Serializable {
val hasSiteUrl: Boolean = siteUrl.isNotBlank()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -111,14 +111,14 @@ class EngagedPeopleListFragment : Fragment() {
recycler.layoutManager = layoutManager

userProfileViewModel.onBottomSheetAction.observeEvent(viewLifecycleOwner) { state ->
var bottomSheet = childFragmentManager.findFragmentByTag(USER_PROFILE_BOTTOM_SHEET_TAG)
var bottomSheet = childFragmentManager.findFragmentByTag(UserProfileBottomSheetFragment.TAG)
as? UserProfileBottomSheetFragment

when (state) {
ShowBottomSheet -> {
if (bottomSheet == null) {
bottomSheet = UserProfileBottomSheetFragment.newInstance(USER_PROFILE_VM_KEY)
bottomSheet.show(childFragmentManager, USER_PROFILE_BOTTOM_SHEET_TAG)
bottomSheet.show(childFragmentManager, UserProfileBottomSheetFragment.TAG)
}
}

Expand Down Expand Up @@ -192,7 +192,7 @@ class EngagedPeopleListFragment : Fragment() {
}

is OpenUserProfileBottomSheet -> {
userProfileViewModel.onBottomSheetOpen(event.userProfile, event.onClick, event.source)
userProfileViewModel.onBottomSheetOpen(event.userProfile, event.source)
}
}

Expand Down Expand Up @@ -304,8 +304,6 @@ class EngagedPeopleListFragment : Fragment() {
private const val KEY_LIST_SCENARIO = "list_scenario"
private const val KEY_LIST_STATE = "list_state"

private const val USER_PROFILE_BOTTOM_SHEET_TAG = "USER_PROFILE_BOTTOM_SHEET_TAG"

@JvmStatic
fun newInstance(listScenario: ListScenario): EngagedPeopleListFragment {
val args = Bundle()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class ListScenarioUtils @Inject constructor(
imageManager,
notificationsUtilsWrapper
)
headerNoteBlock.setIsComment(note.isCommentType)
headerNoteBlock.setReplyToComment(note.isCommentReplyType)

val spannable: Spannable = notificationsUtilsWrapper.getSpannableContentForRanges(headerNoteBlock.getHeader(0))
val spans = spannable.getSpans(
Expand Down
Loading
Loading