Page MenuHomePhabricator

ImagePanel should show loading animation for first image
Open, Needs TriagePublic

Description

When the PagelistWidget dialogue window is opened for the second or third time, there is no loading animation for the first image.

Reproduction steps

  • Open an Index: page ( ex: Index:War and Peace.djvu)
  • Navigate to edit mode
  • Click on "Preview pagelist"
  • Click on any of the generated page numbered buttons to open a popup
  • Inside the popup navigate to a different page

Documentation

Criteria

  • Loading animation should be shown even for the first image everytime.

Event Timeline

Soda moved this task from Backlog to Pagelist Widget on the ProofreadPage board.
Sandyabhi subscribed.

@Soda Can you share the current status on this task?

Change #1198640 had a related patch set uploaded (by Siddharth_Singh_dev; author: Siddharth_Singh_dev):

[mediawiki/extensions/ProofreadPage@master] T278617 — Ensure pending spinner shows every time in Pagelist ImagePanel

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

Change #1198663 had a related patch set uploaded (by Reeti; author: Reeti):

[mediawiki/extensions/ProofreadPage@master] PagelistWidget: Show loading animation for first image on every open

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

Change #1198640 abandoned by Siddharth_Singh_dev:

[mediawiki/extensions/ProofreadPage@master] T278617 — Ensure pending spinner shows every time in Pagelist ImagePanel

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

Change #1198680 had a related patch set uploaded (by Reeti; author: Reeti):

[mediawiki/extensions/ProofreadPage@master] Show loading animation for first image in PagelistWidget dialog

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

Change #1198680 abandoned by Aklapper:

[mediawiki/extensions/ProofreadPage@master] Show loading animation for first image in PagelistWidget dialog

Reason:

See https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ProofreadPage/+/1198663 by same author

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

Hi,
It seems Siddharth Singh may no longer be working on this task. Could you please confirm if it’s okay for me to take over and continue the work? If there is no objection, I can assign the task to myself and proceed.
Thank you!

It seems Siddharth Singh may no longer be working on this task.

@Reeti: What makes you think so?

I saw the patch was marked abandoned and there’s been no recent updates. Just wanted to confirm before proceeding thanks!

I might be doing something wrong, but I can't seem to replicate this bug. If I click a pagelist number to open the dialog, it shows the loading animation. Then I close the dialog and click a different number (because clicking the same one doesn't work at that point, it's already selected), then the previous page's image shows for a moment but is surrounded by the loading animation.

Is the bug that the previous page shouldn't be shown at that point?

@Samwilson I think the reproduction steps is to close the window and then reopen it and see if the loading animation shows up. (I'm also a bit rusty on what the bug is but I remember that it involved closing the window)

That's what I thought, but it looks like there is a loading animation, but it's perhaps only visible when you're zoomed out far enough for there to be space around the image. But the fact that there is an image there at all when you first open the dialog seems wrong (well, unless it's the right image! but this seems to be that it's showing the previously-selected page's image).

@Reeti Have you been able to replicate the buggy behaviour?

@Samwilson Thanks for catching that! I'll fix the previous image flash by clearing the image source on dialog close and resetting the loading state. Will update you on this soon.

@Samwilson I've pushed a fix in Gerrit change 1207022 that resolves the image flashing issue. Can you check?

Thanks for working on that @Reeti, but I think we first need to define more what this bug actually is. Are you able to reproduce the loading animation bug? If that's not the bug, then we should update the title and description with what the actual problem is.

Try the following (with network throttling enabled, to make it clearer what's happening):

  1. Click on a page number to open the dialog.
  2. Animation is shown, and the image loads. Zoom out now so that the outer area is visible.
  3. Close the dialog (the zoom level will be remembered).
  4. Click on a different page number to open the dialog again.
  5. See the animation around the image (the old image).

I've tried various combinations of switching pages within the dialog, and visual mode or not, but it doesn't seem to change. I did manage to get it into a state where any subsequent opening of the dialog would always end up on the same page regardless of which number I clicked, but unfortunately I can't replicate that.

Thanks @Samwilson for the detailed steps. I can reproduce the loading animation appearing around the previous image after reopening on a different page (especially with throttling). I’ll update the task title/description to reflect this behavior if that’s the core bug, and ensure the change here aligns with that scope.

Change #1228444 had a related patch set uploaded (by Reeti; author: Reeti):

[mediawiki/extensions/ProofreadPage@master] Fix duplicate image loading and remove redundant show() call Bug: T278617 Change-Id: I6832a1cb93eedacd06d77d8bb7d73937ec51e0c1

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

Change #1228438 had a related patch set uploaded (by Reeti; author: Reeti):

[mediawiki/extensions/ProofreadPage@master] Fix duplicate image loading and remove redundant show() call

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

Change #1228444 abandoned by Reeti:

[mediawiki/extensions/ProofreadPage@master] Fix duplicate image loading and remove redundant show() call Bug: T278617 Change-Id: I6832a1cb93eedacd06d77d8bb7d73937ec51e0c1

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

I can reproduce the loading animation appearing around the previous image after reopening on a different page (especially with throttling). I’ll update the task title/description to reflect this behavior if that’s the core bug

Sorry, no, I meant that the animation there isn't a bug. The bug is that the previous image is shown when no image should be.

Thanks @Samwilson for the review.
I have updated the patch please review.

Change #1228438 abandoned by Aklapper:

[mediawiki/extensions/ProofreadPage@master] Fix duplicate image loading and remove redundant show() call

Reason:

See https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ProofreadPage/+/1198663 instead

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

@Reeti: Please do not post the same comment in Phabricator and in Gerrit, because folks get twice the notifications. Thanks!

[Removing inactive task assignee]