Skip to content
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

Add in-app camera #6763

Open
wants to merge 35 commits into
base: develop
Choose a base branch
from
Open

Add in-app camera #6763

wants to merge 35 commits into from

Conversation

p1gp1g
Copy link
Contributor

@p1gp1g p1gp1g commented Aug 7, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

This PR add a built-in / in-app camera to element as a lab feature. If the feature is not enable, it adds a button to attachment list to open the native camera for video.

Motivation and context

Currently, users have to choose between :

  • choosing a default behavior for the camera (photo or video)
  • or setting a default to one type.

That means, it adds clicks to take a photo/video or users won't change media type (it requires to go to settings to change the default behavior).

This fixes #1389, #2224, #3331, #1623 and maybe others.

Screenshots / GIFs

Outdated screenshots
  • The camera view before recording a video
  • The camera view while recording a video
  • The camera view before taking a photo
  • Only one attachment button to open a camera when the feature is enabled
  • 2 attachment buttons to open a camera when the feature is disabled : one for a picture, one for a video

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 11

Checklist

@bmarty bmarty added the Z-Community-PR Issue is solved by a community member's PR label Aug 16, 2022
@bmarty
Copy link
Member

bmarty commented Aug 16, 2022

Thanks for the PR.
@onurays can you review it please? I think you already worked on this part previously.

@bmarty bmarty requested a review from onurays August 16, 2022 15:07
@DoM1niC
Copy link

DoM1niC commented Aug 24, 2022

works fine in my Fork!
But two things here, when I send pictures in Landscape the orientation is wrong and in Video mode the light is missing :( @p1gp1g

@DoM1niC
Copy link

DoM1niC commented Aug 26, 2022

The fragment removal (2b5920e) brick the AttachmentsCameraFragment @p1gp1g

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Aug 26, 2022

The fragment removal (2b5920e) brick the AttachmentsCameraFragment @p1gp1g

Thanks, it should be easy to fix. I'll check that when I can

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Aug 27, 2022

Rebased (& fix the fragment)

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Aug 27, 2022

@DoM1niC : Here is the orientation support :)

@p1gp1g p1gp1g marked this pull request as draft August 29, 2022 09:08
@p1gp1g
Copy link
Contributor Author

p1gp1g commented Aug 29, 2022

I think it looks OK now, so it is ready for review :)

@p1gp1g p1gp1g marked this pull request as ready for review August 29, 2022 20:49
@onurays
Copy link
Contributor

onurays commented Sep 5, 2022

Thanks for the PR! I have some UX-related remarks:

  1. I am a little bit surprised when the app suddenly requests audio recording permission. My intention was just to take a photo.
  2. In landscape mode, the orientation of icons is wrong (90 degrees rotated).
  3. When I want to record a video camera and video icons are switching their positions. This seems not like a common UX for me.
  4. While recording a video I expected to see a timer.

We need a design review before the code review. @gaelledel can you or someone else in the team have a look (I can send you an apk if needed).

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Sep 5, 2022

Thanks for the review !

1. I am a little bit surprised when the app suddenly requests audio recording permission. My intention was just to take a photo.

That's needed for the video record, I guess you already gave the authorization for the camera so you have received the single last request. We can change the behavior to ask for this permission only once the user try to record a video.

2. In landscape mode, the orientation of icons is wrong (90 degrees rotated).

Can you give a screenshot of it ? It will help to see what's wrong. There was an issue with the orientationListener being enabled onCreate instead of onStart, it was probably because of that.

3. When I want to record a video camera and video icons are switching their positions. This seems not like a common UX for me.

The UX is inspired from Open Camera. We can change for another one (edit: cf note behind)

4. While recording a video I expected to see a timer.

Good idea, and done :)

Edit: Thanks to a long delay for my train, I've implement another UX : photo on click and video on longclick + flash for video,
I m posting screenshots later

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Sep 6, 2022

Here are the screenshots with the new UX

Outdated screenshots

Edit: Outdated screenshots, see first post for the last screenshots.

@kojid0
Copy link
Contributor

kojid0 commented Sep 6, 2022

Would it be possible to add a button to switch the front/back camera?
Like here, from Telegram. WhatsApp has a similar icon.
photo_resized

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Sep 6, 2022

Would it be possible to add a button to switch the front/back camera? Like here, from Telegram. WhatsApp has a similar icon.

That's the purpose of this button:

image

We may change to another icon if it is not obvious

@kojid0
Copy link
Contributor

kojid0 commented Sep 6, 2022

Oh, thanks for the explanation. I thought it was for video recording but I just looked closer and saw the arrow 😄.
I suggest to add "Tap for photo, hold for video" in order to make users understand that there is a "hidden" video record feature.

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Sep 12, 2022

I have updated the screenshots on the first post

@DoM1niC
Copy link

DoM1niC commented Sep 29, 2022

The Portrait and Landscaping is wrong here after Capture
Screenshot_20220929-174453.png

@p1gp1g
Copy link
Contributor Author

p1gp1g commented Sep 29, 2022

The Portrait and Landscaping is wrong here after Capture <screenshot>

Does it happen often/Do you have instruction to reproduce ? I don't have this version installed anymore right now

@DoM1niC
Copy link

DoM1niC commented Oct 1, 2022

The Portrait and Landscaping is wrong here after Capture

Does it happen often/Do you have instruction to reproduce ? I don't have this version installed anymore right now

I use your last Commits in my fork.... I just do a photo in portrait or landscape and the orientation still wrong for me...

@DoM1niC
Copy link

DoM1niC commented Oct 5, 2022

THX for the fix ae2a920

we need some resolve :(

@DoM1niC
Copy link

DoM1niC commented Oct 8, 2022

@p1gp1g can you resolve all issues from your current code ? THX 4 your Work!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InApp Camera
5 participants