fix(StackViewport): weird interaction between scroll() and setImageIdIndex()

[jobunk]

Context

I had encountered an issue in the StackViewport class, where the user would scroll, afterwards use the public method "setImageIdIndex()" to jump to a specific image and then continue scrolling. The expected behavior in this situation would be to that the viewport continues scrolling from the image that the user just jumped to. However the actual behavior was that the scrolling would pick up back where the user had last scrolled and completely ignore the "setImageIdIndex()" that had happened.

Changes & Results

This happens because "this.targetImageIdIndex", which is used during scrolling, was never updated. My workaround was to just use the "setStack()" method instead, since it actually updates "this.targetImageIdIndex" correctly. However I believe that that is not the intended use of the method, so here is my suggested fix. It adds an optional boolean that tells the viewport to also move the scrolling position to the given index. This boolean is optional, because the method is also called from within scroll(), in which case we don't want to adjust the targetImageIdIndex.

Testing

I only tested it locally in the context of my project. The idea is to scroll to an image, then jump to another image far away using "setImageIdIndex(, true)" and then continue scrolling. If the scrolling picks up from the position that you jumped to, the error has been fixed. I also didn't see any weird behavior with other functions, which makes sense, since it's an optional input parameter that was added.

Checklist

PR

• My Pull Request title is descriptive, accurate and follows the
  semantic-release format and guidelines.

Code

• My code has been well-documented (function documentation, inline comments,
  etc.)

• I have run the yarn build:update-api to update the API documentation, and have
  committed the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)

Public Documentation Updates

• The documentation page has been updated as necessary for any public API
  additions or removals.

Sorry I'm not sure if this is needed here.

Tested Environment

• "OS: Windows
• "Node version: v20.12.2
• "Browser: Firefox

[netlify]

✅ Deploy Preview for cornerstone-3d-docs ready!

Name	Link
🔨 Latest commit	d6139f2
🔍 Latest deploy log	https://app.netlify.com/sites/cornerstone-3d-docs/deploys/661d63c9c29df2000882811f
😎 Deploy Preview	https://deploy-preview-1208--cornerstone-3d-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[jobunk]

@sedghi Could you review this? Sorry that I can't provide more for the testing part. If something is unclear I'm happy to provide further explanation.

[sedghi]

If the scrolling picks up from the position that you jumped to, the error has been fixed.

I don't think that is the correct behavior. The scroll should not depend on the state (where it was jumped) and should only scroll in the requested direction.

It seems like you are injecting your app-level logic into the library, which is usually not a good idea.

I recommend implementing that logic in your app:

• Detect the jump and keep track of it.
• When you want to scroll, check the state of the app to see where it jumped.
• Then set ImageIdIndex before you scroll from that location and then scroll.

Does this make sense?

[jobunk]

If the scrolling picks up from the position that you jumped to, the error has been fixed.

I don't think that is the correct behavior. The scroll should not depend on the state (where it was jumped) and should only scroll in the requested direction.

It seems like you are injecting your app-level logic into the library, which is usually not a good idea.

I recommend implementing that logic in your app:

* Detect the jump and keep track of it.

* When you want to scroll, check the state of the app to see where it jumped.

* Then set `ImageIdIndex` before you scroll from that location and then scroll.

Does this make sense?

Thank you for you response. What you describe in your implementation recommendation makes sense and is basically the behavior I expected from this module. However when I adjust ImageIdIndex by using the setImageIdIndex() method, the scrolling will not be aware of this since it uses the targetImagingIdIndex variable, which is never adjusted. It will basically jump back to the pre setImageIdIndex() position the next time it's called and then start scrolling. This issue is what I tried to solve in this PR.

Please let me know if I misunderstood anything in your recommendation. In my opinion my solution is not very pretty, but it is the best I could come up with given that the setImageIdIndex() and scroll() functions are kind of intertwined.

[sedghi]

@jobunk can you please provide a minimum example to show case the issue?