Skip to content
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 support for deleting crashes in database. #10

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

X-Ryl669
Copy link

@X-Ryl669 X-Ryl669 commented Aug 7, 2018

This allows the application to clear all previous crashes (useful when updating the application, not to get previous crashes cluttering the database too).

@X-Ryl669
Copy link
Author

X-Ryl669 commented Aug 7, 2018

The travis failure is likely a misconfiguration of travis build, not from the code (the code builds correctly).

@ajitsing
Copy link
Owner

ajitsing commented Aug 8, 2018

@X-Ryl669 Thanks for the PR. Initially when I designed this lib I thought the cleaning of old data will be taken care by uninstalling the app or deleting the data manually from settings.

However your point seems valid in case of upgrades.

The only way for users to see all the crashes is using CrashListActivity. So if they don't want the older build's crashes they should be provided a way in the activity to clear all the current crashes. And if users are not using CrashListActivity then that would mean that they are not worried about previous crashes at all.

So IMO the better way to provide this functionality would be to provide a button in CrashListActivity rather than exposing an API for it in the Sherlock class.

Thoughts?

@X-Ryl669
Copy link
Author

X-Ryl669 commented Aug 8, 2018

In my case, I have a bug that cause the application main service to crash when starting, at "hostile" place. Clicking the crash notification tries to start the application (which is still trying to start its service since it did not close properly), which crashes. So said differently, it crashes with no report. If the user restart the app, the service is not started, so it works. My idea was querying Sherlock's crash list upon onCreate and if the list is not empty, starts the CrashActivity instead of the main activity, then empty the list. So we can have the user empty the list manually via a button, but it's also cool to allow doing it programmatically because it might be confusing for non tech savvy users.

@ajitsing
Copy link
Owner

You can still query the list of crashes from Sherlock with the help of Sherlock.getAllCrashes(). And when the list is not empty then start the CrashListActivity. But the problem with this approach would be even if the app is working fine but has some old crashes, it will always open the CrashListActivity first. I believe you wouldn't want such behaviour.

My suggestion would be don't crash the app when it is used in hostile places instead handle it gracefully by showing the user a friendly message.

Though I still like the idea of adding clear all crashes button on CrashListActivity.

What do you think?

@X-Ryl669
Copy link
Author

Even if the app is working fine but has some old crashes, it will always open the CrashListActivity first. I believe you wouldn't want such behaviour.

This is exactly the behaviour I was look for. Typically, I don't expect the app not to crash. So when it has crashed, I want the system to show a message for it (only visible the next time the app is started). It's displayed once, since after it's shown, the app is deleting all the crashes (the new one and any previous one). The user can (and will probably) ignore the crash activity. But if she wants to report she can do it via usual sharing method.

I would be also fine to have a button in my app triggering the display of the CrashListActivity, and a button in the activity allowing to clear all previous crashes (ideally, it should be a checkbox that can be pre-checked to "Erase all crashes when leaving")

I'm sorry I wasn't clear about the hostile environment. What I call hostile is when the users run my app on their phones. There are so many different situation (due to the high number of "optimized" chinese-based Android OS derivative, sensors, acceptances for permissions, changes in configuration while the app is running but paused, etc...) that it's very hard to predict everything. So when it crashes:

  1. The user is aware of it ("Force close message"), but
  2. The app state is not (since it was not able to save correctly its last state), so it can't resume correctly.
  3. Having Sherlock built-in does not save it, since once the user click on the notification, the app is restarted, and Sherlock is supposed to show the crash activity by itself (as I understand it)
  4. Since the app tries to resume its previous state (in my case, no activity is started but a foreground service), it crashes again and you can't see Sherlock's main activity.
  5. However, when the user starts my app directly, it's not starting the service but the main activity that checks Sherlock's presence of any previous crash and if found, it'll forward to the CrashListActivity.

I need a way to avoid this cycle continueing, either by the user clicking a "Clear all crashes" button (although I think it's too complex for a non tech-savvy user), or in my code by calling a clearAllCrashes function.

Probably a checkbox with "Erase all previous crashes" label would be better since it requires no user action to work (if I can set the "checked" state while launching the activity).

@X-Ryl669
Copy link
Author

Please check this implementation, and tell me if it's ok for you ?

@ajitsing
Copy link
Owner

@X-Ryl669 I believe the main problem here is the cyclic nature of the crash. We should try and fix that. I don't want to change Sherlock just for it. Although I still find the delete all crash functionality useful. I have a different UX in mind for it. So for now I will create a separate issue for it.

Maybe as part of this PR we can just add an API in Sherlock to delete all crashes. If you agree with it and want to do the changes, you have to follow below guidelines to make this enhancement.

  • Add all the db logic in SherlockDatabaseHelper
  • Expose API in Sherlock.java
  • User CrashTable.TRUNCATE_QUERY query to clear all crashes
  • Add Tests for both Sherlock.java and SherlockDatabaseHelper.java
  • Add the same API in sherlock-no-op which does nothing.
  • Run all the tests in local.

@X-Ryl669
Copy link
Author

Add all the db logic in SherlockDatabaseHelper

Done

Expose API in Sherlock.java

Done

User CrashTable.TRUNCATE_QUERY query to clear all crashes

I haven't done like this because the name is confusing (truncate is not the same as delete from table).

Add Tests for both Sherlock.java and SherlockDatabaseHelper.java

Will look through this. Right now the tests are passing, but I've not added any test for it.

Add the same API in sherlock-no-op which does nothing.

Ok, will do.

Run all the tests in local.

Sure, since Travis is broken...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants