Mobile gallery block - Upload options #1610

mkevins · 2019-11-22T08:09:37Z

This aims to track issues related to uploading media from the Gallery block. It is part of this issue: #1416.

Test Builds

Builds are open for testing on these draft PRs:

WordPress-Android: Enable gallery upload options in production WordPress-Android#11234 (comment)
WordPress-iOS: Enable gallery upload options in production WordPress-iOS#13374 (comment)

Media uploading and ids

In the current state, the MediaPlaceholder component utilizes the addToGallery prop described here: WordPress/gutenberg#18262 to remove duplicates via the id prop whenever new media is selected. This is because the collection of image elements is currently keyed on id or url if id is not defined.

When a media item is local (i.e. not finished uploading, in the case of adding media from device), it may have a temporary local id. This can result in "collisions" (i.e. the local id of a newly added and currently uploading media item being equal to the "server id" of an already uploaded item within the same gallery).

One way to address this is to negate the local media id over the bridge (both directions), this way, the media models / database on WordPress-Android will not require updating, and the Gutenberg (JavaScript) side will not see collisions. I'd love to know if there are alternative approaches to solving this as well.

Update: The collision id issue has been confirmed with steps here.

The text was updated successfully, but these errors were encountered:

mchowning · 2019-11-22T14:52:13Z

Recently investigated a call that was failing due to the whole using-the-local-id behavior you're discussing here. Basically we are making a call to request media info for an image before the image id has been updated from the local id, which fails (although, like you note, it could succeed if the id happened to match an image already on web). Turns out this particular error appears to not have any significant user impact because as soon as we do get the new (proper) id from the web, we retry the call with that id. wordpress-mobile/WordPress-Android#10779 (comment)

mkevins · 2019-11-26T09:05:42Z

Thanks @mchowning for confirming that local <-> remote id collisions are indeed possible. Also, thanks for providing context, and linking to your thorough description of your findings. It's very helpful!

In the case for gallery, the colliding ids may affect user experience, since we have some deduping logic based on id. As an example scenario, imagine the following:

Current state of attributes.images:

[
  { id: 1, url: 'cat' }, // remote id
  { id: 2, url: 'dog' }, // remote id
  { id: 3, url: 'bird' }, // remote id
]

User adds some local images:

[
  { id: 1, url: 'monkey' }, // local id (collision)
  { id: 4, url: 'snake' }, // local id
  { id: 5, url: 'tiger' }, // local id
]

After deduping, we'd have gallery images state as:

[
  { id: 1, url: 'cat' }, // remote id
  { id: 2, url: 'dog' }, // remote id
  { id: 3, url: 'bird' }, // remote id
  { id: 4, url: 'snake' }, // local id
  { id: 5, url: 'tiger' }, // local id
]

So, the 🐒 monkey will not appear in the gallery, blocked by the 🐈 cat.

One solution is to modify the deduping logic, while another would be to modify the way we generate local-ids to prevent the possibility of collisions.

mkevins · 2020-02-06T08:00:49Z

pinarol · 2020-02-06T08:16:40Z

@mkevins it'd be great if you also add these items here and steps here.

pinarol · 2020-02-06T08:26:24Z

Choose from the free photo library
Gallery block should allow uploading images from the free photo library.

Note: ❌ On iOS, I observed that only the first image is added to the gallery block, with the rest appended as image blocks.

@koke Could you help detecting the root cause of this one?

koke · 2020-02-06T10:31:15Z

@koke Could you help detecting the root cause of this one?

It took me a while to get it running, but there's no mystery, it's just what the code is explicitly doing:

// Append the first item via callback given by Gutenberg.
if let firstItem = assets.first {
    insertOnBlock(with: firstItem)
}
// Append the rest of images via `.appendMedia` event.
// Ideally we would send all picked images via the given callback, but that seems to not be possible yet.
appendOnNewBlocks(assets: assets.dropFirst())

iamthomasbishop · 2020-02-06T20:38:40Z

@mkevins Had a chance to review and jotted down some notes/feedback. I also saw some of what has already been pointed out, but some other minor things.

Both

On the block settings sheet, “Images Size” row label should be “Image Size"
Padding on caption is too small. I believe the padding on the caption should be 8px

Android

“Link To” and “Image Size” bottom sheets shouldn’t have “Cancel” button
I don’t think this is specific to the Gallery block (see Video block), but the block settings bottom sheet feels a little tight at the bottom of the sheet. Can we add ~16px under the last list item?
Performance seems to suffer when there are ~15+ gallery images (only seeing this on Android)

iOS

As others have noted, on iOS, when you add multiple images from Free Photo Library, it breaks the images up into separate image blocks — haven't seen this happening on Android

chipsnyder · 2020-02-07T21:46:02Z

Note:On Android, I encountered a red screen after confirming the captured photo.
✔️ Fixed via: 09a7d22

@mkevins Just validated this issue I'm running gutenberg-mobile at 09a7d22bceda261ac8f7cd1ff018f818564c4ee0 and I'm still seeing an error with this flow. The video is below; this happened for me 4/4 times.

chipsnyder · 2020-02-07T22:26:21Z

PR is opened for the gallery upload iOS issue

wordpress-mobile/WordPress-iOS#13404

chipsnyder · 2020-02-10T13:07:58Z

#1610 (comment)

This was an error in my environment. Double checked with https://36287-9306568-gh.circle-artifacts.com/0/Artifacts/WordPress-pr-11234-build-36287.apk and it's working now. ✅

chipsnyder · 2020-02-10T14:00:15Z

It's looking good @mkevins!

Tested on: https://36287-9306568-gh.circle-artifacts.com/0/Artifacts/WordPress-pr-11234-build-36287.apk

So far these cases are passing for me:
✅ Choose from device (stay in editor) - Successful upload
✅ Take a photo
✅ Choose from the free photo library

And I had one error:
❌ Choose from device (stay in editor) - Failed upload

Retry All seems to do nothing

Create a Gallery
Successfully add images to the gallery
Add an image from the device

While the image is uploading

Turn off wifi
Validate the "Failed to insert media. Please tap for options"
Turn wifi back on

Validate wifi is back (in the video I added another image)

Tap the failed image
Tap Retry All

Result: The image still displays the "Failed to insert media. Please tap for options"
Expected: Image should add the overlay, indicating it's attempting to upload.

Video:

mkevins · 2020-02-11T10:39:49Z

Thanks @iamthomasbishop for testing this out and providing great feedback!

These remaining issues are not related specifically to this PR, so I'll open new issues for them to be tackled in another PR(s).

Both

On the block settings sheet, “Images Size” row label should be “Image Size"

Padding on caption is too small. I believe the padding on the caption should be 8px

Android

“Link To” and “Image Size” bottom sheets shouldn’t have “Cancel” button

I don’t think this is specific to the Gallery block (see Video block), but the block settings bottom sheet feels a little tight at the bottom of the sheet. Can we add ~16px under the last list item?

Performance seems to suffer when there are ~15+ gallery images (only seeing this on Android)

iamthomasbishop · 2020-02-11T17:38:08Z

These remaining issues are not related specifically to this PR, so I'll open new issues for them to be tackled in another PR(s).

Sounds good! 👍 Thanks for wrangling that!

mkevins self-assigned this Nov 22, 2019

pinarol mentioned this issue Nov 22, 2019

Gallery block - semi cross platform approach #1416

Closed

3 tasks

mkevins mentioned this issue Nov 22, 2019

[RNMobile] Gallery - GalleryImage component WordPress/gutenberg#18155

Merged

5 tasks

mkevins mentioned this issue Nov 27, 2019

[RNMobile] Gallery - Tiles component WordPress/gutenberg#18130

Merged

5 tasks

pinarol mentioned this issue Nov 27, 2019

Gallery - Handle post update if uploads are completed when the editor is closed #1629

Closed

mkevins mentioned this issue Dec 12, 2019

Mobile Gallery Block - Known Issues #1674

Open

14 tasks

This was referenced Jan 22, 2020

Fix local media id collisions #1800

Closed

Fix local media id collisions wordpress-mobile/WordPress-Android#11125

Closed

This was referenced Feb 5, 2020

Enable gallery upload options in production #1855

Merged

Local media ids can collide with remote ids wordpress-mobile/WordPress-Android#11239

Open

pinarol mentioned this issue Feb 6, 2020

Free photo library: Allow uploading multiple images into Gallery #1863

Closed

mkevins closed this as completed in #1855 Feb 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mobile gallery block - Upload options #1610

Mobile gallery block - Upload options #1610

mkevins commented Nov 22, 2019 •

edited

Loading

mchowning commented Nov 22, 2019

mkevins commented Nov 26, 2019

mkevins commented Feb 6, 2020 •

edited by geriux

Loading

pinarol commented Feb 6, 2020

pinarol commented Feb 6, 2020

koke commented Feb 6, 2020

iamthomasbishop commented Feb 6, 2020

chipsnyder commented Feb 7, 2020

chipsnyder commented Feb 7, 2020

chipsnyder commented Feb 10, 2020

chipsnyder commented Feb 10, 2020 •

edited

Loading

mkevins commented Feb 11, 2020

iamthomasbishop commented Feb 11, 2020

Mobile gallery block - Upload options #1610

Mobile gallery block - Upload options #1610

Comments

mkevins commented Nov 22, 2019 • edited Loading

Test Builds

Media uploading and ids

mchowning commented Nov 22, 2019

mkevins commented Nov 26, 2019

mkevins commented Feb 6, 2020 • edited by geriux Loading

Test Builds

Uploading Test Cases

WordPress-Android

WordPress-iOS

pinarol commented Feb 6, 2020

pinarol commented Feb 6, 2020

koke commented Feb 6, 2020

iamthomasbishop commented Feb 6, 2020

chipsnyder commented Feb 7, 2020

chipsnyder commented Feb 7, 2020

chipsnyder commented Feb 10, 2020

chipsnyder commented Feb 10, 2020 • edited Loading

mkevins commented Feb 11, 2020

iamthomasbishop commented Feb 11, 2020

mkevins commented Nov 22, 2019 •

edited

Loading

mkevins commented Feb 6, 2020 •

edited by geriux

Loading

chipsnyder commented Feb 10, 2020 •

edited

Loading