Switch views to ViewBinding by vbuberen · Pull Request #253 · ChuckerTeam/chucker

vbuberen · 2020-02-27T13:28:01Z

📄 Context

Chucker relied on findViewById stuff for quite a long time. Since ViewBinding is officially released, I believe we should switch to it to make library code more concise and robust.

📝 Changes

Enabled ViewBinding in build.gradle.
Switched all Activities, Fragments and Adapters to use ViewBinding.
Removed some boilerplate, like duplicate of onTransactionItemClickListener().
Fixed link to tutorial, which became invalid after recent resources renaming.

📎 Related PR

Would like to merge it after Feature/tls info #252 so I could add new fields here as well.
Need to merge Add missing ThrowableViewModel #257 since there were some resources renaming.

🛠️ How to test

Just run it.

…hrowable models

…ragments

vbuberen · 2020-02-28T13:30:24Z

Renamed all views because we can forget about possible ids clashing, like it happened with findViewById - now we are always sure that we use the right view thanks to ViewBinding.

vbuberen · 2020-03-01T08:09:52Z

After resources renaming in #257 I need to merge that PR first and update this one afterwards.

MiSikora

I left some comments. One another thing I see is that view bindings are lateinit properties and they are never set to null in onDestroyView for fragments and in onDestroy for Activities which leads to leaks. It could be fixed manually or it could leverage mechanism of delegation and lifecycles like in i.e. this article - https://medium.com/@Zhuinden/simple-one-liner-viewbinding-in-fragments-and-activities-with-kotlin-961430c6c07c.

MiSikora · 2020-03-01T08:33:08Z

+                        if (position == 0) {
+                            Chucker.dismissTransactionsNotification(this@MainActivity)
+                        } else {
+                            Chucker.dismissErrorsNotification(this@MainActivity)
+                        }


I think it would be better to operate on constants from HomePageAdapter.

MiSikora · 2020-03-01T08:34:21Z

-        clazz.text = throwable.clazz
-        message.text = throwable.message
-        stacktrace.text = throwable.content
+        errorBinding.apply {


Sometimes I see that with(binding) is used and sometimes binding.apply(). I would stick to one convention. Preferably with one.

I don't agree that we should always stick to one particular scope function.
with or apply decision is based mainly on actions applied and on how you read it.
In onCreate() we are doing something using errorBinding, that is why I used with there.
In populateUI() I used apply, because I explicitly mean that I want to apply some properties to items from this binding.

It is still not clear to me to be honest. Should I split binding into two blocks of with and apply if I first do something with it and the apply some properties to it?

No. I would stick to one function in case of using it in the same place. But in this particular case I use these extensions in different functions.
So here it is just a matter of preference, because there are no clear conventions on these functions usage. Also, we don't care about the return type in both cases, since we are just removing multiple repetitions of errorBinding to improve readability. Finally, we will end up with the same Java code generated with additional variable generated, so there is no difference in preferring one or another in terms of some performance benefits.

Ok, I got it now.

MiSikora · 2020-03-01T08:36:00Z

-                code.text = "!!!"
+            transactionId = transaction.id
+
+            itemBinding.apply {


MiSikora · 2020-03-01T08:36:56Z

-            val recyclerView: RecyclerView? = fragment.view?.findViewById(R.id.chuckerTransactionResponseRecyclerView)
-            progressBar?.visibility = View.VISIBLE
-            recyclerView?.visibility = View.INVISIBLE
+            fragment.payloadBinding.apply {


MiSikora · 2020-03-01T08:37:07Z

-            progressBar?.visibility = View.INVISIBLE
-            recyclerView?.visibility = View.VISIBLE
-            recyclerView?.adapter = TransactionBodyAdapter(result)
+            fragment.payloadBinding.apply {


MiSikora · 2020-03-01T08:59:20Z

        }

-        private fun setStatusColor(holder: ViewHolder, transaction: HttpTransactionTuple) {
+        private fun setStatusColor(itemBinding: ChuckerListItemTransactionBinding, transaction: HttpTransactionTuple) {


itemBinding argument is not needed as it is a property in the constructor.

MiSikora · 2020-03-01T09:10:56Z

+            protocol.text = transaction?.protocol
+            status.text = transaction?.status.toString()
+            response.text = transaction?.responseSummaryText
+            ssl.setText(if (transaction?.isSsl == true) R.string.chucker_yes else R.string.chucker_no)


Shouldn't this show an empty text or hide a field in case isSsl is null? Realistically users should never experience this since it comes from the request which is processed rather immediately. Just something I noticed.

But maybe this suggests that the DB model is not proper. Maybe it should be split into requests and responses tables and transaction table would refer to those by their IDs. This is definitely not for this PR but maybe it should be discussed in some other ticket? I can create one and describe the issue I see with the current DB model.

Yeah, I believe it should be hidden.

As to DB issues - sure, feel free to raise it. I wasn't involved in the current design, so would gladly join if needed.

I'm not sure if you forgot about it during CR fixes (the hiding part). Other than that LGTM.

Yeah, forgot it. Already fixed. Please approve it if other things are fine.

vbuberen · 2020-03-01T09:28:51Z

@MiSikora I don't agree with your point about memory leaks in fragments. Despite it is being said in Google's tutorial, there is no case in Chucker where we would get a memory leak, because we don't use anything like retain fragments, etc. While getting the fragment from the backstack we will assign a new binding anyway. So there will be no leaks.
If I am wrong, please provide a case and an evidence that there is a leak, because I don't see any at the moment.

vbuberen · 2020-03-01T09:29:54Z

And I am against the solution from the article you suggested, because it is just redundant complexity.

MiSikora · 2020-03-01T09:52:21Z

@MiSikora I don't agree with your point about memory leaks in fragments. Despite it is being said in Googles tutorial, there is no case in Chucker where we would get a memory leak, because we don't use anything like retain` fragments, etc. While getting the fragment from the backstack we will assign a new binding anyway. So there will be no leaks.

But there will be leaks in the meantime. Fragments will hold reference to memory which is not used and no longer will be used. I understand that it is something negligible and it will not cause crashes, especially in a QA tool with couple of screens. It might be fine to ignore them at this scale but those are still leaks, even if temporary.

If I am wrong, please provide a case and an evidence that there is a leak, because I don't see any at the moment.

There was even a short discussion on Twitter about this. That references an issue on Google Issue Tracker and to leak definition on Leak Canary pages. https://twitter.com/Piwai/status/1201686009349455872

And I am against the solution from the article you suggested, because it is just redundant complexity.

I don't see how it is redundant but I agree that it introduces complexity. It really depends on if and how you want to deal with leaks. If there is no need to handle them then this solution (or nulling out views) is probably unnecessary. Otherwise there can be one well-defined mechanism that handles that or manual assigning of null to views.

vbuberen · 2020-03-01T10:36:12Z

Thanks for pointing out to this discussion, but I am already aware of it.

This discussion is also the reason why I asked you to provide an evidence in particular Chucker case. We have LeakCanary here, so you could easily catch the issue if it existed. If you are able to do it - I would gladly fix it myself.

And this is also the reason for me to say redundant complexity. We don't have this problem at the moment, right? So why would we add more code?
I believe that Chucker isn't a showcase for all theoretically possible cases, but rather a tool which does its job and gets new code when the new feature request appears or the new bug reported. Otherwise, why solve the problem which doesn't even exist? (I might be wrong if you could catch a memory leak here).

vbuberen · 2020-03-01T10:57:11Z

Talking about LeakCanary - see that we have an old version here. 😄
Going to create a separate PR with update.

MiSikora · 2020-03-01T10:59:22Z

Oh no, don't get me wrong. I'm not advocating that there are memory issues in Chucker. I'm only pointing out that views might be mishandled in fragments, even if leaks are not noticeable or even present right now.

Issues we are talking about would only occur if Chucker would add fragment transactions and if it is fine to postpone handling them for such a case then it is ok to do so.

vbuberen · 2020-03-01T11:05:50Z

Ok, I got your point. I am not the only core contributor here, so my point isn't the single source of truth, but I am advocating to solve such problems one step at a time (when they really arise).

MiSikora

LGTM

cortinico · 2020-03-02T19:49:01Z

 <?xml version="1.0" encoding="utf-8"?>
 <ImageView xmlns:android="http://schemas.android.com/apk/res/android"
-    android:id="@+id/chuckerTransactionResponseBinaryData"
+    android:id="@+id/binaryData"


Thanks for driving this forward @vbuberen @MiSikora
Out of curiosity, why we renamed back all the IDs to don't have chucker in the name? Was this because we wanted to have a cleaner code (due to view binding)?

When we used findViewById we could get into trouble when user has the same id for some view as Chucker. In such cases Chucker would crash, since, for example, instead of toolbar from activity_transaction it would find a toolbar from some layout in user's project. Due to this issue I renamed all ids for views for these long chucker..... items.

Now, when we switched to ViewBinding we can be 100% sure that the generated binding class has references to proper views, so we can change their ids to more concise ones.

This was one of the reasons I decided to switch Chucker to ViewBinding.

Awesome, thanks for the clarification

* 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>

vbuberen added 10 commits February 26, 2020 20:27

Switch to ViewBinding in activities

27f9f64

Switch to ViewBinding in ErrosAdapter, add formattedDate field into T…

a5876ad

…hrowable models

Transaction adapter switch to ViewBinding

30c2466

Remove variable for formatted date from models

1b94fe0

Switch to ViewBinding in TransactionPayloadAdapter

3e58285

Switch to ViewBinding in TransactionPaayload and TransactionOverviewF…

877e1db

…ragments

Switch list fragments to ViewBinding

ed0b04c

Fix link for tutorial opening

0642723

Rename views

4d51dd3

Merge latest develop changes

41dcbf3

vbuberen requested a review from cortinico February 28, 2020 13:38

Merge latest develop changes

8ecd093

vbuberen added the enhancement New feature or improvement to the library label Mar 1, 2020

vbuberen added this to the 3.2.0 milestone Mar 1, 2020

MiSikora reviewed Mar 1, 2020

View reviewed changes

Address code review feedback

4626cef

Hide SSL field if isSSL is null

5c85edc

MiSikora approved these changes Mar 1, 2020

View reviewed changes

vbuberen merged commit 4d44302 into develop Mar 1, 2020

vbuberen deleted the update/viewbinding branch March 1, 2020 12:33

cortinico reviewed Mar 2, 2020

View reviewed changes

Uh oh!

Conversation

vbuberen commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📄 Context

📝 Changes

📎 Related PR

🛠️ How to test

Uh oh!

vbuberen commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbuberen commented Mar 1, 2020

Uh oh!

MiSikora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiSikora Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiSikora Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbuberen commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbuberen commented Mar 1, 2020

Uh oh!

MiSikora commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbuberen commented Mar 1, 2020

Uh oh!

vbuberen commented Mar 1, 2020

Uh oh!

MiSikora commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbuberen commented Mar 1, 2020

Uh oh!

MiSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbuberen commented Feb 27, 2020 •

edited

Loading

vbuberen commented Feb 28, 2020 •

edited

Loading

MiSikora left a comment •

edited

Loading

MiSikora Mar 1, 2020 •

edited

Loading

MiSikora Mar 1, 2020 •

edited

Loading

vbuberen commented Mar 1, 2020 •

edited

Loading

MiSikora commented Mar 1, 2020 •

edited

Loading

MiSikora commented Mar 1, 2020 •

edited

Loading