Page MenuHomePhabricator

Make ImageGateway request a batch of image data for ImageCarousel's use
Closed, DeclinedPublic8 Estimated Story Points

Description

As part of T216198, ImageCarousel.js was refactored to support advancing its own images when one of the navigation controls is clicked (previously OverlayManager instantiated a new instance of ImageCarousel with each navigation control click). Each time a user clicks one of the navigation controls (arrows) on the carousel, ImageGateway makes a request for the image data (e.g. url to image, license info, etc).

However, this introduced a new problem: If multiple ImageGateway promises happened to resolve while a user is on a slide, multiple images would be inserted into the overlay. A quick fix involving a $el closure variable was made to account for this, but it warrants a better solution.

T219410 proposed making these promises cancelable. However, @Jdrewniak suggested that a better solution might be to instead have ImageGateway make one request for a batch of data of all the page's images. e.g.:

var thumbs = mediaWiki.mobileFrontend.getCurrentPage()._thumbs.map( i => i.options.filename );

new mw.Api().get({
	action: 'query', 
	format: 'json', 
	formatversion: 2, 
	prop: 'imageinfo',
	titles: thumbs,
	iiprop: ['url','extmetadata'],
iiextmetadatafilter: ['LicenseShortName', 'Artist'],
	iiurlwidth: 1920,
	iiurlheight: 1920
})

This could be either be done by ImageCarousel when it instantiates or by injecting this data into ImageCarousel. By doing this, the promise count would be limited to one so the chance of multiple promises resolving on the same slide would be eliminated.

See Jan's research on this at https://docs.google.com/document/d/1so4URfU5z9utBA5tuafIyGSEbGriJJFF6stYHpNgtCk/edit?usp=sharing for more background/info.

Acceptance criteria

  • ImageGateway requests a batch of image data. ImageCarousel/ImageGateway no longer make a request for the data of one image on each slide
  • After the promise is resolved, each navigation click will start to load the relevant image immediately - no spinner is shown.
  • The loadFailMessage is refactored to work with the new batch data request. It currently shows when either the promise rejects or the image fails to load.
  • The quick fix $el closure variable inside ImageCarousel.js postRender method is removed (let's go back to using (this,$el/self.$el)

Event Timeline

nray renamed this task from Make ImageCarousel load a batch image data to Make ImageGateway load a batch of data for all ImageCarousel images.Apr 25 2019, 12:11 AM
nray renamed this task from Make ImageGateway load a batch of data for all ImageCarousel images to Make ImageGateway request a batch of image data for use by ImageCarousel.
nray renamed this task from Make ImageGateway request a batch of image data for use by ImageCarousel to Make ImageGateway request a batch of image data for ImageCarousel's use.

@Jdrewniak Please feel free to add anything to this if you have additional info/insights

Jdlrobson moved this task from Incoming to Upcoming on the Web-Team-Backlog board.

Are there any complications expected for request continuation? For example, what happens if we have too many images to fit in one response.

We had some discussion on what should happen when the promise for a batch of image fails. @nray, may have some additional notes.

Maybe we can isolate the batch request responsibility:

  • Allow configuration of batch size
  • Cache successful and unsuccessful responses
  • Expose a function to get responses. If they're available, they're returned immediately. If they're unavailable, they're requested. If they're available, they're returned immediately. The function returns a Promise.
  • Isolate fetcher from the batch part, if possible. The API may not make this practical.
  • Should be unit testable
ovasileva set the point value for this task to 8.Apr 30 2019, 4:52 PM

It may be possible to shrink pointage as subtasks are extracted.

I'm moving this back to needs analysis because @Jdrewniak found an article with 2,429 images that this approach might not handle so well or its implementation might get more complex than its worth.

I'm thinking that we should explore other options now and feel inclined to close this ticket unless others feel that we should try this.

Closing this ticket for reasons above. If anyone feels strongly that we should pursue this route feel free to reopen