Improve performance of the gallery block, by removing unnecessary requests.

[jorgefilipecosta]

Description

This PR adds a check in GalleryImage block to verify if we already have the URL if yes the media is not requested. Until now the media was requested anyway even if the value was not used. So we were requesting each image of the gallery without a need.

This PR had improvements to the gallery frame because I did not notice PR #2488. I prefer the changes in that PR so I remove frame changes from this PR and I will try to rebase the existing PR.

Fixes: #4032

How has this been tested?

Add a gallery to post, save the post, reload the post verify the images are not requested.
See the conversion from shortcodes still works, for example by pasting [gallery ids="10868,10875,10872"].

[joemcgill]

@jorgefilipecosta – Initially, I really liked the approach you were taking with the changes to the gallery frame. Would you mind either opening a new PR with those changes or adding them back here?

[jorgefilipecosta]

Hi @joemcgill, thank you for having a look at this PR, your PR #2488 was more ambitious, as it changed the frame of the gallery to the edit mode which I think we should do.
The biggest difference from the original state of this pr to the version we had in #2488 was the function to load the specific attachment id's, but in fact I think @joemcgill version is less hacky compared to what I had 😄
So I rebased PR #2488, and I added a new commit with more improvements. I think we can use that one for the changes to the gallery media frame.
If there is any change I made in your PR that you don't agree with just let me know and I will correct the situation.

[aduth]

Won't this cause issues on subsequent re-renders of the component where image is assigned, but url is as well? In other words, do we risk an infinite loop of setAttributes -> componentWillReceiveProps -> setAttributes?

[jorgefilipecosta]

Hi @aduth, I don't think so, because of this condition in the with select image: id && ! url ? getMedia( id ) : null,, it guarantees that as soon as we have an URL the image will become null so the setAttributes function will not be called again.

[aduth]

So what is the image prop representing as proposed here? It's very unclear to me. Would it be accurate to say that it's "a nullable value only set when the ID is known, the URL is not known, and a REST API request is in progress, then proceeding to be unset immediately once the response is received and the URL becomes known"? If so, how can we... make that more clear.

[jorgefilipecosta]

Hi @aduth, thank you for the review, you are right, it is not easy to understand this logic. I did some changes in order to try to make the logic easier.

[mcsf]

I've pushed a suggestion improving the comments, @jorgefilipecosta.

I have one issue with this, though, due to the component's eagerness to satisfy attributesToSet:

1. Add a Classic block with content [gallery ids="1,2,3"] where the IDs correspond to proper images.
2. Convert to blocks. As expected, a Gallery block loads with said images.
3. Press Undo to revert to the Classic block. Observed: The Gallery block remains in place. Expected: to recover the original Classic block. Keep pressing Undo: same result.

This is because attributes keep getting re-set once we undo. The following patch helps visualize:

diff --git a/core-blocks/gallery/gallery-image.js b/core-blocks/gallery/gallery-image.js
index f38d2ad23..854925896 100644
--- a/core-blocks/gallery/gallery-image.js
+++ b/core-blocks/gallery/gallery-image.js
@@ -80,6 +80,10 @@ class GalleryImage extends Component {
 			this.props.setAttributes( attributesToSet );
 		}
 
+		if ( ! attributesToSet && this.props.attributesToSet ) {
+			console.log( 'dropped attributesToSet', this.props.attributesToSet );
+		}
+
 		// unselect the caption so when the user selects other image and comeback
 		// the caption is not immediately selected
 		if ( this.state.captionSelected && ! isSelected && this.props.isSelected ) {

[jorgefilipecosta]

Hi @mcsf,
Thank you for the review!

Press Undo to revert to the Classic block. Observed: The Gallery block remains in place. Expected: to recover the original Classic block. Keep pressing Undo: same result.

Unfortunately, it looks like this was an already existing problem without an easy fix. To solve it I refactored the code to handle the requests directly in the transform similar to what we do with the image upload.
Doing an undo creates an empty gallery block and another undo clears it. I think although not perfect this undo behavior is better than what we had and is something acceptable and consistent with what happens in other actions e.g: drag&drop upload.

[mcsf]

I'm concerned that we're making substantial concessions and making the fix more complex than it should when maybe the root problem lies elsewhere.

Right now

To the point: in order to avoid getting stuck in an "undo loop" (described here), we are manually setting up a subscription to all data changes until all our images are loaded, then committing all of their newfound data via a single updateBlockAttributes action dispatch, for which we've also altered the transforms API.

Instead, what if we look at this from the angle of content history and Undo?

Why we should look elsewhere

Prior to these changes, each indivual GalleryImage would wait (using withSelect) for its corresponding image object to resolve, after which it would update the Gallery block via setAttributes( { images: …, [ { url, alt } ] } ). However, ignoring the technical challenges and thinking from a user's perspective, there was no meaningful change of attributes:

• From the Gallery block's instantiation (upon conversion from shortcode), from a user's standpoint, the attributes already are that the gallery comprises a certain set of images. The fact that Gutenberg needs to resolve them is a technical detail.
• Once they are resolved, this doesn't correspond to any user-facing change. Sure, the UI needs the images resolved in order to enable certain editing features, but the user has made no change to the block.

Counter-proposal

This is but a lengthy way to conclude that GalleryImage instances should be able to signal resolution to the Gallery block and let the block update itself in a way that doesn't create a new Undo level.

It's a potentially dumb idea that would also require an interface change — at its simplest, it could mean a hidden option in setAttributes( newAttrs, { specialOption } ) — so please shoot it down if you think it's doomed. setAttributes is just an interface to dispatch UPDATE_BLOCK_ATTRIBUTES, and we have a precedent for undo history batching:

gutenberg/editor/store/reducer.js

Lines 151 to 170 in a5d6a53

	/**
	* Returns true if, given the currently dispatching action and the previously
	* dispatched action, the two actions are modifying the same property such that
	* undo history should be batched.
	*
	* @param {Object} action Currently dispatching action.
	* @param {Object} previousAction Previously dispatched action.
	*
	* @return {boolean} Whether to overwrite present state.
	*/
	export function shouldOverwriteState( action, previousAction ) {
	if ( ! previousAction || action.type !== previousAction.type ) {
	return false;
	}
	return overSome( [
	isUpdatingSameBlockAttribute,
	isUpdatingSamePostProperty,
	] )( action, previousAction );
	}

With this in mind, there's two ways forward: a pretty naïve one and a more robust one:

1. The first approach is to simply have a { silent: true } option that would materialize, perhaps, as a meta field in the action payload ({ type: 'UPDATE_BLOCK_ATTRIBUTES', uid, attributes, meta: { silent: true } }) which shouldOverwriteState would recognize. Simple, and it would work if resolving images was guaranteed to be quick and fail-proof, but actually prone to weird history scenarios if a user interacts with the editor between the Gallery block being inserted and the image data being ready.

2. What this really is about is multi-action narratives ("sagas"? ). When image data is obtained, what we want is to complement a previous block update. This is what I mean:
   a) Some action is dispatched: a new Gallery block is inserted. This creates a new undo level. Suppose this action has an identifier: type: '…', actionId: 123.
   b) However, we know our state tree to be in a temporary state, since our Gallery block only has incomplete image information.
   c) Other actions may be dispatched, most of them creating new undo levels.
   d) At some point, we have image data and want to dispatch an action to update the Gallery block's attributes. However, we signal the action with: type: …, complementsAction: 123. Then, the state will be updated, but the past will be rewritten so that the history snapshot created by our initial action with ID 123 gets updated by merging in the last action's state diff.

This all requires messing with the editor reducer, but in my mind can be done relatively atomically. Comments welcome.

[catehstn]

@jorgefilipecosta do you have a decision on @mcsf 's suggestions above? Is this PR worth keeping or should we close it?

[mcsf]

@catehstn: This is still basically blocked by #8119, which has been explored in a couple of ways but so far never with a satisfying solution. Also see #11504 (comment). I'd like to explore another solution and am trying to slot in some dev time for that.

[jorgefilipecosta]

Thank you for checking this PR @catehstn. @mcsf summarized the status well. Although this problem is not the same as the undo problems, the way the undo problems are solved will affect the changes required here so it is blocked until the other issue is solved. I added the Status Blocked tag (I should have done it sooner).

[jorgefilipecosta]

The original issue #4032 was addressed in an alternative way so this PR is not relevant anymore.