Skip to content

Add color to request and response body when content-type is json#930

Merged
cortinico merged 24 commits into
ChuckerTeam:developfrom
Amirhy:feat_colorful_body
Mar 4, 2023
Merged

Add color to request and response body when content-type is json#930
cortinico merged 24 commits into
ChuckerTeam:developfrom
Amirhy:feat_colorful_body

Conversation

@Amirhy

@Amirhy Amirhy commented Nov 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

📷 Screenshots

Screen Shot 1401-09-07 at 14 21 45

Screen Shot 1401-09-07 at 14 22 09

📄 Context

I have been using chucker for a long time. But the gray texts in the body of the requests and responses bothered me and my teammates's eyes and also made it difficult for us to read the texts properly.
So I decided to color the text of body of requests and responses.
Although I've only done it for application/json content-type and I'll support the rest of them soon.
I also fixed a bug that if there is no HeaderItem type inside the adapter items and you search for a text in the body of requests and responses, the text does not highlight properly.

📝 Changes

  • Implement colorful text for body of request and response when content-type is application/json

🚫 Breaking

🛠️ How to test

I just write some tests for json formatting in SpanUtilTest class that you can run them and also you can do functional testing.

⏱️ Next steps

Implement this feature for other content-types , for example application/xml

@Amirhy Amirhy changed the title Feat colorful body Add color to request and response body when content-type is json Nov 28, 2022
@cortinico

cortinico commented Dec 2, 2022

Copy link
Copy Markdown
Member

Thanks for sending this @Amirhy. I won't be able to review this till sometime next week as I'm a bit busy at the moment 👍

@Amirhy

Amirhy commented Dec 11, 2022

Copy link
Copy Markdown
Contributor Author

Hi @cortinico. Hope you 're doing well.
Did you have free time to review my PR?

@cortinico

cortinico commented Dec 13, 2022

Copy link
Copy Markdown
Member

Did you have free time to review my PR?

Sorry I was quite busy this week. I'll have to look into this over the next weekend.
In the meanwhile @ArjanSM do you want to do a pass here?

@ArjanSM

ArjanSM commented Dec 14, 2022

Copy link
Copy Markdown
Contributor

Did you have free time to review my PR?

Sorry I was quite busy this week. I'll have to look into this over the next weekend. In the meanwhile @ArjanSM do you want to do a pass here?

@cortinico I've been a little occupied today. I'll take a look at it tomorrow.

ArjanSM
ArjanSM reviewed Dec 17, 2022
View reviewed changes

@ArjanSM ArjanSM left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR @Amirhy 🏅
I've made a few suggestions.
Also, I personally feel that it would be nice if you could create different PRs (one for text coloring feature and the other for the Text highlighting bug) or the reference to the feature issue and the bug in the PR description.

Comment thread library/src/main/kotlin/com/chuckerteam/chucker/internal/support/SpanTextUtil.kt Outdated
Comment thread library/src/main/kotlin/com/chuckerteam/chucker/internal/support/SpanTextUtil.kt Outdated
Comment thread library/src/main/kotlin/com/chuckerteam/chucker/internal/support/SpanTextUtil.kt Outdated
@Amirhy

Amirhy commented Dec 24, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for the PR @Amirhy 🏅 I've made a few suggestions. Also, I personally feel that it would be nice if you could create different PRs (one for text coloring feature and the other for the Text highlighting bug) or the reference to the feature issue and the bug in the PR description.

Thanks for your time @ArjanSM. I will fix them as soon as possible.

@Amirhy

Amirhy commented Dec 25, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for the PR @Amirhy 🏅 I've made a few suggestions. Also, I personally feel that it would be nice if you could create different PRs (one for text coloring feature and the other for the Text highlighting bug) or the reference to the feature issue and the bug in the PR description.

Ok I removed my changes that fix the mentioned bug, and I will create another PR for them.

@Amirhy

Amirhy commented Jan 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@cortinico @ArjanSM kindly reminder

ArjanSM
ArjanSM reviewed Jan 24, 2023
View reviewed changes

@ArjanSM ArjanSM left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sorry for the delay. Just a few to nits to begin with. I'll give it a more thorough pass later in the week.

Comment thread library/src/androidTest/kotlin/com/chuckerteam/chucker/SpanUtilTest.kt Outdated
Comment thread library/src/androidTest/kotlin/com/chuckerteam/chucker/SpanUtilTest.kt
Comment thread library/src/androidTest/kotlin/com/chuckerteam/chucker/SpanUtilTest.kt
Comment thread library/src/androidTest/kotlin/com/chuckerteam/chucker/SpanUtilTest.kt
Comment thread library/src/androidTest/kotlin/com/chuckerteam/chucker/SpanUtilTest.kt
Comment thread ...src/main/kotlin/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadAdapter.kt Outdated
Comment thread ...src/main/kotlin/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadAdapter.kt Outdated
Comment thread ...src/main/kotlin/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadAdapter.kt Outdated
Comment thread ...rc/main/kotlin/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadFragment.kt Outdated
Comment thread ...rc/main/kotlin/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadFragment.kt Outdated
@Amirhy Amirhy closed this Feb 7, 2023
@Amirhy Amirhy reopened this Feb 7, 2023
@Amirhy

Amirhy commented Feb 7, 2023

Copy link
Copy Markdown
Contributor Author

sorry for the delay. Just a few to nits to begin with. I'll give it a more thorough pass later in the week.

Hi @ArjanSM,

Hope you are doing well.

I resolved all threads you created.Please let me know if you have any other comment.

ArjanSM
ArjanSM reviewed Feb 13, 2023
View reviewed changes

@ArjanSM ArjanSM left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Getting there!
Just a bunch of few nits.

Comment thread library/src/main/kotlin/com/chuckerteam/chucker/internal/support/SpanTextUtil.kt Outdated
Comment thread library/src/main/kotlin/com/chuckerteam/chucker/internal/support/SpanTextUtil.kt Outdated
Comment thread library/src/main/kotlin/com/chuckerteam/chucker/internal/support/SpanTextUtil.kt Outdated
@Amirhy

Amirhy commented Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

Hi Dear @ArjanSM

Do you have any new nits about my changes? ;-)

I want you check this comment and let me know what is your opinion?

cortinico
cortinico reviewed Feb 25, 2023
View reviewed changes

@cortinico cortinico left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code looks good to me 👍 @ArjanSM already did a great review!
Only left a minor nits.

Comment thread library/src/main/kotlin/com/chuckerteam/chucker/internal/support/SpanTextUtil.kt Outdated
Comment thread library/src/main/kotlin/com/chuckerteam/chucker/internal/support/SpanTextUtil.kt Outdated
Comment thread ...rc/main/kotlin/com/chuckerteam/chucker/internal/ui/transaction/TransactionPayloadFragment.kt
Comment on lines +328 to +329
if (result.isEmpty())
result.add(subSequence(0, length))

@cortinico cortinico Feb 25, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why this?

@Amirhy Amirhy Feb 28, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's because when the char sequence doesn't have any lines so I should add all of its content to the result.

@Amirhy Amirhy requested a review from a team as a code owner February 28, 2023 08:17
@ArjanSM

ArjanSM commented Mar 3, 2023

Copy link
Copy Markdown
Contributor

Hi Dear @ArjanSM

Do you have any new nits about my changes? ;-)

I want you check this comment and let me know what is your opinion?

Aah! my bad! 👍

cortinico
cortinico approved these changes Mar 4, 2023
View reviewed changes

@cortinico cortinico left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is awesome @Amirhy Thanks for taking the time to add this feature!

@cortinico cortinico merged commit 3ae4d08 into ChuckerTeam:develop Mar 4, 2023
@misael-jonathan

misael-jonathan commented Apr 12, 2023

Copy link
Copy Markdown

hi @Amirhy , I think there's performance issue when we used this implementation in large json data. I just updated my chucker with this colored request/response, in some cases it took 2-3 secs to load my response, and if its super big it'll just stuck on loading state. Can u verify this issue on your side?

@Amirhy

Amirhy commented Apr 16, 2023

Copy link
Copy Markdown
Contributor Author

Hi @misael-jonathan ,
Thanks for sharing your experience.
If it is possible please send me your json to my email (amirhosein.hosseini76@gmail.com)
I will investigate on it and inform you as soon as possible.

@misael-jonathan

misael-jonathan commented Apr 17, 2023

Copy link
Copy Markdown

Hi, I just sent the json to your email. Actually it is the same json you can find in LargeJson.kt in the sample app.
So, I think you can start from there.

Thanks @Amirhy

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

Reviewers

@cortinico cortinico cortinico approved these changes

@ArjanSM ArjanSM ArjanSM approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Amirhy @cortinico @ArjanSM @misael-jonathan