Add switching between encoded and decoded URL formats. #233

MiSikora · 2020-02-09T20:43:39Z

📷 Screenshots

Show us what you've changed, we love images.

📄 Context

Why did you change something? Is there an issue to link here? Or an extarnal link?

There is an issue - #155

📝 Changes

Which code did you change? How?

From the exsiting code this PR touches mostly ViewModels and Entities. I added a way to display encoded or decoded URL (or its parts) with a wrapper FormattedUrl class.

⚠️ Breaking

Is there something breaking in the API? Any class or method signature changed?

No.

📝 How to test

Is there a special case to test your changes?

Run the sample app. Use checkbox in the top right corner to switch between encoded and decoded URLs. The option should persist between application restarts.

🔮 Next steps

Do we have to plan something else after the merge?

No.

Closes #155

vbuberen · 2020-02-09T21:21:20Z

Thanks for your contribution.

I haven't checked the code yet, but would like to provide some feedback on UI.
For me it looks not good from UX point of view to have a checkbox in a dropdown.
I would switch to some icon with 2 states (off/on) just inside toolbar to reduce number of actions required for switching.
We will have space there especially after my future planned update where share/save operations will move into FAB.

As to code review - will do in nearest days, since have quite limited availability at the moment.

P.S. Big thumbs up for tests)) Soon we will have coverage reports here in Chucker with requirements of some specific coverage level, so you are helping a lot.

vbuberen · 2020-02-09T21:24:35Z

Side note: welcome to our #chucker channel in Kotlinlang Slack. We discuss some plans and ideas there as well as share some things to find out if it is worth implementation.
We could discuss UX topic of your changes there if needed, for example.

MiSikora · 2020-02-09T21:24:46Z

Ok, thanks for the feedback. I wanted to use a toggle icon button but couldn't figure out how to represent HTTP encoding with an image. Maybe I'll come up with something.

Also, I'm thinking now it makes sense to make this option configurable from code. Users will probably expect to have a default option on app reinstallation as well. Since this is a QA plugin the apps will be reinstalled all the time. Not sure what is the best place for that. Chucker with a static function? Some option on ChukcerInterceptor? I'll have to update README.md.

vbuberen · 2020-02-09T21:25:55Z

Yes, it makes sense to provide users with option to configure default preference while setting up Chucker, like they can with retention period.

vbuberen · 2020-02-09T21:27:04Z

Ok, thanks for the feedback. I wanted to use a toggle icon button but couldn't figure out how to represent HTTP encoding with an image. Maybe I'll come up with something.

I will look for some icons as well and share ideas to not be just critic)))

cortinico · 2020-02-10T22:26:52Z

Also, I'm thinking now it makes sense to make this option configurable from code. Users will probably expect to have a default option on app reinstallation as well. Since this is a QA plugin the apps will be reinstalled all the time. Not sure what is the best place for that. Chucker with a static function? Some option on ChukcerInterceptor? I'll have to update README.md.

I'd rather avoid this approach. Offering this as a configurable feature has the side effect of extending the API surface for a limited benefit in terms of features.

I think we can just offer some UI controls (a button, checkbox, radio button, menu entry, etc...) to toggle the visibility of encoded/decoded URL.

vbuberen · 2020-02-11T10:30:00Z

What about saving state of selection (show encoded or decoded URLs)? Will we save the state or just make users switch after every reinstall?
Also, do you see it as a switch for every individual request or general in list of items?

MiSikora · 2020-02-11T11:17:11Z

State is already saved to preferences on switching it. The main issue I see users have to switch it every time whenever their app is reinstalled. Which I assume would be often given the nature of the library.

vbuberen · 2020-02-11T11:55:07Z

Sorry @MiSikora, I addressed this question to @cortinico. I also referred case about reinstalls, which happens quite a lot during development.

cortinico · 2020-02-12T00:37:24Z

What about saving state of selection (show encoded or decoded URLs)? Will we save the state or just make users switch after every reinstall?

👍 for this approach. We can definitely use SharedPreferences or a table in the Room Database to store key value pairs with the current state.

Also, do you see it as a switch for every individual request or general in list of items?

If I would have to develop this feature just for my self I'll do it stateless and only in the "detail" view (e.g. you click on the encoded URL and you magically decode it). I can't think of any meaningful use case that would push me to develop this feature for the list view as well (at least in my experience). I can anyway understand how this can be helpful for some so it's probably worth to investigate/consider.

Please also note that this has implications on how we search through the list (like if you decode URLs in, you probably want to offer search through the decoded URLs as well?)

vbuberen · 2020-02-12T10:32:16Z

@MiSikora Sorry for not checking your code. Would like to come to some agreement regarding this feature first to not make you and me do some back-and-forth check/fix steps to not spend even more time. I will check as soon as we get the final vision of this feature flow.

@cortinico Makes sense. I agree that I would prefer to have this so-called switch encode/decode in each individual transaction, without saving states between sessions at all, since also can't remember scenario where I needed to decode all urls at once.
But I don't want to push too hard here, since we might be breaking the rule You are not your user.

Probably, we should do some sort of survey somewhere and get some meaningful responses.

cortinico · 2020-02-13T18:58:57Z

without saving states between sessions at all, since also can't remember scenario where I needed to decode all urls at once.

Exactly. That's my strong point. I'd try to make this stateless to avoid complication and just provide a simple way to decode a URL, nothing more.

vbuberen · 2020-02-14T10:02:34Z

Ok, in such case let's skip the idea of doing survey etc. to not waste too much time.
@MiSikora could I ask you to update your code in a way that we will just have an options menu item in transaction details, which will switch encoded/decoded view without any state savings in SharedPreferences or db?

vbuberen · 2020-02-14T10:27:55Z

Also, I promised to provide icon options for the toggle.
My suggestion is to use eye icons:
https://materialdesignicons.com/icon/eye
https://materialdesignicons.com/icon/eye-off

The meaning behind this icon something like convert item into human readable text or not.

Having no state might be fine and we will definitely look for the feedback from users and change the way this feature works depending on it.

MiSikora · 2020-02-14T11:56:29Z

Hey, sorry for being silent. I'm a bit occupied lately. I will update the code to reflect discussion some time soon

vbuberen · 2020-02-14T19:26:37Z

@MiSikora Sure, take your time. We already appreciate you doing this PR and absolutely no pressure :)

MiSikora · 2020-02-15T10:59:32Z

I adapted the code. One thing that was not clear to me was HttpTransaction handling from the database. In the title LiveData it was assumed that the value might be null but in the transaction LiveData it was not. I changed signature to expliclitly say that the value might be null in the database and handled it in the code.

Below are the screenshots with the redesigned approach. By default URLs are not encoded. The eye icon is only visible when the overview tab is active.

vbuberen · 2020-02-22T17:51:56Z

Tried to run the sample app from your repo and wasn't able to see mentioned switch between encoding/decoding. Probably, I did something wrong?
Here is the sequence of actions:

Added redirectTo("https://ascii.cl?parameter=%22Click+on+%27URL+Encode%27%21%22").enqueue(cb) into HttpBinClient.
Opened this transaction in the sample app and saw that it is already decoded and clicks on eye icon do nothing.
Here is how it looks like.

vbuberen · 2020-02-22T17:54:54Z

Also, after trying it out myself it seems for me that we might improve the UX. Would be awesome if we could detect if url is encoded and show the eye icon only in such cases. For decoded or normal urls, like we have in our sample app show no eye icon, since I see no reason for willing to encode the url, which wasn't encoded originally.

vbuberen · 2020-02-22T17:56:05Z

And it would be awesome if you add this url for check encode/decode functionality into the sample app.

MiSikora · 2020-02-22T17:59:20Z

Wow, I'm sorry about that. It doesn't seems to work for me now as well. I must have messed up something with pushing and reverting commits. I'll double check my local commits and also I'll apply your suggestion about not showing eye icon for URLs which do not require decoding.

vbuberen · 2020-02-22T18:00:44Z

No worries here. You are doing a great amount of work for Chucker and it is fine that there might be some typos.

This reverts commit 69e7b485

MiSikora · 2020-02-22T18:30:41Z

Ok, I rebased to accommodate for the LiveDataRecord in tests and fixed URL not displaying in encoded form. I also added mentioned suggestion about showin the icon.

vbuberen · 2020-02-23T09:15:02Z

Yeah, can confirm that everything works.

Wouldn't you mind adding redirectTo("https://ascii.cl?parameter=%22Click+on+%27URL+Encode%27%21%22").enqueue(cb) into HttpBinClient in Chucker sample app so we could easily test this functionality on a regular basis?

Also found out that since the eye icon is available only in Overview after switching state user will see encoded/decoded url on all 3 tabs. It is not a big deal andI believe it is not worth it to add some additional logic for handling this, but just be aware.

MiSikora · 2020-02-23T09:53:15Z

Ok, I added the samples.

Not switching back on other tabs is intentional as there is share functionality which also uses encoding/decoding based on the users' preference. Should this feature not affect sharing?

vbuberen · 2020-02-23T09:58:58Z

library/src/main/java/com/chuckerteam/chucker/internal/data/entity/HttpTransaction.kt

-        path = ("/${url.pathSegments().joinToString("/")}${url.query()?.let { "?$it" } ?: ""}")
-        scheme = url.scheme()
+        val formattedUrl = FormattedUrl.fromHttpUrl(url, encoded = false)
+        this.url = formattedUrl.url


Maybe we could rename url parameter for this function, so there will be no need in using this.url for this line?

How about httpUrl?

Works for me 👍

vbuberen · 2020-02-23T10:12:00Z

library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionViewModel.kt

+    val transactionTitle: LiveData<String> = RepositoryProvider.transaction()
+        .getTransaction(transactionId)
+        .combineLatest(encodeUrl) { transaction, encodeUrl ->
+            if (transaction != null) "${transaction.method} ${transaction.getFormattedPath(encode = encodeUrl)}" else ""


This line seems too long.

Suggested change

if (transaction != null) "${transaction.method} ${transaction.getFormattedPath(encode = encodeUrl)}" else ""

if (transaction != null) {

"${transaction.method} ${transaction.getFormattedPath(encode = encodeUrl)}"

} else {

""

}

It looks long to me as well but it does not break Detekt rules. I can change it but I would rather rely on static analysis for such stuff. Otherwise what's the point of it?

Ok, as you wish. I will update rules for static analysis soon.

vbuberen · 2020-02-23T10:13:04Z

Ok, I finished code review. Everything seems fine and works like it should.

vbuberen · 2020-02-23T10:14:31Z

The only request for you - in the following PRs could you, please, wait for the whole review to pass, so the reviewer could leave all his comments? I think it is not that convenient that you do multiple commits with single line changes as well as for reviewer there is also need to refresh PR after every such commit.

This is not only more convenient, but also allows to not spin up CI too often and wait for checks to pass as well as burn some energy 😄

MiSikora · 2020-02-23T10:16:27Z

Sure. But wouldn't it be easier for both sides to leave all comments at once instead of one by one? There is such a feature in GH while doing CR, and this way I know when the code is reviewed as a whole. :)

vbuberen · 2020-02-23T10:18:17Z

I left comments one by one during one review. It took that much time, because I also checked some things on a device etc.
I would leave the final review anyway after my whole review.

MiSikora · 2020-02-23T10:19:12Z

Ok, makes sense. I'll try to be less eager with fixes between reviews in the future. :)

vbuberen · 2020-02-23T10:19:38Z

I would use one of the following options anyway, so you will see everything and it won't be one by one as you say.

vbuberen · 2020-02-23T10:20:36Z

Thanks for your time and for implementation of this feature request 👍 ❤️

vbuberen · 2020-02-23T10:27:24Z

And yeah, I won't use Add a single comment button in future as well. Thanks for pointing out.

* Remove scroll flags (#210) * Fix/gradle properties (#211) * Allow Gradle parallel build * Fix version name * Fix for curl command (#214) * Fix R8 minification crash on TransactionViewModel creation. (#219) * Big resources renaming (#216) * Fix clear action crash when application is dead (#222) * Fix for crash on Save transaction action (#221) * Show warning is transaction is null, fix crash in Save action * Uncomment sample transactions * Replace mulptiple returning with multiple throw due to detekt issue * Add message into IOException, update string for request being not ready * Fix for NPE in NotificationHelper (#223) * Add additional check fo transaction being not null before getting its notificationText * Extract transaction item from transactionBuffer * ViewModel refactoring (#220) * Update ViewModel dependency, refactor TransactionViewModel * Dependencies clean up * Switch to ViewModel on the main screen * Fix depleting bytes from the response. (#226) * Use HttpUrl instead of Uri for parsing URL data. * Do not read image sources directly from the response. * Simplify gzip logic. * Move gzip checks from IoUtils to OkHttpUtils. * Remove unused 'Response.hasBody()' extension. * Update library/src/main/java/com/chuckerteam/chucker/internal/support/OkHttpUtils.kt * Revert resource renaming (#227) * Revert renaming * Add changelgos for 3.1.2 (#230) * Add missing section to release 3.1.1 and 3.1.2 (#232) * Update Github templates (#235) * Update templates * Remove redundant dot * Remove default `no` from the checkbox * Switch to platform defined HTTP code constants (#238) * Add instruction for checkbox selection (#237) * Fix HttpHeader serialization when obfuscation is enabled (#240) * Update README (#243) * Add Action to validate the Gradle Wrapper (#245) * Gradle to 6.2 (#247) * Do not refresh transaction when it is not being affected. (#246) * Do not refresh transaction when it is not being affected. * Use correct null-aware comparison for HttpTransaction. * Add switching between encoded and decoded URL formats. (#233) * Add switching between encoded and decoded URL formats. * Make URL encoding transaction specific. * Change test name for upcoming #244 PR. * Use LiveDataRecord for combineLatest tests. * Properly switch encoding and decoding URL. * Show encoding icon only when it is viable. * Add encoded URL samples to HttpBinClient. * Avoid using 'this' scoping mechanism for URL. * Fix typo in feature request comment (#251) * RTL Support (#208) * Remove ltr forcing and replace ScrollView in Overview * Replace Overview layout, add rtl support for it * Add textDirection and textAlignment property for API 21+ * Fix host textview constraints * Replace android:src with app:srcCompat * Update ids for layouts to avoid clashes * Update Material components to stable * Remove supportsRTL tag from Manifest, update Gradle plugin * Styles update * Remove supportsRTL from library manifest * Revert usage of supportVectorDrawables to avoid crashes on APIs 16-19 due to notifications icons * Fix lint issue with vector drawable * Response search fixes (#256) * Fix response search to be case insensitive * Add minimum required symbols * Fix invalid options menu in response fragment * Feature/tls info (#252) * Add UI for TLS info * Implement logic for retrieving TLS info * Address code review feedback * Switch views to ViewBinding (#253) * Switch to ViewBinding in activities * Switch to ViewBinding in ErrorsAdapter, add formattedDate field into Throwable models * Transaction adapter switch to ViewBinding * Remove variable for formatted date from models * Switch to ViewBinding in TransactionPayloadAdapter * Switch to ViewBinding in TransactionPaayload and TransactionOverviewFragments * Switch list fragments to ViewBinding * Fix link for tutorial opening * Rename views * Address code review feedback * Hide SSL field if isSSL is null * Libs update (#260) * Update tools versions * JUnit update * Feature/truth (#258) * Add Truth, update part of test classes * Convert other tests to use Truth, fix date parser test * Add Truth assertions to FormatUtilsTest, fix ktlint issue * Update assertions to a proper syntax * Add missing ThrowableViewModel (#257) * Add Error ViewModel, update title in TransactionActivity in onCreate * Switch from errors to throwable naming to have a uniform naming style * Rename toolbar title * Migrating from Travis to GH Actions (#262) * Setup GH Actions * Run only on Linux * Remove Travis File * Run only gradlew directly * Update targetSDK and Kotlin (#264) * Add stale builds canceller (#265) * Add filters * Update Gradle wrapper validation workflow * Update pre-merge workflow * Fixed various Lints (#268) * fixed typos * fixed KDocs * Replace Travis badge with GH Actions badge (#269) * Remove redundant JvmName (#274) * Fix margins and paddings for payload items (#277) * Add selective interception. (#279) * Add selective interception. * Update README.md. * Align formatting in README with other points. * Avoid header name duplication. * Strip interception header also in the no-op library. * UX improvements (#275) * Add icon for non-https transactions * Update secondary color to be more contrast * Simplify protocol resources setting * Add tests to format utils (#281) * add tests to format utils * fixes after code review * formatting fix Co-authored-by: adammasyk <adam.masyk@programisci.eu> * format utils test refactor (#285) * format utils test refactor * share text test refactor * Migrate to Kotlin Coroutines (#270) * Add coroutine as a dependency in build.gradle * Migrate AsyncTasks to kotlin coroutines * Migrate executors with the coroutines in repositories * Multi cast upstream response for Chucker consumption. (#267) * Multi cast response upstream for Chucker consumption. * Read buffer prefix before potentially gunzipping it. * Inform Chucker about unprocessed responses. * Simplify multi casting logic. * Move read offset to a variable. * Inline one-line method. * Give better control over TeeSource read results. * Add documentation to TeeSource. * Close side channel when capacity is exceeded. Co-authored-by: Volodymyr Buberenko <vbuberen@users.noreply.github.com> * Remove unnecessary mock method. (#289) * removed redundant Gson configurations (#290) * increased test coverage for format utils (#291) Co-authored-by: Karthik R <karthr@paypal.com> * added few test cases for json formatting (#295) * Properly handle unexhausted network responses (#288) * Handle properly not consumed upstream body. * Handle IO issues while reading from file. * Update dependencies (#296) * Update depencies * Update OkHttp to 3.12.10 * Handle empty and missing response bodies. (#250) * Add failing test cases. * Remove unused const. * Gzip response body if it was gunzipped. * Add test cases for regular bodies in Chucker. * Fix rule formatting. * Use proper name for application interceptor. * Return original response downstream. * Account for no content with gzip encoding. * Use Truth for Chucker tests. * Honor empty plaintext bodies. * Revert changes to HttpBinClient. * Update library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt * Update library/src/main/java/com/chuckerteam/chucker/internal/support/OkHttpUtils.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com> Co-authored-by: Volodymyr Buberenko <vbuberen@users.noreply.github.com> * Add hasFixed size to RecyclerViews (#297) * Detekt to 1.7.4 (#299) * Revert "Add selective interception. (#279)" (#300) This reverts commit d14ed64. * Prepare 3.2.0 (#298) * Update versions info * Update Changelog * Fix links and update date Co-authored-by: Michał Sikora <michalsikora90@gmail.com> Co-authored-by: Nicola Corti <corti.nico@gmail.com> Co-authored-by: Sergey Chelombitko <119192+technoir42@users.noreply.github.com> Co-authored-by: Michał Sikora <me@mehow.io> Co-authored-by: Hitanshu Dhawan <hitanshudhawan1996@gmail.com> Co-authored-by: adammasyk <masiol91@gmail.com> Co-authored-by: adammasyk <adam.masyk@programisci.eu> Co-authored-by: Nikhil Chaudhari <nikhyl777@gmail.com> Co-authored-by: karthik rk <karthik.rk18@gmail.com> Co-authored-by: Karthik R <karthr@paypal.com>

MiSikora mentioned this pull request Feb 20, 2020

Do not refresh transaction when it is not being affected. #246

Merged

vbuberen added this to the 3.2.0 milestone Feb 22, 2020

MiSikora added 6 commits February 22, 2020 19:08

Add switching between encoded and decoded URL formats.

c4f4543

Save URL preference persistently.

635ba83

Revert "Save URL preference persistently."

718ab74

This reverts commit 69e7b485

Make URL encoding transaction specific.

0d92664

Change test name for upcoming #244 PR.

d1c263b

Use LiveDataRecord for combineLatest tests.

2a75a10

Properly switch encoding and decoding URL.

2ca40e4

Show encoding icon only when it is viable.

a0199f8

Add encoded URL samples to HttpBinClient.

07aec08

vbuberen reviewed Feb 23, 2020

View reviewed changes

Avoid using 'this' scoping mechanism for URL.

ac20c43

vbuberen reviewed Feb 23, 2020

View reviewed changes

vbuberen approved these changes Feb 23, 2020

View reviewed changes

vbuberen merged commit bee8813 into ChuckerTeam:develop Feb 23, 2020

MiSikora deleted the url-formatting branch February 23, 2020 10:29

Add switching between encoded and decoded URL formats. #233

Add switching between encoded and decoded URL formats. #233

Conversation

MiSikora commented Feb 9, 2020 • edited Loading

📷 Screenshots

📄 Context

📝 Changes

⚠️ Breaking

📝 How to test

🔮 Next steps

vbuberen commented Feb 9, 2020

vbuberen commented Feb 9, 2020

MiSikora commented Feb 9, 2020 • edited Loading

vbuberen commented Feb 9, 2020

vbuberen commented Feb 9, 2020 • edited Loading

cortinico commented Feb 10, 2020

vbuberen commented Feb 11, 2020

MiSikora commented Feb 11, 2020 • edited Loading

vbuberen commented Feb 11, 2020 • edited Loading

cortinico commented Feb 12, 2020

vbuberen commented Feb 12, 2020

cortinico commented Feb 13, 2020

vbuberen commented Feb 14, 2020

vbuberen commented Feb 14, 2020 • edited Loading

MiSikora commented Feb 14, 2020

vbuberen commented Feb 14, 2020

MiSikora commented Feb 15, 2020 • edited Loading

vbuberen commented Feb 22, 2020

vbuberen commented Feb 22, 2020

vbuberen commented Feb 22, 2020

MiSikora commented Feb 22, 2020

vbuberen commented Feb 22, 2020

MiSikora commented Feb 22, 2020 • edited Loading

vbuberen commented Feb 23, 2020

MiSikora commented Feb 23, 2020

vbuberen Feb 23, 2020

Choose a reason for hiding this comment

MiSikora Feb 23, 2020

Choose a reason for hiding this comment

vbuberen Feb 23, 2020

Choose a reason for hiding this comment

vbuberen Feb 23, 2020

Choose a reason for hiding this comment

MiSikora Feb 23, 2020

Choose a reason for hiding this comment

vbuberen Feb 23, 2020

Choose a reason for hiding this comment

vbuberen commented Feb 23, 2020

vbuberen commented Feb 23, 2020 • edited Loading

MiSikora commented Feb 23, 2020 • edited Loading

vbuberen commented Feb 23, 2020

MiSikora commented Feb 23, 2020 • edited Loading

vbuberen commented Feb 23, 2020

vbuberen commented Feb 23, 2020

vbuberen commented Feb 23, 2020

MiSikora commented Feb 9, 2020 •

edited

Loading

MiSikora commented Feb 9, 2020 •

edited

Loading

vbuberen commented Feb 9, 2020 •

edited

Loading

MiSikora commented Feb 11, 2020 •

edited

Loading

vbuberen commented Feb 11, 2020 •

edited

Loading

vbuberen commented Feb 14, 2020 •

edited

Loading

MiSikora commented Feb 15, 2020 •

edited

Loading

MiSikora commented Feb 22, 2020 •

edited

Loading

vbuberen commented Feb 23, 2020 •

edited

Loading

MiSikora commented Feb 23, 2020 •

edited

Loading

MiSikora commented Feb 23, 2020 •

edited

Loading