feat: 3129 Allow to swipe between product images on the full screen image view #3325
Conversation
…arChend1kar/smooth-app into swipeable_product_image_view
…into swipeable_product_image_view
omkarChend1kar
changed the title
There was a problem hiding this comment.
Hi @omkarChend1kar!
I don't have much time to review tonight but there's something I believe is slightly flawed in your code:
- when you click on an existing image, you display the swipe page
- when you click on a non existing image, you go to the "create a new picture" page
- so far so good
- but on the swipe page, you seem to display all images, regardless of them existing or not
- that means crap pages I'm afraid, like in your video you didn't dare to got to "packaging"
- I believe that you should display only displayable images, and not empty pages for empty images
I understood that just reading your code; maybe there's something I'm missing.
| @@ -42,7 +42,9 @@ class _ProductImageViewerState extends State<ProductImageViewer> { | |||
| @override | |||
| void initState() { | |||
| imageData = widget.imageData; | |||
| imageProvider = NetworkImage(imageData.imageUrl!); | |||
| imageProvider = NetworkImage( | |||
| imageData.imageUrl ?? '', | |||
There was a problem hiding this comment.
That's not possible: if there's nothing to display you shouldn't be there with a non-existing image.
There was a problem hiding this comment.
Ohh ,Right I should have first check what is behaviour when images are not present, That was presumptuous of me to consider images will always be present.
There was a problem hiding this comment.
@omkarChend1kar Thinking again about it, it's not that simple either, as the imageUrl can be refreshed from outside (perhaps not yet, but that's part of #3018 that I'm implementing).
That would mean that if on the gallery you click on an existing image, you go to a swipe page with all images, existing or non existing. On a non existing image, you have to display something specific like the "no picture" logo, with a "new image" button: that's what you should change from your PR code.
And if on the gallery you click directly on a non existing image, you go directly to the "new image" feature (like now).
There was a problem hiding this comment.
@monsieurtanuki Actually when imageUrl is without proper path or let say empty string, As I have declared there, It shows that No image icon, That's why I was putting empty string there instead of passing null, Since it was throwing nulll exception, After what you said about this, I thought of filtering list with non null Image paths and then show only uploaded images of whichever category would be displayed,
But I guess this is what you have just mentioned above right,To show the icon/ text for non-existing images.
There was a problem hiding this comment.
I don't agree: this is not NetworkImage's job to display crap.
That's our job to display something relevant when we do know there's no image there.
And in the previous screen does display something when there's no image - we have a specific "no image" logo. At least for consistency, we should display the same logo.
And, I think I already mentioned that earlier, this in NOT the same behavior when you click on "edit": either you modify an existing image, or you create a new image. In your case you always try to modify, and that will fail with no source image.
There was a problem hiding this comment.
Although, I do have one query, After I put our specified logo image at the place of non-existing image source, It doesn't make to display edit button, Instead of that what if we put Add button there and do whatever we do in previous image gallery place such that it will user to add image there itself, Still not sure how going to implement it but just that for non-existing image add buttons looks better.
There was a problem hiding this comment.
@omkarChend1kar That's correct - for the "no image" cases we need:
- a specific "no image" logo, instead of the image
- a specific "add" button label, instead of "edit"
- a specific "add" action, instead of "edit"
There was a problem hiding this comment.
Hi @omkarChend1kar!
Still not convinced; please have a look at my comments.
Beyond that, as a user I'm a bit confused if
- from page A you see all the images
- if you click on an image in page A you go to page B
- on page B you can swipe towards all page A's images
I know, that's precisely the purpose of the issue/PR: @teolemon just checking, could you confirm that the behavior matches our needs without adding confusion.
For the record, currently we display only the 4 major images, but by the end of the year we should display all the "other" images (tons of them).
Not related to the code:
- could you please specify in the PR comments that it
Closes #3129. - it's not a
chore(or then everything is a chore) (but that's another story), it's afeat
| @@ -42,7 +42,9 @@ class _ProductImageViewerState extends State<ProductImageViewer> { | |||
| @override | |||
| void initState() { | |||
| imageData = widget.imageData; | |||
| imageProvider = NetworkImage(imageData.imageUrl!); | |||
| imageProvider = NetworkImage( | |||
| imageData.imageUrl ?? '', | |||
There was a problem hiding this comment.
I don't agree: this is not NetworkImage's job to display crap.
That's our job to display something relevant when we do know there's no image there.
And in the previous screen does display something when there's no image - we have a specific "no image" logo. At least for consistency, we should display the same logo.
And, I think I already mentioned that earlier, this in NOT the same behavior when you click on "edit": either you modify an existing image, or you create a new image. In your case you always try to modify, and that will fail with no source image.
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| WidgetsBinding.instance.addPostFrameCallback((_) { |
There was a problem hiding this comment.
Not sure but perhaps would be better in initState.
There was a problem hiding this comment.
Yeah, Since I didn't use stateful widget, There was no initstate, And only appBar text that needed to be rebuilt, That's why went with Valuenotifier only
There was a problem hiding this comment.
Will convert to stateful, That would be more readable.
There was a problem hiding this comment.
Will convert to stateful, That would be more readable.
Good idea. And you can get rid of ValueNotifier<int>.
Noted, Will mention it closes #3129 |
|
teolemon
added
image carousel
@teolemon Then perhaps we would be better off going directly from the product page to the new swipe page and ignore the pool (especially as for the moment it does not display the "other" images). Nothing blocking, though. |
Codecov Report
@@ Coverage Diff @@
## develop #3325 +/- ##
===========================================
- Coverage 10.49% 10.45% -0.04%
===========================================
Files 254 255 +1
Lines 12362 12404 +42
===========================================
Hits 1297 1297
- Misses 11065 11107 +42
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
the pool is absolutely not in my plan for this. You should just see it as a source for a potential image selector in that case. |
@teolemon Let me rephrase:
My previous comment was about the redundancy between page "A" and its 4 images, and page "B" with a single image but instant access to the other 3. I named page "A" the "pool" because that's as close as we are from a pool in Smoothie, and it did provide access to the "other" images (actually it did not because the other images cannot be accessed sync, and I removed them temporarily some weeks ago). So, when @omkarChend1kar finishes the current PR, we'll have redundant access to the 4 images. |
|
ok, sorry for the misunderstanding. |
@monsieurtanuki @teolemon Okay, Then are we going with the approach of removing page A and directly going to Page B ? |
@omkarChend1kar Let's say it's a possible enhancement for a different issue and PR. For the moment, let's implement a swipe gesture that works correctly with existing and non existing images. If ever we decide to remove page A, all the work on page B will already be done. |
…into swipeable_product_image_view
2. Displaying default No-image for Non-existing url 3. Displaying add button for Default No-image 4. Enabled action on Add button to add new image
…arChend1kar/smooth-app into swipeable_product_image_view
@monsieurtanuki I am not able decide what would be more important from user's perspective, Whether to see all the images with just a swipe or be able to upload the images with few steps ,It depends on user to user and why exactly he/she is visiting the Products page. |
I think the idea is to make the contributions quicker. |
|
My view on this:
(Havn't had a look at the code) |
It does collide because on the same page you would have the crop tool working and the swipe effect - when you swipe, do you swipe to another image or do you move the crop area?
Minor rewriting is necessary because @omkarChend1kar started before my code was merged, but it would work. |
The crop area shouldn't be swipable, just the image view. When clicking on the edit button you get to a not swipable page. Just like in the normal phone gallery |
Except that what is suggested in #3332 is a unique landing page when you click on an image in the product page. |
What if we manage to, Enable crop and click new image functionalities with click of edit icon or button just like now ,On current page itself, Without navigating to another screen ,So with click of Edit button swiping will be disabled and If we put a cross/close icon on right corner,It will disable all controls to crop/click new image and enable swiping back. Though, Haven't considered how we are going to implement it ,But would it serve the purpose, If we ever manage to do it. |
That would be in contradiction with #3332, wouldn't it?
The problem is not "opening a new page". The problem is "having to click on a button" for that.
We don't "put a cross/close icon on right corner", that's the flutter standard for pages with Navigator.push<String>(
context,
MaterialPageRoute<String>(
builder: (BuildContext context) => CropPage(File(inputPath)),
fullscreenDialog: true, // here
),
);
Understood. So basically you discard #3332 and replace the existing code with something you don't know how to implement (I don't either). For only 4 images, remember. Anyway, it's a matter of UX choice (then code maintenance), and we could go on arguing which is the best (?) solution. |
…arChend1kar/smooth-app into swipeable_product_image_view
…arChend1kar/smooth-app into swipeable_product_image_view
There was a problem hiding this comment.
Hi @omkarChend1kar!
Unfortunate timing: we were both working on the same files, I wrote significant changes and merged first.
You've solved the conflicts by removing my changes, which in that particular case was really not appropriate.
From #3329 and #3339 we use a different way to get picture providers. That allows us to be more flexible if the image is in an "in-between" status: e.g. the user changed the image but the image is not uploaded yet and we want to display that image locally, before the upload, before the product refresh.
Please solve the conflicts accordingly. If needed, you can create a new PR from scratch - could be faster and easier.
Sorry about that.
…mage view (Conflicts after openfoodfacts#3332)
…mage view (Formatting Changes)
| @@ -1,17 +1,19 @@ | |||
| import 'dart:io'; | |||
| late Map<ProductImageData, ImageProvider?> _selectedImages; | ||
|
|
||
| bool _isRefreshed = false; |
| ImageProvider? _provideImage(ProductImageData imageData) => | ||
| TransientFile.getImageProvider(imageData, _barcode); | ||
|
|
||
| imageData.imageUrl == null ? null : NetworkImage(imageData.imageUrl!); |
| @@ -80,7 +79,7 @@ class _ProductImageGalleryViewState extends State<ProductImageGalleryView> { | |||
| ) | |||
| : null, | |||
| leading: SmoothBackButton( | |||
| onPressed: () => Navigator.maybePop(context), | |||
| onPressed: () => Navigator.maybePop(context, _isRefreshed), | |||
| ), | ||
| ); | ||
| Future<void> _newImage(ProductImageData data) async { |
| void dispose() { | ||
| _localDatabase.upToDate.loseInterest(_barcode); | ||
| super.dispose(); | ||
| _isImageUrlAvailable = widget.imageData.imageUrl != null; |
|
|
||
| String get _barcode => _initialProduct.barcode!; | ||
| /// When the image is edited, this is the new image |
Seems like making new branch from |
|
@omkarChend1kar Indeed, a refreshed |
@monsieurtanuki Will I have to close this PR, Before creating new PR for same issue. |
|
@omkarChend1kar I've just converted the current PR to draft, to avoid confusion. We can even close it now (and we will close it anyway). |
Ohkay!! |
|
Closed in favor of #3363. |
What
Screenshot/Mockup after :
Swipeable.Product.Images.mp4
Part of