Page MenuHomePhabricator

Make ImageCarousel discard old promises
Closed, DeclinedPublic5 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).

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 (and not ideal) attempt was made to correct this by creating a closure variable that referenced the current instance's $el element and using this reference when the promise resolves or rejects to perform DOM manipulations. One of the reasons this is bad is because when the promise resolves, it also sets instance variables so we could potentially be overwriting instance variables with incorrect info if multiple promises resolve while on the same slide.

We should probably fix this by making a cancelable promise and canceling the promise with each onSlide in a way that the logic inside the resolve/reject handlers doesn't get a chance to run.

Acceptance criteria

  • Only one imageGateway.getThumb resolve handler or only one imageGateway.getThumb reject handler should execute when a user is on any given slide. i.e. if user quickly advances slides and then stops, only one image should appear and not multiple (which could happen if multiple resolve handlers executed)
  • The $el closure variable inside the postRender method is removed (let's go back to using (this,$el/self.$el)

Developer Notes

Working patch at https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/498998/ . Feel free to modify or discard

Event Timeline

nray created this task.Mar 27 2019, 4:01 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 27 2019, 4:01 PM
nray renamed this task from Example title to Make ImageCarousel gracefully handle multiple promises.Mar 27 2019, 4:02 PM
nray updated the task description. (Show Details)Mar 27 2019, 6:00 PM
nray updated the task description. (Show Details)
nray updated the task description. (Show Details)Mar 27 2019, 6:03 PM
nray updated the task description. (Show Details)
nray updated the task description. (Show Details)Mar 27 2019, 6:05 PM
nray updated the task description. (Show Details)Mar 27 2019, 6:10 PM
nray renamed this task from Make ImageCarousel gracefully handle multiple promises to Make ImageCarousel discard old promises.Mar 27 2019, 6:14 PM

Change 498998 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Make ImageCarousel discard old promises

https://gerrit.wikimedia.org/r/498998

phuedx triaged this task as High priority.Mar 28 2019, 12:26 PM
phuedx added a subscriber: phuedx.

High because the debt is in an area that we're actively working on and it's both a UX problem and a resource consumption problem.

phuedx moved this task from Needs refactor to Cleanup on the Technical-Debt board.
Jdlrobson set the point value for this task to 5.Apr 2 2019, 4:52 PM
nray closed this task as Declined.Apr 25 2019, 12:15 AM

I'm closing this ticket for now in favor of exploring T221813 instead

Change 498998 abandoned by Nray:
Make ImageCarousel discard old promises

Reason:
will explore T221813 first

https://gerrit.wikimedia.org/r/498998