Gutenberg-mobile: Mobile gallery block - Upload options

[mkevins]

This aims to track issues related to uploading media from the Gallery block. It is part of this issue: https://github.com/wordpress-mobile/gutenberg-mobile/issues/1416.

Test Builds

Builds are open for testing on these draft PRs:

• WordPress-Android: https://github.com/wordpress-mobile/WordPress-Android/pull/11234#issuecomment-582252774
  
  
  
  • WordPress-iOS: https://github.com/wordpress-mobile/WordPress-iOS/pull/13374#issuecomment-582251935
  
  

Media uploading and ids

In the current state, the MediaPlaceholder component utilizes the addToGallery prop described here: https://github.com/WordPress/gutenberg/pull/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.

[mchowning]

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. https://github.com/wordpress-mobile/WordPress-Android/pull/10779#issuecomment-555759072

[mkevins]

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: monkey will not appear in the gallery, blocked by the :cat2: 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]

Test Builds

Builds are open for testing on these draft PRs:

• WordPress-Android: https://github.com/wordpress-mobile/WordPress-Android/pull/11234#issuecomment-582252774
  
  
  
  • WordPress-iOS: https://github.com/wordpress-mobile/WordPress-iOS/pull/13374#issuecomment-582251935
  
  

Uploading Test Cases

WordPress-Android

• [x] Gallery block - Choose from device (stay in editor) - Successful upload - steps
• [x] Gallery block - Choose from device (stay in editor) - Failed upload - steps
• [x] Gallery block - Choose from device (leave the editor) - Successful upload - steps
• [x] Gallery block - Choose from device (close and re-open the editor with ongoing uploads) - steps
• [x] Gallery block - Take a photo - steps
• [x] Gallery block - Choose from the free photo library - steps

WordPress-iOS

• [x] Gallery block - Choose from device (stay in editor) - Successful upload - steps
• [x] Gallery block - Choose from device (stay in editor) - Failed upload - steps
• [x] Gallery block - Choose from device (leave the editor) - Successful upload - steps
• [x] Gallery block - Choose from device (close and re-open the editor with ongoing uploads) - steps
• [x] Gallery block - Take a photo - steps
• [x] Gallery block - Choose from the free photo library - steps

[pinarol]

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

[pinarol]

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]

@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]

@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]

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]

PR is opened for the gallery upload iOS issue

https://github.com/wordpress-mobile/WordPress-iOS/pull/13404

[chipsnyder]

https://github.com/wordpress-mobile/gutenberg-mobile/issues/1610#issuecomment-583628763

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]

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

1. Create a Gallery
2. Successfully add images to the gallery
3. Add an image from the device
4. While the image is uploading
5. Turn off wifi
6. Validate the "Failed to insert media. Please tap for options"
7. Turn wifi back on
8. Validate wifi is back (in the video I added another image)
9. Tap the failed image
10. 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]

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]

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!