Anki-Android
Anki-Android copied to clipboard
enhancement: allow camera access when using image occlusion
Purpose / Description
When adding a image (image occlusion) allow the user to access camera and add image using it along with gallery option
Fixes
- Fixes #15017
Approach
- Adding the option for the camera in note editor, with the help of bottom dialog sheet fragment the user can either choose camera or the gallery as desired.
- Used Bottom dialog sheet or the modal sheet guidelines from Google Android MDC here
- The bottom sheet fix class allow the horizontal height issue when using the bottom sheet in horizontal mode
How Has This Been Tested?
Tested on google emulator and one plus nord ce (different themes)
Screen_recording_20231219_155655.webm
Checklist
Please, go through these checks before submitting the PR.
- [x] You have a descriptive commit message with a short title (first line, max 50 chars).
- [x] You have commented your code, particularly in hard-to-understand areas
- [x] You have performed a self-review of your own code
- [x] UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
- [ ] UI Changes: You have tested your change using the Google Accessibility Scanner
One thought: I'd prefer if the design looked more like Google Keep (it feels a lot more 'Android & clean', as the current nav feels like it belongs on iOS)
This is implemented using a menu can do that I held back as it would have required one more menu file, implementing it and pushing it seems fine to me
Do it, if it's not too much work
Simply used the icons and switched to wrap_content
Looks great. Thank you.
"Select image" may associate with selecting an existing image from gallery, but now that the camera option has been added, how about replacing it with "Add image" like AnkiMobile?
I am fine with either one and whatever the others like
Not sure here, but is a cancel button necessary?
Not sure here, but is a cancel button necessary?
Tapping the scrim (obscured view) is acceptable, and feels like standard functionality. I don't think we need an explicit 'cancel'
They can be dismissed by:
- Tapping the scrim
https://m3.material.io/components/bottom-sheets/guidelines#22deb8a2-3513-4a2e-aa47-c64875e15e12
Also, please add screenshots of this in the other themes (plain, dark and black)
Dark:
Black:
If it is just Camera
and gallery, isn't there a default Android solution for that? As an example, I saw the dialog below in Notion:
The current structure is as follows:
- Select image
- Camera
- Gallery
- Paste Image from Clipboard
How about the following structure:
- Camera
- Files (or Gallery)
- Clipboard
or
- Add image
- Camera
- Files (or Gallery)
- Clipboard
(My focus here is on structure, not naming or order.)
Since the desktop version does not have camera option, the buttons eventually mean the two options: "Files" and "Clipboard". They are lined up on the same and top hierarchy.
- Files
- Clipboard
Since AnkiDroid will have camera option as AnkiMobile, it will have (at least) the three options: "Camera", "Files", "Clipboard". So perhaps the three options should be lined up on a same hierarchy, as the four options in AnkiMobile, and as the two options above in the desktop version.
Based on this viewpoint on second thought, the current structure above might be a bit unnatural.
Using the flow suggested by @snowtimeglass it is much better and made some string changes i.e Add image
instead of Select image
and putting the option to paste image inside the bottom sheet only making more sense,
Coming to the other question
@BrayanDSO using 512
is one of the workaround you can't use the EXPANDED_STATE
due the the 16:9 google factor if you go through the stack overflow link I dropped above in the workaround you would make out why that class and 512, yes we could have used the extension function rather than a class both would have done the same work no difference so doesn't matter if we used a class/function the end goal is to allow the state to be expanded, google have imposed it in M3 not a good idea though it is stupid in some cases they themselves don't follow that in some cases, and this doesn't follow is here our case we can't impose the 16:9 stupid factor as we allow the user to use our app in landscape mode.
If it is just Camera and gallery, isn't there a default Android solution for that? As an example, I saw the dialog below in Notion:
This also requires the intents that we use currently so doesn't matter what we use and different phones have different option like my OnePlus has a share option too in case of Notion which is not there in your case(Samsung).
Lastly, I removed the Fix file as now I am using another workaround using onStart
method which also fixes the issue. if you want to track what I did and why you can refer to the StackOverflow
Using the flow suggested by @snowtimeglass it is much better and made some string changes i.e
Add image
instead ofSelect image
and putting the option to paste image inside the bottom sheet only making more sense,
Thank you so much for your consideration and work. However, with apologies, I have updated the suggestion above. Now I'm more interested in the following structure (the first suggestion in the updated comment above):
- Camera (←New button on "Add" screen. The label is, e.g.,
Take photo with camera
) - Files
- Clipboard
It no longer uses the Add image
string and the bottom sheet.
I think that it would enable users to choose one of the options more instantly and easily, just as on the following structure on the desktop version and on the current AnkiDroid 2.17alpha12:
- Files
- Clipboard
I am making it consistent with Anki desktop
@BrayanDSO no pressure - just wanted to mention I think this is pending a re-look from you based on the comment history ?
Busy. Don't block this because of me
commit history is not reviewable commit by commit / it's not useful
Will rebase
Just a note, I would personally prefer all the photos I take from apps to be saved permanently in a public folder. I think since Q you can use media store API for that, IIRC without needing permission.
Also, I'm seeing that File.createTempFile
is used, that probably won't survive process death, if that is of any concern.
that probably won't survive process death, if that is of any concern.
Why do we want to do that? I don't think we need it here
Opening an external camera app means AnkiDroid can be killed while the camera is running. This used to be a common occurrence back in the day, not sure about how exactly this problem is user-visible on more modern Android versions and modern devices...
Especially seeing how simulating process death just got way harder with the recent AS update.
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically
@david-allison could you please remove the stale
tag, I will get back to this in few days
I'd like to merge this after we cut for 2.18 - I think we've got enough to work on just at the moment for 2.18 and we need to get that out very quickly to meet a Play Store requirement
@mikehardy still blocked on the 2.19 cut I believe
Just merge, will retrocut on tag