search

Limegrass / Wanicchou

WIP Online J-J Dictionary Android App with Anki Support - Essentially archived at the moment, working on a lighter alternative that's slightly derailed due to other efforts
GNU General Public License v3.0
14 stars 2 forks source link

Anki Import button crash #48

Closed Limegrass closed 5 years ago

Limegrass commented 5 years ago

Google Play review seems to suggests that the Anki Import button is appearing without access to Anki, causing a crash upon button press. The button should not reveal if Anki Droid is not installed and permission was not given to access it.

cassdeckard commented 5 years ago

I'm not sure about the button appearing without access to Anki, but I can reproduce a crash by:

  1. Long press AnkiDroid app icon and select "App Info"
  2. Open "Permissions", disable "Storage"
  3. Open Wanicchou and hit the Anki "send" button
cassdeckard commented 5 years ago

This actually happens in AnkiDroid's API sample app too so I opened a similar issue there: https://github.com/ankidroid/apisample/issues/10

One workaround would be to catch the IllegalStateException and display an informative message to the user.

Limegrass commented 5 years ago

@mattdeckard Thanks for the great investigative work. It looks like AnkiDroid itself is not functional without the storage permission since it defaults to a save location where it is required, but it would definitely be nice to have an API call available that returns whether the storage permission is granted to Anki and/or an api call that request storage permission on the behalf of AnkiDroid if it doesn't have it. I'd prefer that over adding error handling if possible.

If you have more time on your hands, you could submit an issue to the AnkiDroid project to add such an API call, or even add it yourself. Otherwise, I can add the API calls and submit a pull request when I finish up re-architecturing Wanicchou itself. Though there's room for debate on whether it should even exist if AnkiDroid is not functional without it, so who knows if it'll get accepted.

The button appearing issue was a result from an accidental commit when I was testing and rushing to get this out, since I was anticipated not having time to work on it soon when I released it. Luckily I have the time again, and it's resolved in the typical case (where the user grants AnkiDroid storage permission.) in a commit to the 1.0.2 branch, but it'll still be a just bit more until I'm ready to release that version due to rearchitecturing.

cassdeckard commented 5 years ago

Sure thing! Thank you for making this app. I've found it so useful, it's become my main method of adding cards to my Japanese deck. I used to use Aedict3 to add them from JMDict entries and then manually replaced the English definitions with ones from Sanseido using AnkiDroid's card editor. So you can imagine how much time this app has saved me! 😄

I added an issue here

I'd be happy to work on the PR too but I'm going to try to wait until that issue gets some discussion cause I'm not sure which route the AnkiDroid maintainers will want to go.

Limegrass commented 5 years ago

Glad to hear it! That's definitely the right route in the API case.

But, really. You have no idea how happy knowing someone else actually gets use from the app. Despite the fact that a lot of my dev has been focused on self-satisfaction... Definitely more feature focused soon(tm) though. Probably additional sources and send-to Wanicchou (for direct searching of highlighted text) after things are cleaned up.

Limegrass commented 5 years ago

@mattdeckard Once again, thank you for the research. I added a check and message as suggested. It completely slipped me to just check their permissions with Android's PackageManager, and they do conveniently provide their package name to check.

With that said, it would probably help a lot to have the apisample updated so other developers know how to handle the issue at least. I personally would still appreciate an API method for checking API availability and Storage permission (or both as a combination and being encompassed by a method like isApiAvailable from the apisample), but we'll see how they feel about it (eventually). Feels a little silly to me to expose the app's package name in an API method and forcing every developer to write the code to perform the same two checks.