Fix : Android - Picture looks smaller in Preview page #7660

ahmdshrif · 2022-02-09T19:19:01Z

Details

Fixed Issues

Tests

Verify that no errors appear in the JS console

QA Steps

1- Launch the app
2- Log in with any account
3- Tap on any user
4- Tap on plus button and add attachment
5- Tap on Select from Gallery
6- Attach any picture from Gallery
7- Tap Send
8- Tap on attachment again

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

AndrewGable · 2022-02-09T20:24:58Z

Can we test on all platforms? Thanks!

ahmdshrif · 2022-02-09T20:31:44Z

I think the issue was reported on android only and I do fix it for native.
I catch this issue on the web also. do I need to search for a new fix since the web has another way to handle image preview ? or can I report it as a new issue?

rushatgabhane · 2022-02-09T20:34:42Z

@ahmdshrif let's try to fix it for web too

ahmdshrif · 2022-02-09T20:34:59Z

On the web, you can just click and this will do the zoom we expect

AndrewGable · 2022-02-09T20:35:15Z

If the bug doesn't happen on main, then this PR broke it. All issues need to be fixed across all platforms.

rushatgabhane · 2022-02-09T20:36:41Z

the web and desktop bug is on main too

AndrewGable · 2022-02-09T20:37:39Z

Makes sense, then let's fix across all platforms 👍

ahmdshrif · 2022-02-09T20:39:16Z

ok, working on a web and desktop fix.

ahmdshrif · 2022-02-09T20:47:07Z

I add zoom to be true as the default value on the web.
and it does what we expect.

Screen.Recording.2022-02-09.at.10.45.18.PM.mov

rushatgabhane · 2022-02-09T20:51:51Z

As long as it doesn't cause a regression, that's fine.

rushatgabhane

@ahmdshrif all images on web are zoomed in by default. Let's fix that

ahmdshrif · 2022-02-09T21:42:56Z

@ahmdshrif all images on web are zoomed in by default. Let's fix that

@rushatgabhane I think that is what we expect. the small image does not fill the screen so we zoom it by default to take the screen width.

or what do you mean

rushatgabhane · 2022-02-09T21:46:42Z

@ahmdshrif an image that isn't small is zoomed by default.

Screencast.from.10-02-22.12-44-29.AM.+03.1.mp4

ahmdshrif · 2022-02-09T21:48:23Z

ok get it will fix that , thanks

ahmdshrif · 2022-02-09T21:53:01Z

I get this error when trying to run on the desktop is this my local issue or is this issue on the main.

[Renderer] [HPM] Error occurred while trying to proxy request /api?command=Log from localhost:8080 to http://[::1]:9000 (ECONNREFUSED) (https://nodejs.org/api/errors.html#errors_common_system_errors)
[Renderer] [HPM] Error occurred while trying to proxy request /api?command=Log from localhost:8080 to http://[::1]:9000 (ECONNREFUSED) (https://nodejs.org/api/errors.html#errors_common_system_errors)

rushatgabhane · 2022-02-09T21:56:39Z

@ahmdshrif desktop is working fine for me.

ahmdshrif · 2022-02-09T23:22:01Z

it shoud be fix now @rushatgabhane
the web zooming

rushatgabhane · 2022-02-10T14:24:28Z

src/components/ImageView/index.js

@@ -22,7 +22,7 @@ class ImageView extends PureComponent {
        this.state = {
            containerHeight: 0,
            containerWidth: 0,
-            isZoomed: false,
+            isZoomed: true,


@ahmdshrif can we go with an alternative approach because it feels like a workaround, and it's buggy. Let's keep isZoomed be false by default.

It's still buggy for normal images. What do you think of using imgTop, imgLeft, etc instead.

screencast-from-10-02-22-05-12-43-pm-03_GIc5JsAV.mp4

it must not get zooming if it's small. Let me try to test with different image sizes. on my side.
I think imgTop, imgLeft, is more related to position when you move the image. but let me check this.

@rushatgabhane can you share with me the image that have the issue because for me I try many image sizes and still work fine with me

Screen.Recording.2022-02-11.at.1.49.35.AM.mov

Here you go

Cute puppy 😍

@rushatgabhane ok thanks, I fix t it now

Screen.Recording.2022-02-11.at.2.09.24.AM.mov

Cute puppy 😍

ikrr 😍

@rushatgabhane ok thanks, I fix t it now

Fantastic!

rushatgabhane

@ahmdshrif two points for large images.

The image is zoomed in only for the first time it's opened. (subsequent opens aren't zoomed but they have the flicker problem below)
The image flickers from zoomed in to zoomed out state which is undesireable.

Also, small images are zoomed in by default, so we can't zoom them in further.

Let me know what you think

Screencast.from.11-02-22.03-27-34.AM.+03.mp4

ahmdshrif · 2022-02-11T00:55:58Z

ok, thanks for your feedback I will take some time thinking about a more stable way for the web.

ahmdshrif · 2022-02-13T14:38:49Z

@rushatgabhane it's now stable and we can zoom the small image also.

preview :

Screen.Recording.2022-02-13.at.4.32.44.PM.mov

rushatgabhane

@ahmdshrif sorry, but requesting a really minor change.

Could you please add JSdoc for all the functions, thanks!

Co-authored-by: Andrew Gable <andrewgable@gmail.com>

src/components/ImageView/index.js

rushatgabhane

@AndrewGable all yours! LGTM

OSBotify · 2022-03-03T03:14:47Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-03-11T01:00:44Z

🚀 Deployed to staging by @AndrewGable in version: 1.1.42-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

kbecciv · 2022-03-14T02:57:59Z

@ahmdshrif @AndrewGable @rushatgabhane PR is failing on 1.1.42.3 build

Screen_Recording_20220311-083833_New.Expensify.mp4

ahmdshrif · 2022-03-14T09:51:50Z

@kbecciv which platform it fails on it?
and can you please share the image that fails?

kbecciv · 2022-03-14T16:29:33Z

@ahmdshrif It fails on Andriod app.

ahmdshrif · 2022-03-15T19:38:01Z

ook, we can repro this issue with very small images only. #7591 (comment)
and this is one of the other edge cases we can't repro during the test. and need to be handled also.

rushatgabhane · 2022-03-15T20:28:08Z

@ahmdshrif wait what, I don't understand why this particular image isn't working while these are - #7591 (comment).

The image width and height is correct too

imageWidth = Math.round(this.props.windowWidth);
imageHeight = Math.round(imageWidth * aspectRatio);

ahmdshrif · 2022-03-15T20:34:59Z

I am still debugging this. I just mention we do not repro this because this edge case wasn't mentioned.

for now, I just note this image has a large size, and android was crashed with this image. and we have a workaround to avoid this crash.

// Resize the image to max dimensions possible on the Native platforms to prevent crashes on Android. To keep the same behavior, apply to IOS as well.
            const maxDimensionsScale = 11;
            imageHeight = Math.min(imageHeight, (this.props.windowHeight * maxDimensionsScale));

rushatgabhane · 2022-03-15T20:51:12Z

Cool.
Note: large images like this one - #7660 (comment) work

OSBotify · 2022-03-15T20:53:27Z

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

ahmdshrif · 2022-03-15T20:56:33Z

@rushatgabhane yes this case is so weird

ahmdshrif · 2022-03-17T22:13:44Z

the update I have for now is this something related to android and how he resize the image
because the same code works fine on ios.

android gets stuck with an image with this size(and aspect ratio) only but still works fine with other sizes.

I think the solution will be to change most of the code on the calculateImageSize to make sure it's not going to this edge case for android.
is this something we should do here or can move it to a new issue? @AndrewGable @rushatgabhane

Christinadobrzyn · 2022-03-24T04:14:03Z

@AndrewGable and @rushatgabhane - sorry to also ping you but we're at the 7 day payment timeframe. Is this new issue a regression that needs to be fixed before paying/closing this job?

rushatgabhane · 2022-03-24T04:17:41Z

@Christinadobrzyn this issue isn't completely fixed, I've asked @ahmdshrif to fix it here - #7591 (comment)

We'll create a follow up PR, after which the payment should be done. I hope this answers your question

ahmdshrif marked this pull request as ready for review February 9, 2022 20:10

ahmdshrif requested a review from a team as a code owner February 9, 2022 20:10

botify requested review from AndrewGable and rushatgabhane February 9, 2022 20:10

MelvinBot removed the request for review from a team February 9, 2022 20:10

rushatgabhane suggested changes Feb 9, 2022

View reviewed changes

ahmdshrif requested a review from rushatgabhane February 10, 2022 00:30

rushatgabhane reviewed Feb 10, 2022

View reviewed changes

ahmdshrif requested a review from rushatgabhane February 11, 2022 00:14

rushatgabhane suggested changes Feb 11, 2022

View reviewed changes

ahmdshrif requested a review from rushatgabhane February 13, 2022 14:36

resize imgaes to screen width

9f1a309

rushatgabhane suggested changes Mar 2, 2022

View reviewed changes

ahmdshrif and others added 2 commits March 2, 2022 10:53

Update src/components/ImageView/index.js

581997a

Co-authored-by: Andrew Gable <andrewgable@gmail.com>

add js doc

37875cd

ahmdshrif requested a review from rushatgabhane March 2, 2022 09:20

delet empty lines

f3d5951

AndrewGable reviewed Mar 2, 2022

View reviewed changes

src/components/ImageView/index.js Outdated Show resolved Hide resolved

rushatgabhane suggested changes Mar 2, 2022

View reviewed changes

src/components/ImageView/index.js Outdated Show resolved Hide resolved

src/components/ImageView/index.js Outdated Show resolved Hide resolved

src/components/ImageView/index.js Outdated Show resolved Hide resolved

src/components/ImageView/index.js Outdated Show resolved Hide resolved

apply pr suggestions

d874894

ahmdshrif requested a review from rushatgabhane March 2, 2022 19:57

rushatgabhane approved these changes Mar 2, 2022

View reviewed changes

AndrewGable approved these changes Mar 3, 2022

View reviewed changes

AndrewGable merged commit ea691f0 into Expensify:main Mar 3, 2022

rushatgabhane mentioned this pull request Mar 10, 2022

Enable no multiple empty lines rule Expensify/eslint-config-expensify#45

Merged

OSBotify mentioned this pull request Mar 10, 2022

Deploy Checklist: New Expensify 2022-03-10 #8074

Closed

botify mentioned this pull request Mar 15, 2022

[HOLD for payment 2022-04-13] [HOLD for payment 2022-03-22] Android - Picture looks smaller in Preview page #7591

Closed

ahmdshrif mentioned this pull request Apr 1, 2022

fix android resize #8285

Merged

53 tasks

Fix : Android - Picture looks smaller in Preview page #7660

Fix : Android - Picture looks smaller in Preview page #7660

Conversation

ahmdshrif commented Feb 9, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

AndrewGable commented Feb 9, 2022

ahmdshrif commented Feb 9, 2022

rushatgabhane commented Feb 9, 2022

ahmdshrif commented Feb 9, 2022

AndrewGable commented Feb 9, 2022

rushatgabhane commented Feb 9, 2022 • edited Loading

AndrewGable commented Feb 9, 2022

ahmdshrif commented Feb 9, 2022

ahmdshrif commented Feb 9, 2022

rushatgabhane commented Feb 9, 2022

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

ahmdshrif commented Feb 9, 2022

rushatgabhane commented Feb 9, 2022 • edited Loading

ahmdshrif commented Feb 9, 2022

ahmdshrif commented Feb 9, 2022

rushatgabhane commented Feb 9, 2022

ahmdshrif commented Feb 9, 2022 • edited Loading

rushatgabhane Feb 10, 2022

Choose a reason for hiding this comment

ahmdshrif Feb 10, 2022

Choose a reason for hiding this comment

ahmdshrif Feb 10, 2022

Choose a reason for hiding this comment

rushatgabhane Feb 11, 2022

Choose a reason for hiding this comment

AndrewGable Feb 11, 2022

Choose a reason for hiding this comment

ahmdshrif Feb 11, 2022

Choose a reason for hiding this comment

rushatgabhane Feb 11, 2022

Choose a reason for hiding this comment

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

ahmdshrif commented Feb 11, 2022

ahmdshrif commented Feb 13, 2022

rushatgabhane left a comment

Choose a reason for hiding this comment

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

OSBotify commented Mar 3, 2022

OSBotify commented Mar 11, 2022

kbecciv commented Mar 14, 2022

ahmdshrif commented Mar 14, 2022 • edited Loading

kbecciv commented Mar 14, 2022

ahmdshrif commented Mar 15, 2022

rushatgabhane commented Mar 15, 2022

ahmdshrif commented Mar 15, 2022 • edited Loading

rushatgabhane commented Mar 15, 2022 • edited Loading

OSBotify commented Mar 15, 2022

ahmdshrif commented Mar 15, 2022

ahmdshrif commented Mar 17, 2022 • edited Loading

Christinadobrzyn commented Mar 24, 2022

rushatgabhane commented Mar 24, 2022 • edited Loading

ahmdshrif commented Feb 9, 2022 •

edited

Loading

rushatgabhane commented Feb 9, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane commented Feb 9, 2022 •

edited

Loading

ahmdshrif commented Feb 9, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane left a comment •

edited

Loading

ahmdshrif commented Mar 14, 2022 •

edited

Loading

ahmdshrif commented Mar 15, 2022 •

edited

Loading

rushatgabhane commented Mar 15, 2022 •

edited

Loading

ahmdshrif commented Mar 17, 2022 •

edited

Loading

rushatgabhane commented Mar 24, 2022 •

edited

Loading