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

Adding collapse/expand JSON payloads feature. #1107

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

jeancsanchez
Copy link

📷 Screenshots

chucker-take-2.mov
chucker-take-1.mov

📄 Context

First of all, thank you so much for this awesome library!!!
I'm mobile developer, and I've been using this library in my job every single day. In some situations, I have to share my screen with a backend developer to show that the payload response has a bug and it should to be fixed on backend side.
However, it often becomes complicated to do so because the payload is too large and the search bar doesn't help much some times.
This change adds one more ViewHolder type where it is possible to collapse/expand JSON payloads, helping us to find some portion of the JSON more easily, specially when it has a lot of nested objects/arrays.

⏱️ Next steps

1 - Remove unnecessary commas when the payload is collapsed.
2 - Improve searching text when the payload is collapsed.

@jeancsanchez jeancsanchez requested a review from a team as a code owner October 3, 2023 15:39
# Conflicts:
#	library/src/main/kotlin/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadFragment.kt
@jeancsanchez jeancsanchez changed the title Adding collapsable/expand JSON payloads feature. Adding collapse/expand JSON payloads feature. Oct 3, 2023
@cortinico
Copy link
Member

Thanks for sending this over @jeancsanchez
The feature looks really interesting. I will have to review it and test it thoroughly before we can merge it. Please allow me some time to do so 👍

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Hi @jeancsanchez thanks for your patience while I tested and reviewed this change. This feature is really promising and it would makes sense inside Chucker.

I do have a couple of points to share though.

  1. I'm unsure how this feature will play out with heavily nested objects, specifically because you won't split the lines anymore. An example is this simple response:
Before After
Screenshot_1697896509 Screenshot_1697896498

And this example had only one level of nesting. Can we had some examples to the Chucker sample app to stress test this feature?

  1. Similarly, the feature was not working well big JSON, like the one used for the /post sample request. See
video-1697896725.mp4

We need to see what's going on here before we can merge this

@@ -53,6 +59,12 @@ internal class TransactionBodyAdapter : RecyclerView.Adapter<TransactionPayloadV
TransactionPayloadViewHolder.BodyLineViewHolder(bodyItemBinding)
}

TYPE_BODY_COLLAPSABLE -> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TYPE_BODY_COLLAPSABLE -> {
TYPE_BODY_LINE_COLLAPSABLE -> {

Copy link
Member

Choose a reason for hiding this comment

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

Can we rename Collapsable to LineCollapsable everywhere?

Comment on lines +4 to +6
android:tint="#F5F5F5"
android:viewportWidth="24"
android:viewportHeight="24">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
android:tint="#F5F5F5"
android:viewportWidth="24"
android:viewportHeight="24">
android:tint="#FFFFFF"
android:viewportWidth="24.0"
android:viewportHeight="24.0">

Comment on lines +4 to +6
android:tint="#F5F5F5"
android:viewportWidth="960"
android:viewportHeight="960">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
android:tint="#F5F5F5"
android:viewportWidth="960"
android:viewportHeight="960">
android:tint="#FFFFFF"
android:viewportWidth="960"
android:viewportHeight="960">

Can you update the viewport side to be 24 and adapt the path instructions here?

mapToJsonElements()
} catch (t: JsonSyntaxException) {
Toast.makeText(context, t.message, Toast.LENGTH_LONG).show()
Logger.error(t.message ?: "Error when formatting json")
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather display the "Error when formatting json" in the Toast and dump the stacktrace in logcat instead

Comment on lines +540 to +548
forEach { item ->
when (item) {
is TransactionPayloadItem.BodyLineItem -> bodyBuilder.append(item.line)
is TransactionPayloadItem.HeaderItem,
is TransactionPayloadItem.ImageItem -> newList.add(item)

else -> Unit
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Not a super big fan of this approach where you re-add HeaderItem and ImageItem.
The only thing you need to do is to map BodyLineItem to BodyCollapsibleLineItem if the user clicked. Could you clean this up a bit?

Copy link
Member

Choose a reason for hiding this comment

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

Also can you make this when exhaustive?

@@ -226,10 +240,182 @@ internal sealed class TransactionPayloadViewHolder(view: View) : RecyclerView.Vi
const val LUMINANCE_THRESHOLD = 0.25
}
}

internal class BodyJsonViewHolder(
Copy link
Member

Choose a reason for hiding this comment

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

Can we be consistent with the naming here?
This should be a BodyLineCollapsibleViewHolder

android:id="@+id/imgExpand"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginTop="4dp"
Copy link
Member

Choose a reason for hiding this comment

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

@dimen/chucker_half_grid

android:layout_height="wrap_content"
android:gravity="start"
android:minLines="1"
android:layout_marginTop="4dp"
Copy link
Member

Choose a reason for hiding this comment

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

@dimen/chucker_half_grid

Comment on lines +406 to +407
private fun View.show() = apply { isVisible = true }
private fun View.gone() = apply { isVisible = false }
Copy link
Member

Choose a reason for hiding this comment

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

There is a ktx library inside androidx that was providing those functions already.

view.isGone = true
view.isVisible = true

Can we use it?

Comment on lines +274 to +279
for ((key, value) in entrySet()) {
JsonObject().also {
it.add(key, value)
attrList.add(TransactionPayloadItem.BodyCollapsableItem(jsonElement = it))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why are you doing this? Could you explain?

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