-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix crash in flashcard viewer when socket permission is denied #17989
Conversation
First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible. |
627944b
to
459591c
Compare
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.
The amount of people using an alternative OS and deliberately negating AnkIDroid the internet permission is quite small, the work to maintain any solution is quite big, and the reward is mostly non existent, so fallbacks and alternative solutions aren't worth it. You need to find a definitive solution that works for everyone and isn't a pain to maintain.
If you want to really solve that, you need either to replace all of the POST requests in the Anki desktop repo, which I find extremely unlikely to happen, or to discover some way to intercept POST requests in Android without a server. I tried it, other people here tried it, nobody managed to do it.
the server wasn't actually used in this screen
It is. See one of the comments below
a fallback when server cannot be started
A pain to maintain, and the tendency of Anki is to use more and more POST requests, including the reviewer. So it will become less and less functional with a fallback.
My understanding is that the socket is needed to process POST requests, and it looks like all interaction there, is in JavaScript.
It does use POST requests.
For pages needing the socket, I can try making a more friendly crash screen, explaining the socket permission is needed.
That's acceptable IMO. You can open the PR.
a fallback screen with only basic deck options shown there and a warning that more options are available when turning network permission on
Not worth it.
val server = AnkiServer(this).also { it.start() } | ||
|
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 breaks the i18nresources
POST request used by image occlusion cards
@@ -1549,7 +1548,7 @@ abstract class AbstractFlashcardViewer : | |||
if (card != null) { | |||
card.settings.mediaPlaybackRequiresUserGesture = !cardMediaPlayer.config.autoplay | |||
card.loadDataWithBaseURL( | |||
server.baseUrl(), | |||
getMediaBaseUrl(getMediaDirectory(AnkiDroidApp.instance).path), |
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 breaks javascript imports from other files
Thank you for your time reviewing this!
While I understand that only GrapheneOS users can deny this permission, the reason why it is possible to do it is to increase security, which all devices would benefit from if we could avoid the socket completely. See this quote from original thread:
I understand the server can't be avoided as I initially thought, and that you don’t want the extra maintenance cost for other screens. For the flashcard viewer, I just pushed a different implementation to avoid breaking things on devices where it would previously work (it's not showing in the closed PR, so please follow the link). For devices without the socket permission, the fallback solution will apply and a toast will be shown to explain that some cards may fail to render properly (do you have some of these specific decks I can try?). Previously, it would crash with a generic crash screen and nothing would render. I believe the extra maintenance cost of this PR is minimal, compared to the gains it can provide for basic uses. (I would prefer not having to maintain a fork just for this). I understand my other suggestions have a too important maintenance cost, and I will not try to convince you for them ;)
Is there a summary of what was tried and why it didn't work? Like what prevented using a Javascript <> Kotlin bridge instead of POST requests, for example? |
Firstly: THANK YOU for the enthusiasm and well-documented PR TL;DR: This is probably not going to make it to production, and I'd agree with Brayan that better error messages would be appreciated, and an easy win. I've provided some context for how & why things are the Frankensteinian monster that they are, and I don't have much free time, but I'll endeavour to listen to proposals to get this through. I really appreciate the high quality PR, and effort should be reciprocated. An update which fails to support default functionality won't be accepted, as I suspect it will lead to abuse of contributors from technical users (sorry, this sucks, because your updated PR improves things). We can't reopen this PR now it's been closed or force-pushed to. It would be best to move this discussion to #15718 for posterity. Feel free to copy/paste anything you feel is useful. The cleanest way to resolve this would be to modify your OS: revoking the Reworking the app infrastructure to accommodate such a niche use case is going to be a fairly significant undertaking on your behalf. If you think you can make it work in a maintainable manner, then it'll be considered, but I don't want to give you false hope that it'll be merged. We're going to need to maintain this long after a motivated & skilled individual loses interest.
We barely have the time or people to keep the lights on right now, so we're not going to be able to put the effort into developing this change, but it feels disrespectful to ask for a significant amount of work from you, where there is a slim chance of it making it into production. I feel the best path to continue would be to provide some context, and see if you feel there's a reasonable path to getting this merged. Hacking in a change which breaks some default note types is a non-starter. "We don't support your use case, enable I'd agree with Brayan that better error messages would be appreciated, and an easy win. Context Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/Reviewer.kt Lines 1466 to 1485 in f9ee824
AnkiDroid has undertaken significant effort to reuse Anki Desktop's code (this stops us being a port, and ensures that there's a 100% functionality match between all the Anki clients) Anki Desktop uses a HTTP-based API to communicate between the backend (Rust) and the frontend (Svelte/TS). Protobufs define an API. We codegen clients from these protobufs:
Our implementation of the Kotlin-based codegen is in: https://github.com/ankidroid/Anki-Android-Backend, which references https://github.com/ankitects/anki/tree/main/ts for the Svelte pages An example of Svelte calling through to these backend methods: If you want to move functionality to a JavaScriptInterface, you probably want to call through to Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/pages/PostRequestHandler.kt Lines 63 to 94 in 988402a
Generating the We handle video and audio streaming with the server, but as far as I recall, this likely won't cause an issue, and this PR would work correctly. Finally, the screen that you're modifying |
Purpose / Description
I would like to use Anki offline (without the "Network" permission from GrapheneOS turned on).
In #15718, you showed potential interest for a PR.
Fixes
android.permission.INTERNET
is necessary for people with other OS #15718 by not crashing on flashcard view (deck options and statistics screens are still unavailable)Approach
How does this change address the problem?
I initially wanted to implement a fallback when
server
cannot be started, but I noticed that in the flashcard viewer, the server wasn't actually used in this screen (or at least, it was my understanding. If that's not the case, I can make an alternative implementation with atry
/catch
).This change allows to use AnkiDroid without a socket for basic needs: creating a deck and training with decks (with default options).
Depending on your openness, I can make other PRs to fix other pages using the socket from
AnkiServer
.POST
requests, and it looks like all interaction there, is in JavaScript.Thank you for reading
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration (SDK version(s), emulator or physical, etc)
I tested with a deck using sound, images and/or basic HTML, and it worked, where it would previously crash.
Learning (optional, can help others)
Describe the research stage
Links to blog posts, patterns, libraries or addons used to solve this problem
Checklist
Please, go through these checks before submitting the PR.