-
-
Notifications
You must be signed in to change notification settings - Fork 329
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
Add an option to remove "errors" tab #100
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
class FeatureManager constructor(context: Context) { | ||
enum class Feature { | ||
HTTP_ONLY, | ||
HTTP_AND_ERROR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -46,10 +51,30 @@ protected void onCreate(@Nullable Bundle savedInstanceState) { | |||
setSupportActionBar(toolbar); | |||
toolbar.setSubtitle(getApplicationName()); | |||
|
|||
FeatureManager featureManager = new FeatureManager(getApplicationContext()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed 👍
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. 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 |
Or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cortinico @redwarp I think we have to do some analyse/conception before doing in this way.
import android.content.Context | ||
import android.content.SharedPreferences | ||
|
||
class FeatureManager constructor(context: Context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a FeatureManager, what it does? Please add some KDoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor
keyword redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
enum class Feature { | ||
HTTP_ONLY, | ||
HTTP_AND_ERROR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It represents Network and Errors tabs.
@@ -46,10 +51,30 @@ protected void onCreate(@Nullable Bundle savedInstanceState) { | |||
setSupportActionBar(toolbar); | |||
toolbar.setSubtitle(getApplicationName()); | |||
|
|||
FeatureManager featureManager = new FeatureManager(getApplicationContext()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed 👍
@@ -76,6 +100,9 @@ protected void onNewIntent(Intent intent) { | |||
* Scroll to the right tab. | |||
*/ | |||
private void consumeIntent(Intent intent) { | |||
if (viewPager != null && viewPager.getVisibility() != View.VISIBLE) { | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like to depend on the visibility of a View in order to do (or not to do) a functionnal choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agreed.
android:id="@+id/singleContainer" | ||
android:layout_width="match_parent" | ||
android:layout_height="match_parent" | ||
android:visibility="gone" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The layout of the main activity should not be changed for this PR.
@@ -39,6 +40,7 @@ class MainActivity : AppCompatActivity() { | |||
) | |||
|
|||
Chucker.registerDefaultCrashHandler(collector) | |||
Chucker.setFeature(applicationContext, FeatureManager.Feature.HTTP_AND_ERROR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Agreed, a layout should not contains elements of 2 differents view. |
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. |
Agreed, I love this feature, I really do. Should we create an issue for this point? |
BTW, thanks for your contribution. It's not because I wont use this feature that we will not add this code ;-) |
Talking about the API, I think that whatever works on the
Sure. @redwarp please go for that. |
On it! |
I agree with @cortinico there, it will be easier to setup it the methods are called from the Chucker class. |
@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. |
@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. |
Hey ;-) I was temped by writing a DSL for that, so I've made one -> #141 @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 :) |
I'll close this PR, we will continue on the other. |
This is for #88.