-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
F43nd1r
committed
Mar 21, 2018
1 parent
3af731a
commit 05e9a53
Showing
9 changed files
with
37 additions
and
20 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
05e9a53
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.
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.
05e9a53
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.
As seen in the readme, the current supported minSdkVersion is 14.
05e9a53
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.
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
05e9a53
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 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
05e9a53
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.
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)
05e9a53
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 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.
05e9a53
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 change was already in later versions of 4, which is why it is not noted in the migration guide. Same for
If using annotations,
@AcraNotification
doesn't have a default value forresChannelName
, which means it is required. Added a note to https://github.com/ACRA/acra/wiki/Interactions#notification anyways.That doesn't sound good. Any ideas how one would work around that?
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 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.
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.
Sounds like a combination of
ACRA.getErrorReporter().handleException(...)
andReportingAdministrator.shouldSendReport
will do what you want.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.
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.
05e9a53
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 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