frontend/android: prompt to save exported files rather than sharing.#3740
frontend/android: prompt to save exported files rather than sharing.#3740bznein merged 2 commits intoBitBoxSwiss:masterfrom
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Fyi the commit titles don't match the commit contents, the second one is just a changelog entry. Would also be nice to add a commit message body explaining the reason for the change. |
bznein
force-pushed
the
codex-test
branch
2 times, most recently
from
23be2db to
d29ff2d
Compare
Thanks, done! |
Beerosagos
left a comment
Beerosagos
left a comment
There was a problem hiding this comment.
I didn't test the code yet, but I think it would make sense to move as much code as possible inside a helper class, since both MainActivity and GoViewModel are already quite cluttered.
Imo the code is a bit hard to follow (two interfaces, back and forth between Activity and ViewModel) and not really Java/Android idiomatic. I tried asking GPT and it suggested using onActivityResult to handle the file picking result. Did you try that?
|
|
||
| @Override | ||
| public void systemOpen(String url) throws Exception { | ||
| if (isLocalFile(url) && saveFileLauncher != null) { |
There was a problem hiding this comment.
There is already a check for local files inside Util.systemOpen, I think you can move the code there
| return true; | ||
| } | ||
|
|
||
| private void saveFileWithPicker(String path) throws Exception { |
There was a problem hiding this comment.
This would be easier to follow by refactoring a few variables, I think. e.g. uri -> outputPath or similar
| private GoViewModel goViewModel; | ||
| private WebViewManager webViewManager; | ||
| private UsbDeviceManager usbDeviceManager; | ||
| private ActivityResultLauncher<Intent> saveFileLauncher; |
There was a problem hiding this comment.
Maybe this can be just a variable instead of a class field?
bznein
force-pushed
the
codex-test
branch
2 times, most recently
from
ce457b8 to
1378d2e
Compare
Beerosagos
left a comment
Beerosagos
left a comment
There was a problem hiding this comment.
tACK.
Note: @bznein and I both contributed to one of the commits, and we both tested and reviewed the outcome
3 participants
Before asking for reviews, here is a check list of the most common things you might need to consider: