Camera extra features #16781
Camera extra features #16781
Conversation
Jenkins BuildsClick to see older builds (8)
|
OmarBasem
force-pushed
the
camera-extra-features
branch
from
db19bdd
to
9de1852
Compare
85% of end-end tests have passedNot executed tests (1)Failed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (33)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@OmarBasem Thank you for PR. Take a look a found issues: ISSUE 1: Mirror effect after taking a picture with the Front CameraSteps to Reproduce:
Actual Result:After taking a picture with the front camera, the image appears mirrored, causing a mirror effect. The objects in the image may appear reversed How the photo is pointed How the image is shown after photo is taken Expected Result:Pictures taken with the front camera should not have a mirror effect. The images should appear as they do in reality |
|
unfortunately there is no exact steps how to reproduce. This error just appears randomly for android device ISSUE 2: The 'java.lang.illegaStateException' error occurs after the photo is taken on AndroidActual result:java.lang.illegaStateException occurs after the photo is taken Expected result:The photo is taken without error Device:real device: pixel 7a, Android 13 Logs: |
|
Hi @VolodLytvynenko, thanks for your testing. Are you sure there shouldn't be a mirror effect? Both iPhone and Android native cameras have a mirror effect |
| rotate (reanimated/use-shared-value "0deg")] | ||
| (orientation/use-device-orientation-change (fn [result] | ||
| (reset! current-orientation result) | ||
| (case result |
There was a problem hiding this comment.
@OmarBasem, case should be used only with literals. Soon I'll open a PR where the linter disallows this dangerous practice (which has caused bugs in the project's history). https://github.com/clj-kondo/clj-kondo/blob/master/doc/linters.md#case-symbol-test-constant
For this piece of code, cond seems to be the best alternative.
There was a problem hiding this comment.
Thanks for the feedback @ilmotta
| :flash-mode (if @flash :on :off) | ||
| :camera-type @camera-type}]) | ||
| (when-not @uri | ||
| [rn/view {:style (style/zoom-container top insets)} |
There was a problem hiding this comment.
This component should ideally be broken down into smaller ones. This can help Reagent return cached results for certain nodes in the tree, whereas a big component will tend to be entirely recomputed and re-rendered by Reagent.
For instance, this part here is the only one that depends on current-zoom. If you extract all these zoom-buttons into a separate component and move the current-zoom state there, then if current-zoom is mutated, the more expensive camera-screen component won't need to re-render.
(defn zoom-buttons
[]
(let [current-zoom (reagent/atom "1")]
(fn [top insets rotate]
[rn/view {:style (style/zoom-container top insets)}
[zoom-button {:value "0.5" :current-zoom current-zoom :rotate rotate}]
[zoom-button {:value "1" :current-zoom current-zoom :rotate rotate}]
[zoom-button {:value "2" :current-zoom current-zoom :rotate rotate}]
[zoom-button {:value "3" :current-zoom current-zoom :rotate rotate}]])))
;; In the camera-screen component
(when-not @uri
[zoom-buttons top insets rotate])@ulisesmac, please correct me if I'm wrong.
There was a problem hiding this comment.
Yep, your suggestion is correct @ilmotta
Also having smaller components with a meaningful name helps to readability 👍
There was a problem hiding this comment.
Thanks @ilmotta. Broke it down a little bit.
| [zoom-button {:value "3" :current-zoom current-zoom :rotate rotate}]]) | ||
| [rn/view {:style (style/confirmation-container insets @uri)} | ||
| [quo/text | ||
| {:on-press #(retake flash uri) |
There was a problem hiding this comment.
Completely optional feedback, but raising awareness nonetheless
You can move the on-press anonymous function to the let binding at the top, where flash and uri are defined. This way, we avoid unnecessary function allocations. Not the case here, but in other cases this practice can help Reagent use cached results because the function instance would be stable across re-renders.
The same refactoring to local binding can be applied for other anonymous functions defined in camera-screen, such as #(reset! camera-ref %) or #(reset! flash (not @flash)).
The Reagent doc says we don't necessarily need to worry about this for the most part, but I still think it's a good practice for React based apps, especially in this restricted and slow (perf-wise) mobile world.
| [rn/view {:style (style/confirmation-container insets @uri)} | ||
| [quo/text | ||
| {:on-press #(retake flash uri) | ||
| :style {:font-size 17 |
There was a problem hiding this comment.
This font-size 17 is weirdly placed like this. As far as I've seen we use typography values from the foundations namespace. Could you expand a bit on this point?
There was a problem hiding this comment.
That's right @ilmotta, please check this comment: #16569 (comment)
There was a problem hiding this comment.
Thanks @OmarBasem. So it seems worth having another issue to track this problem, wdyt?
| :color colors/white}} | ||
| (i18n/label :t/use-photo)]] | ||
| [rn/view {:style (style/bottom-area top insets @uri)} | ||
| [quo/text {:style style/photo-text} (i18n/label :t/PHOTO)] |
There was a problem hiding this comment.
I know I mentioned in another one of your PRs @OmarBasem, but I think it's worth reiterating, also because the fix is simple. Translation keywords in our codebase should be lowercased, for the simple reason that it's the idiomatic way in Clojure. If we already have a translation with the name :t/photo, simply give it a different name like :t/photo-header.
It's obviously not something introduced by this PR, so no worries if you don't want to change the code.
I'm considering adding a custom script to make lint to verify this, since it's easy and cheap.
| (i18n/label :t/retake)] | ||
| (let [camera-ref (atom nil) | ||
| uri (reagent/atom nil) | ||
| current-zoom (reagent/atom "1") |
There was a problem hiding this comment.
Why is the current-zoom value a string? It seems f-zoom-button can handle the number just fine. Seems more correct type-wise.
| camera-type (reagent/atom camera-kit/camera-type-back) | ||
| flash (reagent/atom false) | ||
| current-orientation (atom orientation/portrait)] | ||
| [:f> |
There was a problem hiding this comment.
I believe this pattern of a form-2 component with a functional component using an anonymous function has been deprecated in the repo. It often causes rendering issues, so it's better to extract the functional component using the f-* pattern.
There was a problem hiding this comment.
That's right, my bad
| [rn/view {:style style/screen-container} | ||
| [reanimated/touchable-opacity | ||
| {:active-opacity 1 | ||
| :on-press #(reset! flash (not @flash)) |
There was a problem hiding this comment.
Light suggestion: This is commonly done as (swap! flash not)
Hi @OmarBasem , I apologize for this issue confusion. When I initially investigated it, I checked how it works in Telegram and Viber and they do not use a mirror effect. However, now I see that both iPhone and Android native cameras do use a mirror effect, as well as Discord. I have been mistaken due to the initial information from Telegram and Viber :) |
|
@OmarBasem, I just encountered issue 2 (#16781 (comment)) on the latest nightly build. I will create a separate issue for it. If this PR does not require any new commits, it can be merged. Otherwise, let me know if it needs additional testing after new commits. For now, no additional issues from my side. Thank you! |
OmarBasem
force-pushed
the
camera-extra-features
branch
from
9de1852
to
1a96e94
Compare
|
Thanks for your testing @VolodLytvynenko. I made some changes, you can test it again please. |
85% of end-end tests have passedNot executed tests (1)Failed tests (6)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Passed tests (33)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
|
|
Hi @OmarBasem A few additional minor issues are found. Could you fix them into the next PR related to camera extra features? |
ISSUE 3. Flash and camera switcher icon rotation not correct in landscape mode after flash being tappedSteps to reproduce:
Actual Result:After tapping on the flash icon, the flash icon does not rotate correctly Expected Result:When tapping on the flash icon in landscape mode |
Question / ISSUE:It appears that the flash for Android devices' front camera is not supported. For Pixel 7a the flash is not applied when a photo is taken. Considering that many Android devices do not support front camera flash, it might be better to hide the flash icon when the front camera is enabled for Android devices. Steps:
Actual Result:The flash does not work when taking a photo with the front camera on Android devices. Expected Result:Potential solution:
|
|
@Francesca-G please, review this PR. |
Hi @OmarBasem take a look on the design feedback and let us know when PR can be retested. Thank you! |
|
@Francesca-G can the details left in https://www.figma.com/file/Tf5nfkYvpbnNCo4rKLK7lS/Feedback-for-Mobile?type=design&node-id=3719%3A64506&mode=design&t=ODmz1dy1mNFmwJn3-1 be fixed in the separate follow up or better to fix it in the scope of this PR? |
Separate follow ups work :) thanks |
|
@OmarBasem PR is ready to be merged! Thank you |
# This is the 1st commit message: feat: camera updates updates updates feat: camera feat: camera updates icons icons icons icons revert # This is the commit message #2: lint # This is the commit message #3: review # This is the commit message #4: revert # This is the commit message #5: lint # This is the commit message #6: review # This is the commit message #7: camera features # This is the commit message #8: feat: camera extra features
OmarBasem
force-pushed
the
camera-extra-features
branch
from
1a96e94
to
baa6727
Compare
|
Thanks @Francesca-G and @VolodLytvynenko ! |
fixes: #16580
This PR implements camera features: flash, front-camera, landscape.