Add an option to remove "errors" tab by uOOOO · Pull Request #100 · ChuckerTeam/chucker

uOOOO · 2019-08-20T02:59:01Z

This is for #88.

cortinico

Thanks for the PR @uOOOO
I'd love to get feedbacks from @redwarp and @olivierperez on this as well.
How about hiding the Error Tab just if there are no Throwables to show?
I generally like the Feature approach but is probably an overkill for this scenario?

redwarp · 2019-08-21T09:37:36Z

+class FeatureManager constructor(context: Context) {
+    enum class Feature {
+        HTTP_ONLY,
+        HTTP_AND_ERROR


Instead of HTTP_ONLY and HTTP_AND_ERROR, I would have a more explicit HIDE_ERROR_TAB which would be a boolean in the shared preferences, defaulting to none

redwarp · 2019-08-21T09:38:40Z

        setSupportActionBar(toolbar);
        toolbar.setSubtitle(getApplicationName());

+        FeatureManager featureManager = new FeatureManager(getApplicationContext());


Instead of modifying the layout to display stuff in the frame layout, I would modify the HomePageAdapter itself, so that it only displays the one tab is the HIDE_ERROR_TAB is set to true.

Agreed 👍

redwarp · 2019-08-21T09:44:42Z

I would prefer that instead of having the two feature, we would only have one boolean flag, HIDE_ERROR_TAB: the new error tab is a design decision, whether agreed or not, so hiding it should be made a more explicit flag IMO.
And then I'm never a huge fan of switching layouts between, so let's modify the HomePageAdapter class instead?
Then it would be good. And to have this tab hidden, we could have something like Chucker.setFeature(applicationContext, FeatureManager.Feature.HIDE_ERROR_TAB, true) or something?

That being said, we could do a better job at educating about the error tab, so we probably should work on some text or ui to display in the empty state, when no error has been recorded.

Something like "This tab displays error recorded with the ChcuckerCollector.onError method. Call this method when an error is thrown to check your stacktrace live" <- or something better :-P

redwarp · 2019-08-21T09:46:36Z

Or Chucker.features.setHideErrorTab(true)

olivierperez

@cortinico @redwarp I think we have to do some analyse/conception before doing in this way.

olivierperez · 2019-08-21T10:51:20Z

+import android.content.Context
+import android.content.SharedPreferences
+
+class FeatureManager constructor(context: Context) {


What is a FeatureManager, what it does? Please add some KDoc.

The constructor keyword redundant.

This is my first approach; boolean flag.

Chucker.features.setHideErrorTab(true)

But I found RetentionManager and I just decided make FeatureManager like it used not in memory value but SharedPreference.

olivierperez · 2019-08-21T10:54:27Z

+
+    enum class Feature {
+        HTTP_ONLY,
+        HTTP_AND_ERROR


What is a Feature in your mind?

For me, HTTP is a feature of Chucker, ERROR is another one. Maybe we should rather handle it like that. cc @cortinico @redwarp ?

It represents Network and Errors tabs.

olivierperez · 2019-08-21T10:55:33Z

        setSupportActionBar(toolbar);
        toolbar.setSubtitle(getApplicationName());

+        FeatureManager featureManager = new FeatureManager(getApplicationContext());


Agreed 👍

olivierperez · 2019-08-21T10:57:07Z

     */
    private void consumeIntent(Intent intent) {
+        if (viewPager != null && viewPager.getVisibility() != View.VISIBLE) {
+            return;


I don't like to depend on the visibility of a View in order to do (or not to do) a functionnal choice.

olivierperez · 2019-08-21T10:58:07Z

+        android:id="@+id/singleContainer"
+        android:layout_width="match_parent"
+        android:layout_height="match_parent"
+        android:visibility="gone" />


The layout of the main activity should not be changed for this PR.

olivierperez · 2019-08-21T11:03:57Z

        )

        Chucker.registerDefaultCrashHandler(collector)
+        Chucker.setFeature(applicationContext, FeatureManager.Feature.HTTP_AND_ERROR)


Here we should specify a list of features we want to enable. It's fair to think in the future there will be more features.
And by default, all the features will be enabled.

Something like :

Chucker.init( context = applicationContext, features = listOf(HttpFeature, ErrorFeature /* etc. */) )

And it should be in the Application class.

ping. @cortinico @redwarp What do you think about it?

olivierperez · 2019-08-21T14:00:05Z

I would prefer that instead of having the two feature, we would only have one boolean flag, HIDE_ERROR_TAB: the new error tab is a design decision, whether agreed or not, so hiding it should be made a more explicit flag IMO.
And then I'm never a huge fan of switching layouts between, so let's modify the HomePageAdapter class instead?

Agreed, a layout should not contains elements of 2 differents view.

olivierperez · 2019-08-21T14:00:20Z

Then it would be good. And to have this tab hidden, we could have something like Chucker.setFeature(applicationContext, FeatureManager.Feature.HIDE_ERROR_TAB, true) or something?

During my review I made another proposal of API. I'm not already sure of the one I like most, and I'm ready to read what everyone think about it.

olivierperez · 2019-08-21T14:00:45Z

That being said, we could do a better job at educating about the error tab, so we probably should work on some text or ui to display in the empty state, when no error has been recorded.
Something like "This tab displays error recorded with the ChcuckerCollector.onError method. Call this method when an error is thrown to check your stacktrace live" <- or something better :-P

Agreed, I love this feature, I really do. Should we create an issue for this point?

olivierperez · 2019-08-21T14:03:18Z

BTW, thanks for your contribution. It's not because I wont use this feature that we will not add this code ;-)
I totally understand someones don't wan't to use the "error" part.

cortinico · 2019-08-24T10:24:57Z

During my review I made another proposal of API. I'm not already sure of the one
I like most, and I'm ready to read what everyone think about it.

Talking about the API, I think that whatever works on the Chucker class is fine. Either Chucker.setFeature(Content, Feature, Boolean) or Chucker.init(features = listOf(...)) sounds good to me. I'd rather go for the first as it's more explicit and it's more extendible (easier to support Features with Integer or other type of parameters that are not boolean).

Agreed, I love this feature, I really do. Should we create an issue for this point?

Sure. @redwarp please go for that.

redwarp · 2019-08-28T15:10:59Z

Sure. @redwarp please go for that.

On it!

redwarp · 2019-08-28T15:19:53Z

I agree with @cortinico there, it will be easier to setup it the methods are called from the Chucker class.
Chucker.init(features=listOf) is elegant to me, but it's also more kotlin friendly than java friendly.

ColtonIdle · 2019-10-06T16:08:08Z

@uOOOO any ETA on this? Looking to use chucker, but I don't want to show the errors tab.

uOOOO · 2019-10-17T07:20:57Z

@uOOOO any ETA on this? Looking to use chucker, but I don't want to show the errors tab.

I'm sorry for the late reply. I thought this issue will be handled by the reviewers then. So I didn't change codes.

ColtonIdle · 2019-10-17T07:30:29Z

@cortinico @olivierperez @redwarp let's make this happen. Can you clear up the confusion? Should @uOOOO make changes?

uOOOO · 2019-10-17T07:36:21Z

@cortinico @olivierperez @redwarp let's make this happen. Can you clear up the confusion? Should @uOOOO make changes?

@olivierperez already did it.(Thank you!) . He implemented it as DSL. I think it's good way.
Please refer to #141 PR.

olivierperez · 2019-10-17T07:36:32Z

Hey ;-) I was temped by writing a DSL for that, so I've made one -> #141
I don't say we will keep my solution or forget yours, but it was fun to work on it.

@uOOOO @ColtonIdle Can you look at my PR, and tell me if you're ok with it?

uOOOO · 2019-10-17T08:12:50Z

Hey ;-) I was temped by writing a DSL for that, so I've made one -> #141
I don't say we will keep my solution or forget yours, but it was fun to work on it.

@uOOOO @ColtonIdle Can you look at my PR, and tell me if you're ok with it?

Please don't get me wrong. I just didn't know what to do. My bad :)
It looks good to me. especially DSL configuration.

olivierperez · 2019-10-17T09:03:31Z

I'll close this PR, we will continue on the other.
And please, review #141, contributors are welcome to comment members PR 👍

Show error tab conditionally

4619f88

uOOOO force-pushed the develop branch from 4a0bfeb to 4619f88 Compare August 20, 2019 04:08

cortinico reviewed Aug 20, 2019

View reviewed changes

cortinico requested review from cortinico and redwarp August 20, 2019 14:47

cortinico added the enhancement New feature or improvement to the library label Aug 20, 2019

cortinico added this to the 3.1.0 milestone Aug 20, 2019

redwarp reviewed Aug 21, 2019

View reviewed changes

olivierperez requested changes Aug 21, 2019

View reviewed changes

olivierperez mentioned this pull request Oct 15, 2019

Feature management for #88 #141

Closed

1 task

ColtonIdle mentioned this pull request Oct 17, 2019

Feature Request: Cloud Message Logging #140

Open

olivierperez closed this Oct 17, 2019

Uh oh!

Conversation

uOOOO commented Aug 20, 2019

Uh oh!

cortinico 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!

redwarp commented Aug 21, 2019

Uh oh!

redwarp commented Aug 21, 2019

Uh oh!

olivierperez 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!

uOOOO Aug 22, 2019 • 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!

uOOOO Aug 22, 2019 • 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!

olivierperez commented Aug 21, 2019

Uh oh!

olivierperez commented Aug 21, 2019

Uh oh!

olivierperez commented Aug 21, 2019

Uh oh!

olivierperez commented Aug 21, 2019

Uh oh!

cortinico commented Aug 24, 2019

Uh oh!

redwarp commented Aug 28, 2019

Uh oh!

redwarp commented Aug 28, 2019

Uh oh!

ColtonIdle commented Oct 6, 2019

Uh oh!

uOOOO commented Oct 17, 2019

Uh oh!

ColtonIdle commented Oct 17, 2019

Uh oh!

uOOOO commented Oct 17, 2019

Uh oh!

olivierperez commented Oct 17, 2019

Uh oh!

uOOOO commented Oct 17, 2019

Uh oh!

olivierperez commented Oct 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

uOOOO Aug 22, 2019 •

edited

Loading

uOOOO Aug 22, 2019 •

edited

Loading