Fix export profile on pre-19 API levels #320

penguin359 · 2022-01-29T20:06:43Z

This should fix an issue with the Export Profile button on older Android APIs. The directory used for exporting the profile is stored in the top-level of the external storage and not in the Documents/ subfolder and so needs to be in the list of allowed locations for the FileProvider. I have added an instrumented test that can be seen failing on older APIs, but passes on API 19 and newer. With the fix also included, it is now seen passing on all API levels tested.

This should resolve an issue reported in issue #68.

Added a test to verify that a chooser is opened for exporting the profile
Support exporting profiles on pre-19 APIs

…file

penguin359 · 2022-01-31T04:50:59Z

I need to take my own advice and make sure tests are passing on all targets that they were passing on previously before submitting a pull request. It appears that I introduced a regression in my earlier fix. While the fix did work on API 16, it broke on everything API 19+ as I used the same name for those paths. Giving unique names to both the paths for pre-19 and 19+ seems to fix the issue overall. This is currently passing tests on API 15, 16, 19, 24, 28, and 32 on my local workstation reliably.

I had originally ignored it when I saw it failing on GitHub CI and ignored it as just GitHub being flaky, but it turned out to be failing for a couple of good reasons. One was that the AVDs on GitHub had their SD card disabled so there was no place to save the profile. I've now fixed that on the ui-testing branch. The other issue was that I had not re-tested APIs 19+ after applying my original fix.

The feature this tests verifies requires an SD card and will fail if it's not available.

On pre-19 APIs, the APRSdroid folder is located directly in the top-level of the external storage instead of inside Documents.

penguin359 · 2022-01-31T08:48:32Z

I was hoping to be able to present a fully-passing report on GitHub CI, but I'm still getting a failure on API 24 that isn't showing up anywhere else. I can run it on my local workstation with or without an SD card and it, like all other combinations tested, is passing, but API 24 on GitHub has always reported a failure. It looks like this might be a different bug that hits some weird corner case and unrelated to the original issue. You can see the latest attempt on my profile-testing-debug branch here:

https://github.com/penguin359/aprsdroid/actions/runs/1771498639

If you download the artifacts with the reports at the bottom of the page, you'll find the logcat with the failure along with the HTML reports of the test run. The log shows a FileNotFound exception on PrintWriter() init so it's like the mkdirs() operation is not able to do it's job. I am still debugging this issue, but it doesn't appear to be intermittent and consistently fails at this point.

PrefsAct: Caught exception sharing file: java.io.FileNotFoundException: /storage/emulated/0/Documents/APRSdroid/profile-20220131-0830.aprs (No such file or directory)
PrefsAct: java.io.FileNotFoundException: /storage/emulated/0/Documents/APRSdroid/profile-20220131-0830.aprs (No such file or directory)
PrefsAct: 	at java.io.FileOutputStream.open(Native Method)
PrefsAct: 	at java.io.FileOutputStream.<init>(FileOutputStream.java:221)
PrefsAct: 	at java.io.FileOutputStream.<init>(FileOutputStream.java:169)
PrefsAct: 	at java.io.PrintWriter.<init>(PrintWriter.java:263)
PrefsAct: 	at org.aprsdroid.app.PrefsAct.exportPrefs(PrefsAct.scala:37)

penguin359 · 2022-02-01T07:36:38Z

OK, I finally figured out the final issue with the GitHub CI test runner. For whatever reason, API 24 requires that WRITE_EXTERNAL_STORAGE be granted explicitly. I cannot explain why I can't reproduce this issue locally except for when I actively remove Storage from the app permissions after installation. Simply uninstalling the app and running the test does not seem to trigger the issue. Runtime permissions were added in API 22 which explains why 15 and 21 pass on GitHub CI, but it doesn't explain why it both fails on 24 and passes on 31 every time the test was run prior to adding the GrantPermissionRule. It makes me wonder if we need to add a check for WRITE_EXTERNAL_STORAGE in the PrefsAct for Export Profile similar to what you already had to do in MapAct, but I wish I could reproduce it to confirm the situation. I guess this permission is some kind of soft restricted permission, but I don't quite understand what exactly that means. Anyway, I finally have the report I was looking for. Here it is:

https://github.com/penguin359/aprsdroid/actions/runs/1776836153

And, just for good measure, here is the same test, but with the fix reverted showing it failing only on API 15 from the matrix:

https://github.com/penguin359/aprsdroid/actions/runs/1776867411

Those were made by rebasing this branch on top of github-ci (#315) and pushing it. This PR is based directly on master.

ge0rg · 2022-02-08T18:37:47Z

This PR combines the actual fix with test integration. While I don't disagree with this, we haven't merged the github-ci branch yet. I've had a look at didn't find any new &/ obvious issues, so I suppose I can go forward merging #315?

ge0rg · 2022-02-08T18:29:47Z

src/PrefsAct.scala

-			case e : Exception => Toast.makeText(this, e.getMessage(), Toast.LENGTH_LONG).show()
+			case e : Exception => {
+				Toast.makeText(this, e.getMessage(), Toast.LENGTH_LONG).show()
+				Log.i(TAG, "Caught exception sharing file: " + e, e)


Not sure if this shouldn't be Log.e or at least Log.w ;)

Good point! Probably any errors that warrant generating an on-screen toast message should probably be warn or higher.

After further thought, if we ever hit that catch, it basically means that a piece of user functionality was not able to do its job so it should probably be an error-level log. I can push an update this evening if preferred or, I think this could even be edited from the web though without the GitHub CI branch in place to automatically compile and test, I don't know if I'd trust that.

penguin359 · 2022-02-08T18:43:41Z

This branch was developed on github-ci and test results produced from it, but it does not depend on anything in that branch and cleanly rebased to master. The test framework that it uses has already been merged in to master and you should be able to run the tests in this PR with a ./gradlew connectedCheck. If you revert the commit with the fix, you should see the results change and show the bug. The github-ci branch just runs all tests found on GitHub. You should be able to merge the PRs in either order. I can always rebase again if preferred.

penguin359 · 2022-02-08T18:49:16Z

But yes, #315 should be completely safe because it just adds one new file, .github/workflows/android.yml, and makes a small modification to the README to add the CI results badge generated from the GitHub workflow. The YAML file just tells GitHub to spend a few resources that they provide for free to run the test suite. I haven't seen the Scala plugin complain about it at all. ;-)

penguin359 · 2022-02-08T18:51:43Z

This is the branch that requires more careful review because my very first PR did break things which I only noticed after running the test suite against a wide range of API levels. Now that it's passing 100% everywhere, I'm feeling pretty good about it.

penguin359 · 2022-02-08T19:14:26Z

src/PrefsAct.scala

-			case e : Exception => Toast.makeText(this, e.getMessage(), Toast.LENGTH_LONG).show()
+			case e : Exception => {
+				Toast.makeText(this, e.getMessage(), Toast.LENGTH_LONG).show()
+				Log.i(TAG, "Caught exception sharing file: " + e, e)


After further thought, if we ever hit that catch, it basically means that a piece of user functionality was not able to do its job so it should probably be an error-level log. I can push an update this evening if preferred or, I think this could even be edited from the web though without the GitHub CI branch in place to automatically compile and test, I don't know if I'd trust that.

penguin359 · 2022-02-08T19:18:08Z

src/PrefsAct.scala

 import java.text.SimpleDateFormat
 import java.io.{File, PrintWriter}
 import java.util.Date

 import org.json.JSONObject

 class PrefsAct extends PreferenceActivity {
+	val TAG = "APRSdroid.PrefsAct"


Minor nit-pick, this would normally be a Java private final static and I believe it's currently an instance variable. I'd have to look up what the Scala version of that is. I believe I copied this from the style used elsewhere, however.

penguin359 mentioned this pull request Jan 29, 2022

Feature Request: Profiles #68

Open

penguin359 force-pushed the profile-testing branch from 95828eb to 1d9b9c1 Compare January 30, 2022 09:19

Added a test to verify that a chooser is opened for exporting the pro…

98db421

…file

penguin359 force-pushed the profile-testing branch 2 times, most recently from 5e75138 to 1756780 Compare January 31, 2022 03:06

penguin359 added 3 commits January 30, 2022 23:53

Log any exceptions generated when attempting to export profile

744526e

Don't run tests that require an SD card when it's not present

95b7259

The feature this tests verifies requires an SD card and will fail if it's not available.

Support exporting profiles on pre-19 APIs

597618a

On pre-19 APIs, the APRSdroid folder is located directly in the top-level of the external storage instead of inside Documents.

penguin359 force-pushed the profile-testing branch from 1756780 to 597618a Compare January 31, 2022 07:54

Ensure storage permissions are granted for the test

214b77d

ge0rg reviewed Feb 8, 2022

View reviewed changes

penguin359 commented Feb 8, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix export profile on pre-19 API levels #320

Fix export profile on pre-19 API levels #320

penguin359 commented Jan 29, 2022

penguin359 commented Jan 31, 2022 •

edited

Loading

penguin359 commented Jan 31, 2022

penguin359 commented Feb 1, 2022

ge0rg commented Feb 8, 2022

ge0rg Feb 8, 2022

penguin359 Feb 8, 2022

penguin359 Feb 8, 2022

penguin359 commented Feb 8, 2022

penguin359 commented Feb 8, 2022

penguin359 commented Feb 8, 2022

penguin359 Feb 8, 2022

penguin359 Feb 8, 2022

Fix export profile on pre-19 API levels #320

Are you sure you want to change the base?

Fix export profile on pre-19 API levels #320

Conversation

penguin359 commented Jan 29, 2022

penguin359 commented Jan 31, 2022 • edited Loading

penguin359 commented Jan 31, 2022

penguin359 commented Feb 1, 2022

ge0rg commented Feb 8, 2022

ge0rg Feb 8, 2022

Choose a reason for hiding this comment

penguin359 Feb 8, 2022

Choose a reason for hiding this comment

penguin359 Feb 8, 2022

Choose a reason for hiding this comment

penguin359 commented Feb 8, 2022

penguin359 commented Feb 8, 2022

penguin359 commented Feb 8, 2022

penguin359 Feb 8, 2022

Choose a reason for hiding this comment

penguin359 Feb 8, 2022

Choose a reason for hiding this comment

penguin359 commented Jan 31, 2022 •

edited

Loading