Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

enhancement: allow camera access when using image occlusion

Open criticalAY opened this issue 1 year ago • 26 comments

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

criticalAY avatar Dec 19 '23 10:12 criticalAY

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

criticalAY avatar Dec 19 '23 10:12 criticalAY

Do it, if it's not too much work

david-allison avatar Dec 19 '23 10:12 david-allison

Simply used the icons and switched to wrap_content

criticalAY avatar Dec 19 '23 11:12 criticalAY

Screenshot_2023-12-19-16-45-10-06_7b5228b5a93c00ecba98ecb0735c59e2.jpg

criticalAY avatar Dec 19 '23 11:12 criticalAY

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? image

snowtimeglass avatar Dec 19 '23 11:12 snowtimeglass

I am fine with either one and whatever the others like

criticalAY avatar Dec 19 '23 12:12 criticalAY

Not sure here, but is a cancel button necessary?

BrayanDSO avatar Dec 19 '23 12:12 BrayanDSO

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

david-allison avatar Dec 19 '23 12:12 david-allison

Also, please add screenshots of this in the other themes (plain, dark and black)

BrayanDSO avatar Dec 19 '23 12:12 BrayanDSO

image Dark: image Black: image

criticalAY avatar Dec 19 '23 12:12 criticalAY

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:

image

BrayanDSO avatar Dec 19 '23 12:12 BrayanDSO

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

image

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.
image Based on this viewpoint on second thought, the current structure above might be a bit unnatural.

snowtimeglass avatar Dec 19 '23 15:12 snowtimeglass

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

criticalAY avatar Dec 19 '23 16:12 criticalAY

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,

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

image image

snowtimeglass avatar Dec 19 '23 17:12 snowtimeglass

I am making it consistent with Anki desktop

criticalAY avatar Dec 19 '23 17:12 criticalAY

@BrayanDSO no pressure - just wanted to mention I think this is pending a re-look from you based on the comment history ?

mikehardy avatar Jan 03 '24 19:01 mikehardy

Busy. Don't block this because of me

BrayanDSO avatar Jan 17 '24 21:01 BrayanDSO

commit history is not reviewable commit by commit / it's not useful

image

mikehardy avatar Jan 20 '24 19:01 mikehardy

Will rebase

criticalAY avatar Jan 20 '24 19:01 criticalAY

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.

oakkitten avatar Feb 11 '24 22:02 oakkitten

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

criticalAY avatar Feb 15 '24 16:02 criticalAY

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.

oakkitten avatar Feb 15 '24 20:02 oakkitten

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

github-actions[bot] avatar Feb 29 '24 20:02 github-actions[bot]

@david-allison could you please remove the stale tag, I will get back to this in few days

criticalAY avatar Feb 29 '24 20:02 criticalAY

image image

criticalAY avatar Apr 21 '24 15:04 criticalAY

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 avatar Apr 25 '24 18:04 mikehardy

@mikehardy still blocked on the 2.19 cut I believe

david-allison avatar May 14 '24 09:05 david-allison

Just merge, will retrocut on tag

mikehardy avatar May 14 '24 11:05 mikehardy