Skip to content

Commit

Permalink
backport oreo fixes to acra 4
Browse files Browse the repository at this point in the history
  • Loading branch information
F43nd1r committed Mar 21, 2018
1 parent 3af731a commit 05e9a53
Show file tree
Hide file tree
Showing 9 changed files with 37 additions and 20 deletions.
6 changes: 3 additions & 3 deletions acra/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,13 @@ apply plugin: 'com.jfrog.artifactory'

android {
compileSdkVersion Integer.parseInt(androidVersion)
buildToolsVersion '25.0.2'

lintOptions {
abortOnError false
}

defaultConfig {
minSdkVersion 8
minSdkVersion 14
targetSdkVersion androidVersion
versionName version
consumerProguardFile proguardFile
Expand All @@ -26,7 +25,8 @@ android {
}

dependencies {
compile "com.android.support:support-v4:$supportVersion"
compile "com.android.support:support-compat:$supportVersion"
compile "com.android.support:support-fragment:$supportVersion"
compile "com.android.support:support-annotations:$supportVersion"
annotationProcessor project(':annotationprocessor')
provided project(':annotations')
Expand Down
8 changes: 4 additions & 4 deletions acra/gradle.properties
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
version=4.10.0
group=com.faendir
version=4.11
group=ch.acra
archivesBaseName=acra
androidVersion=24
supportVersion=24.1.1
androidVersion=27
supportVersion=27.1.0
proguardFile=src/main/proguard/proguard.cfg
release.useAutomaticVersion=true
ossrhUser=
Expand Down
3 changes: 3 additions & 0 deletions acra/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="ch.acra.acra">

<uses-permission android:name="android.permission.WAKE_LOCK"/>

<application>
<activity
android:name="org.acra.dialog.CrashReportDialog"
Expand All @@ -29,6 +31,7 @@
<service
android:name="org.acra.sender.SenderService"
android:exported="false"
android:permission="android.permission.BIND_JOB_SERVICE"
android:process=":acra" />

<provider
Expand Down
6 changes: 5 additions & 1 deletion acra/src/main/java/org/acra/builder/ReportExecutor.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import android.content.Context;
import android.content.Intent;
import android.content.SharedPreferences;
import android.os.Build;
import android.os.Debug;
import android.os.Looper;
import android.support.annotation.NonNull;
Expand Down Expand Up @@ -125,10 +126,13 @@ public void execute(@NonNull final ReportBuilder reportBuilder) {
reportPrimer.primeReport(context, reportBuilder);

boolean sendOnlySilentReports = false;
final ReportingInteractionMode reportingInteractionMode;
ReportingInteractionMode reportingInteractionMode;
if (!reportBuilder.isSendSilently()) {
// No interaction mode defined in the ReportBuilder, we assume it has been set during ACRA.initACRA()
reportingInteractionMode = config.reportingInteractionMode();
if (reportingInteractionMode == ReportingInteractionMode.NOTIFICATION && Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
reportingInteractionMode = ReportingInteractionMode.SILENT;
}
} else {
reportingInteractionMode = ReportingInteractionMode.SILENT;

Expand Down
12 changes: 6 additions & 6 deletions acra/src/main/java/org/acra/sender/SenderService.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import android.content.Intent;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.support.v4.app.JobIntentService;

import org.acra.ACRA;
import org.acra.ACRAConstants;
Expand All @@ -34,22 +35,21 @@

import static org.acra.ACRA.LOG_TAG;

public class SenderService extends IntentService {
public class SenderService extends JobIntentService {

public static final String EXTRA_ONLY_SEND_SILENT_REPORTS = "onlySendSilentReports";
public static final String EXTRA_APPROVE_REPORTS_FIRST = "approveReportsFirst";
public static final String EXTRA_ACRA_CONFIG = "acraConfig";

private final ReportLocator locator = new ReportLocator(this);
private final ReportLocator locator;

public SenderService() {
super("ACRA SenderService");
setIntentRedelivery(true);
locator = new ReportLocator(this);
}

@Override
protected void onHandleIntent(@Nullable final Intent intent) {
if (intent == null || !intent.hasExtra(EXTRA_ACRA_CONFIG)) {
protected void onHandleWork(@NonNull final Intent intent) {
if (!intent.hasExtra(EXTRA_ACRA_CONFIG)) {
if(ACRA.DEV_LOGGING) ACRA.log.d(LOG_TAG, "SenderService was started but no valid intent was delivered, will now quit");
return;
}
Expand Down
5 changes: 3 additions & 2 deletions acra/src/main/java/org/acra/sender/SenderServiceStarter.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import android.content.Context;
import android.content.Intent;
import android.support.annotation.NonNull;
import android.support.v4.app.JobIntentService;

import org.acra.ACRA;
import org.acra.config.ACRAConfiguration;
Expand Down Expand Up @@ -30,10 +31,10 @@ public SenderServiceStarter(@NonNull Context context, @NonNull ACRAConfiguration
*/
public void startService(boolean onlySendSilentReports, boolean approveReportsFirst) {
if (ACRA.DEV_LOGGING) ACRA.log.d(LOG_TAG, "About to start SenderService");
final Intent intent = new Intent(context, SenderService.class);
final Intent intent = new Intent();
intent.putExtra(SenderService.EXTRA_ONLY_SEND_SILENT_REPORTS, onlySendSilentReports);
intent.putExtra(SenderService.EXTRA_APPROVE_REPORTS_FIRST, approveReportsFirst);
intent.putExtra(SenderService.EXTRA_ACRA_CONFIG, config);
context.startService(intent);
JobIntentService.enqueueWork(context, SenderService.class, 0, intent);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import android.content.Context;
import android.content.SharedPreferences;
import android.content.pm.PackageInfo;
import android.os.Handler;
import android.os.Looper;
import android.support.annotation.NonNull;
import android.widget.Toast;
import org.acra.ACRA;
Expand Down Expand Up @@ -79,7 +81,12 @@ public void sendApprovedReports() {

// Send the approved reports.
final SenderServiceStarter starter = new SenderServiceStarter(context, config);
starter.startService(false, false);
new Handler(Looper.getMainLooper()).post(new Runnable() {
@Override
public void run() {
starter.startService(false, false);
}
});

}

Expand Down
4 changes: 3 additions & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
buildscript {
repositories {
jcenter()
google()
}
dependencies {
classpath 'com.android.tools.build:gradle:2.3.3'
classpath 'com.android.tools.build:gradle:3.1.0-rc02'
classpath 'com.github.dcendents:android-maven-gradle-plugin:1.5'
classpath "org.jfrog.buildinfo:build-info-extractor-gradle:4.4.0"
classpath "io.codearte.gradle.nexus:gradle-nexus-staging-plugin:0.9.0"
Expand All @@ -21,6 +22,7 @@ apply plugin: 'io.codearte.nexus-staging'
allprojects {
repositories {
jcenter()
google()
}
}

Expand Down
4 changes: 2 additions & 2 deletions gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#Fri Feb 17 03:38:03 CET 2017
#Wed Mar 21 18:52:10 CET 2018
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-3.3-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-4.4-all.zip

8 comments on commit 05e9a53

@mikehardy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to say thanks @F43nd1r for supporting the 4.x series with modern API support, I have a port to 5.x ready for AnkiDroid but our minSdkVersion is 15, so the apparent ACRA 5.x requirement of a minSdkVersion of 26 is unsupportable for us.

@F43nd1r
Copy link
Member

@F43nd1r F43nd1r commented on 05e9a53 Jul 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As seen in the readme, the current supported minSdkVersion is 14.

@mikehardy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interesting! I was getting compile errors indicating some language features used in 5.x required >= 24 and others required >=26. I'll re-examine that as your note indicates I must be doing something wrong. Thanks for replying

@mikehardy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was the first pre-requisite, clearly documented, for Java 1.8. I have AnkiDroid up to 5.x now, though if it is worth anything I also qualified 4.11 in testing so this release worked well too

@F43nd1r
Copy link
Member

@F43nd1r F43nd1r commented on 05e9a53 Jul 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know that both currently supported versions are doing fine in production apps (which aren't my own).

I see you were upgrading from a version which was released before I joined the project. Did you need any information, which was missing/hidden in the wiki?

(for future reference, related PR is ankidroid/Anki-Android#4893)

@mikehardy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do have some thoughts since it is all very fresh in my mind...they're all small I think

I think in the custom Dialog where you extend the provided base object the API move from using create in the subclass to init was in the javadoc but not the migration guide, so my experience was to discover a bug (exception "already attached") during testing, then discover the javadoc on the superclass method init(), then fix. I was doing test-driven development so this wasn't the worst for me but not all people develop with tests that way and it could lead to surprises. That's a possible migration issue. Also the need for ":acra" in the Manifest was after the switch from 4.6 to >=4.7 with the worker thread and it wasn't noted in Migration but was in BasicSetup

If people are going to Oreo and using Notifications they need to know to assign a channel to that interaction method (we aren't, but I noted that)

If people have dev settings on the device for no activity saving and no background services, the Toast notification lingers until the app is killed post-send, as the hide toast message from the looper can't be sent because of a dead object exception. Those device settings are kind of ridiculous but they are also how we triggered the crash in our app via an underlying bug, and it was unexpected behavior from ACRA

Other than that, I was a little unsure when I got the main CoreConfigurationBuilder, then chained more than one plugin ConfigurationBuilders off of it that when I did an .init at the end it would all come together. Again, with test-driven development I was able to prove to myself the configuration was working so I had confidence in reality :-), but in theory it would be cool to have a programmatic configuration example that showed something like the AnkiDroid combo usage of Dialog (for comments and user-configuration) + Toast (to tell the user a dialog is coming...) in runtime configuration. I think that's just one more line in the example runtime config block in BasicSetup wiki page

ACRA.init complains if you initialize it more than once but it isn't noted anywhere that is illegal. What if you want to change the configuration post-init? Is that possible?

On the testing side, it seemed hard to test for me. ACRA.init can only be called once making configuration testing a bit tricky. And I would love to have the ability to programmatically trigger ACRA as if the app crashed, and have a TestSender I could easily register that would get the whole report all wrapped up in a way that was ready to inspect/assert on. Something like ACRATest @@RunWith(AndroidJunit4.class), a @test method, and in there I could do an inner class that extended an ACRA TestSender object which would have a hook to wait for reports, then I could ACRA.doCrashSimulation() or something and ACRA would do it's whole Interaction / report-generation thing but do the callback to my test and send me a ACRAReportJSONWrapper or something? I'm designing a feature most probably won't use, on the fly, in a text box on the web, that I won't implement so ignore the idea all you like :-). Maybe a sample-app with that test, and a drop-down that hooked "ACRA.doCrashSimulation()" to show everyone how to get it right

Totally separately, I was unable to get acra-storage / acralyzer couchdb replication to work in Cloudant using the easy method described in the documentation and had to use the manual method. Not sure why - I had never used couchdb, or cloudant, or a couchapp before so possible a completely trivial mistake, but the manual instructions worked fine, so that was okay too. Not any ACRA-related problem but the documents were so excellent looking and yet didn't work?

All in all for a big API change upgrade, it was a good experience but you seem sincerely curious so I gave you everything for feedback :-). Thanks for a great project in ACRA and acralyzer, they are very useful for us.

@F43nd1r
Copy link
Member

@F43nd1r F43nd1r commented on 05e9a53 Jul 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in the custom Dialog where you extend the provided base object the API move from using create in the subclass to init was in the javadoc but not the migration guide

This change was already in later versions of 4, which is why it is not noted in the migration guide. Same for

Also the need for ":acra" in the Manifest was after the switch from 4.6 to >=4.7 with the worker thread and it wasn't noted in Migration but was in BasicSetup


If people are going to Oreo and using Notifications they need to know to assign a channel to that interaction method (we aren't, but I noted that)

If using annotations, @AcraNotification doesn't have a default value for resChannelName, which means it is required. Added a note to https://github.com/ACRA/acra/wiki/Interactions#notification anyways.


If people have dev settings on the device for no activity saving and no background services, the Toast notification lingers until the app is killed post-send, as the hide toast message from the looper can't be sent because of a dead object exception.

That doesn't sound good. Any ideas how one would work around that?


Other than that, I was a little unsure when I got the main CoreConfigurationBuilder, then chained more than one plugin ConfigurationBuilders off of it that when I did an .init at the end it would all come together.

Added example configurations at https://github.com/ACRA/acra/wiki/Examples#example-builder-configuration, and linked them at https://github.com/ACRA/acra/wiki/BasicSetup#2-configuration. Feel free to add more examples if you feel like they provide something the existing ones don't.


ACRA.init can only be called once making configuration testing a bit tricky.

ACRA.init being single use has always been this way. There was some discussion in #439 (comment). I think it should be safe to allow. I'll check for sideeffects.

What if you want to change the configuration post-init? Is that possible?

Not allowing configuration changes is a design choice made by previous developers which I don't fully support. I think I'll make it possible but discourage usage for anything else than testing.


And I would love to have the ability to programmatically trigger ACRA as if the app crashed, and have a TestSender I could easily register that would get the whole report all wrapped up in a way that was ready to inspect/assert on.

Sounds like a combination of ACRA.getErrorReporter().handleException(...) and ReportingAdministrator.shouldSendReport will do what you want.


Totally separately, I was unable to get acra-storage / acralyzer couchdb replication to work in Cloudant using the easy method described in the documentation and had to use the manual method.

Replication requires an prototype database. That databse is hosted on a server controlled by the developer of acralyzer, which has issues. The issue is known: ACRA/acralyzer#133. Sorry, nothing I can do there.


All in all for a big API change upgrade, it was a good experience but you seem sincerely curious so I gave you everything for feedback :-). Thanks for a great project in ACRA and acralyzer, they are very useful for us.

Thanks for taking the time and writing this all down :-). I think it is always important to hear opinions and experiences other than mine, especially on the documentation. In the end a lot of things which seem obvious to me as the developer aren't obvious to the user.

@mikehardy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea how to avoid the persistent toast unfortunately.. At least the required settings to cause it is very rare and only. In developer settings

Please sign in to comment.