TheLastCrusade / SoundStream

1 stars 6 forks source link

Add ACRA crash reporting to SoundStream #310

Closed taywrobel closed 11 years ago

taywrobel commented 11 years ago

Adds the ACRA crash reporting to SoundStream. Configures ACRA to pop up a dialog in the event of a crash, asking if it's okay to gather crash data, and get additional comments, before uploading to the CloudAnt crash collection server.

The username and password listed are a reporting user for the database, so it's fine to have it in public. If worse comes to worse, we can delete that user and generate a new one.

Also updates the dependencies file to reflect the needed ACRA jar.

Screenshots below:

Crash dialog: screenshot_2013-08-28-01-16-07

Toast if they report it: screenshot_2013-08-28-01-16-14

ejohnson44 commented 11 years ago

Looks good to me :steam_locomotive:

dgreenhalgh commented 11 years ago

I like this, but before we pull this in:

We didn't discuss this as being a part of our 0.8.1 release. Do we want to pull it into that release or wait until 0.8.2?

reidbaker commented 11 years ago

If Jesse can give it a once over its OK with me that this goes in. On Aug 29, 2013 10:14 AM, "David Greenhalgh" notifications@github.com wrote:

I like this, but before we pull this in:

We didn't discuss this as being a part of our 0.8.1 release. Do we want to pull it into that release or wait until 0.8.2?

— Reply to this email directly or view it on GitHubhttps://github.com/TheLastCrusade/SoundStream/pull/310#issuecomment-23497092 .

theJenix commented 11 years ago

It looks good, but I'm concerned about that password and what it opens us up to. Is there any way we can pull that out into a build parameter or something we can store in the grail diary?

reidbaker commented 11 years ago

Good catch On Aug 29, 2013 10:44 AM, "Jesse Rosalia" notifications@github.com wrote:

It looks good, but I'm concerned about that password and what it opens us up to. Is there any way we can pull that out into a build parameter or something we can store in the grail diary?

— Reply to this email directly or view it on GitHubhttps://github.com/TheLastCrusade/SoundStream/pull/310#issuecomment-23499422 .

taywrobel commented 11 years ago

View commit message/pull request. That "password" is just the api key for CloudAnt. It's a reporting user that only has write permissions to the reporting database.

If we notice any abuses of it, it's trivial to disable and generate a new one. I figured since we're at low bandwidth this is fine for now, but if we get more attention/random people forking us it'd be good to make a new user not checked into source control and only add it in before pushing an apk to the play store. On Aug 29, 2013 11:53 AM, "Reid Baker" notifications@github.com wrote:

Good catch On Aug 29, 2013 10:44 AM, "Jesse Rosalia" notifications@github.com wrote:

It looks good, but I'm concerned about that password and what it opens us up to. Is there any way we can pull that out into a build parameter or something we can store in the grail diary?

— Reply to this email directly or view it on GitHub< https://github.com/TheLastCrusade/SoundStream/pull/310#issuecomment-23499422>

.

— Reply to this email directly or view it on GitHubhttps://github.com/TheLastCrusade/SoundStream/pull/310#issuecomment-23500255 .

theJenix commented 11 years ago

That's fair, but I would prefer we err on the side of caution because I don't think we have time to really effectively look for abuse. Is it easy to regenerate the key and keep it in a separate file?

taywrobel commented 11 years ago

Yeah, as long as there's no java quirks with it being in an annotation that should be fine. What's our current approach if any of handling prefs/config? On Aug 29, 2013 12:54 PM, "Jesse Rosalia" notifications@github.com wrote:

That's fair, but I would prefer we err on the side of caution because I don't think we have time to really effectively look for abuse. Is it easy to regenerate the key and keep it in a separate file?

— Reply to this email directly or view it on GitHubhttps://github.com/TheLastCrusade/SoundStream/pull/310#issuecomment-23504923 .

reidbaker commented 11 years ago

I think our current process is to put it into grail diary. On Aug 29, 2013 11:57 AM, "Taylor Wrobel" notifications@github.com wrote:

Yeah, as long as there's no java quirks with it being in an annotation that should be fine. What's our current approach if any of handling prefs/config? On Aug 29, 2013 12:54 PM, "Jesse Rosalia" notifications@github.com wrote:

That's fair, but I would prefer we err on the side of caution because I don't think we have time to really effectively look for abuse. Is it easy to regenerate the key and keep it in a separate file?

— Reply to this email directly or view it on GitHub< https://github.com/TheLastCrusade/SoundStream/pull/310#issuecomment-23504923>

.

— Reply to this email directly or view it on GitHubhttps://github.com/TheLastCrusade/SoundStream/pull/310#issuecomment-23505162 .

taywrobel commented 11 years ago

Alright, changes are pushed. New organization is this -

app.conf is in the grail-diary project (I pushed that already). It has the config information, and should be dropped in SoundStream/assets before running (it fails gracefully if it's absent, but won't be able to report without it).

CustomApp now initializes the ACRA reporting user from the conf file, not the annotation. The rest of config is through the annotation, since it's cleaner, and I didn't want to add prefs for every single thing.

There's a new class, SoundStreamPrefs, which is a wrapper class around handling the app.conf file. It takes care of loading it, parsing it, and trimming the values. Android doesn't have a build in config file parser, and I didn't want to add a dependency on Apache commons config, so it parses by hand, and therefore only supports simple syntax. We can make it more advanced in the future if the need arises, but for now it's stable and functional.

dgreenhalgh commented 11 years ago

May want to resolve the merge conflicts when you have a chance so that we can get this pulled in once @theJenix approves it

taywrobel commented 11 years ago

Just a small conflict in the strings.xml file. Fixed up and good to go.

dgreenhalgh commented 11 years ago

Something is sorely amiss. Check out your changes. I think GitHub is bugged.

You may want to withdraw this pull request, push up any merges, and reopen the pull request. :hamburger:

taywrobel commented 11 years ago

I think it's good, despite looking FUBAR. This branch was just made before the branch with the test package naming refactor. So, when I had to merge upstream/master and my branch, it brought in those changes onto this branch as well.

This is just git being the stupid system that it is, not github.

I'll admit it's ugly and a pain in the ass to look at, but that's git for ya! (end passive aggressiveness)

To see only the relevant code, you can view the commit directly here - https://github.com/twrobel3/SoundStream/commit/63b6d62379936c0bcb9ac0920b95617a49af4f4b

theJenix commented 11 years ago

What's really bizarre is that commit still shows your old changes, as does the latest commit on your branch...but the pull request appears to be right. :question: Anyway, I think we should pull this in, and see what chaos ensues, and address it then.

taywrobel commented 11 years ago

Agreed. No way to fix things without breaking them first.

Someday git and I will play nice together. Today is not that day.

theJenix commented 11 years ago

On that day, we will have :ice_cream:

taywrobel commented 11 years ago

SIT REP - All clear.

I just synced with my repo with the upstream, and everything builds, smoke test (after being updated to account for some changes) run green, and crash reporting still works.

Stupid git.