openfoodfacts / smooth-app

🤳🥫 The new Open Food Facts mobile application for Android and iOS, crafted with Flutter and Dart
https://world.openfoodfacts.org/open-food-facts-mobile-app?utm_source=off&utf_medium=web&utm_campaign=github-repo
Apache License 2.0
845 stars 281 forks source link

Privacy review of server-side cropping #3656

Closed teolemon closed 1 year ago

teolemon commented 1 year ago

What

monsieurtanuki commented 1 year ago

@teolemon Indeed. We should also tell them how to have us remove the already uploaded privacy-questionable pictures.

stephanegigandet commented 1 year ago

Ideally, it should be a choice that can be easily changed for each picture (e.g. sometimes I want to remove my thumb, the background of my kitchen etc. but other times I'm taking the full picture of the back of the product and cropping to get the ingredients). The easiest would be a switch just above the "take another", "confirm" buttons, that says "Also send the uncropped picture"

stephanegigandet commented 1 year ago

While we are at it, can we rename "Confirm" to "Send"? It's really unclear what it does when we "Confirm" (e.g. are we just confirming the crop and I can do other things before sending)

stephanegigandet commented 1 year ago

Also the choice to send the uncropped picture should not be the default. On all other photo sharing apps, pictures are cropped locally.

stephanegigandet commented 1 year ago

And while I'm doing my shopping list: could we remove the row of buttons (profile / search / history) when we are on the crop page? That would really make taking pictures easier. I keep hitting that history button instead of "confirm"

stephanegigandet commented 1 year ago

Also the choice to send the uncropped picture should not be the default. On all other photo sharing apps, pictures are cropped locally.

Bonus point if we remember that setting for the next photos.

monsieurtanuki commented 1 year ago

While we are at it, can we rename "Confirm" to "Send"? It's really unclear what it does when we "Confirm" (e.g. are we just confirming the crop and I can do other things before sending)

My 2 cents: not convinced "Send" is more explicit than "Confirm" or "OK". From a user perspective, I just confirm that it's the correct crop of the correct image, I probably don't care what the hell is going on with the server (if I know there is one). "Upload" would be more explicit - and more frightening: it will be on the server, I have to be careful with what I upload.

Ideally, it should be a choice that can be easily changed for each picture

I guess a dialog with a checkbox when we click on Confirm/Send would do the trick. With a default value to "send only cropped data". And a "don't ask again" checkbox. Capture d’écran 2023-01-30 à 10 57 41

Bonus point if we remember that setting for the next photos.

Saved in the preferences.

could we remove the row of buttons (profile / search / history)

cf. #3337

stephanegigandet commented 1 year ago

Well I would avoid the dialog as it adds one more screen and click when adding pictures. The less clicks, the better.

I'm never going to click that "Don't ask again" question because for the same product, I can take 3 photos and want to keep only the crop for 1 of them.

The best would be a switch (not a checkbox) just above the Retake / Confirm buttons.

stephanegigandet commented 1 year ago

My 2 cents: not convinced "Send" is more explicit than "Confirm" or "OK". From a user perspective, I just confirm that it's the correct crop of the correct image, I probably don't care what the hell is going on with the server (if I know there is one). "Upload" would be more explicit - and more frightening: it will be on the server, I have to be careful with what I upload.

Well we do want users to be careful, and not send bad photos. I find "Confirm" confusing because the screen the user sees does not say what it does. Its title is "Recycling photo". Maybe if we change the title to "Add a recycling photo" or "Change the recycling photo", then "Confirm" is ok. Otherwise "Send" (to Open Food Facts) makes sense. "Upload" is more ambiguous: it could be a private upload.

monsieurtanuki commented 1 year ago

Well I would avoid the dialog as it adds one more screen and click when adding pictures. The less clicks, the better. I'm never going to click that "Don't ask again" question because for the same product, I can take 3 photos and want to keep only the crop for 1 of them. The best would be a switch (not a checkbox) just above the Retake / Confirm buttons.

Maybe I'm wrong, but

monsieurtanuki commented 1 year ago

Well we do want users to be careful, and not send bad photos. I find "Confirm" confusing because the screen the user sees does not say what it does. Its title is "Recycling photo". Maybe if we change the title to "Add a recycling photo" or "Change the recycling photo", then "Confirm" is ok. Otherwise "Send" (to Open Food Facts) makes sense. "Upload" is more ambiguous: it could be a private upload.

Fair enough about the "private upload". "Send" is less ambiguous.

M123-dev commented 1 year ago

Honestly, I am not a big fan of adding it somewhere directly for the users. We are not a program built for professionals. If we scare them at the beginning with too many warnings and things to know there is a chance to uninstall before finishing the onboarding.

My suggestion:

Image addition: Local crop Photo edit: Serverside crop

+ We can make the crop a optional thing, so that not everyone is forced to crop a picture even when the picture is so good that there is no need for it.

stephanegigandet commented 1 year ago

Image addition: Local crop Photo edit: Serverside crop

That sounds great to me.

monsieurtanuki commented 1 year ago

Image addition: Local crop Photo edit: Serverside crop

That would be a good compromise. Then, we would still have to optimize the performances for local crop, but that's another story.

stephanegigandet commented 1 year ago

Then, we would still have to optimize the performances for local crop, but that's another story.

@monsieurtanuki just curious: the on-device cropping (not the UI for cropping, but the transformation of a JPG to a cropped JPG when we have the coordinates) is causing performance issues?

teolemon commented 1 year ago

20 seconds on @alexgarel 's device

monsieurtanuki commented 1 year ago

There are possible fixes for those performances issues:

  1. fix the algorithm itself (e.g. I know that in a project of mine it's not the same speed to save a file as JPG then crop it as it is to save it as raw then crop it) (or something like that)
  2. use an isolate
  3. move the file cropping part inside the background task instead of cropping the file then calling a background task => that's my favorite!
M123-dev commented 1 year ago

Okay, but now after #3673 we again have the problem that for initial image addition we always have this crop time, which takes long on weaker phones.

For that I suggest that we don't force users to crop, but rather show them the image, with the option to open a crop page, rotate or directly send. Here a beautiful MS Paint visualization edit_not_crop_flow

monsieurtanuki commented 1 year ago

Okay, but now after #3673 we again have the problem that for initial image addition we always have this crop time, which takes long on weaker phones.

That's not correct: I haven't changed anything there. We always have a crop time for the "transient" version, based on fresh or older pictures, before and after #3673. That is a crop computed on a smaller image (smaller than the user's screen), which is faster to compute.

Are you currently experiencing a slow app? Could you then share logs / picture size / crop parameters? If so, I may add isolate (still slow but no stuttering). I may also have a closer look at the slower steps of image cropping.

If after that you still experience a slow app, I'll have a look at your MSPaint sketches, but the UX looks OK now (besides the performances).

monsieurtanuki commented 1 year ago

I've just run some tests, on a 3000x2002 picture and an ios emulator whose screen max size in 736. It took 126ms to "crop" the whole picture with 270°, into a 500x736 transient picture. Which is acceptable for a button click action. It took 490ms to "crop" the whole picture with 270°, into a 1984x2920 to-be-uploaded picture, as a background task.

Obviously I won't be able to run performance tests on my emulator. Would that be an android-specific issue?

@M123-dev I suggest you put print('${LocalDatabase.nowInMillis()}: $message') statements in RotatedCropController.getCroppedBitmap in order to see which operations take long. Yes, I know, I only use the most cutting-edge performance tools :)

monsieurtanuki commented 1 year ago

I should be able to test performances of just the crop operation on my canary smartphone tomorrow.

M123-dev commented 1 year ago

I think @alexgarel had some Performance problems