Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Viewer timeline when data sources change #12284

Merged
merged 2 commits into from
Nov 1, 2024

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Nov 1, 2024

Description

In #12202 we moved most of the tracking logic into the CesiumWidget class. However this skipped the update for the timeline to zoom it to the correct time period when data sources are added, like in this sandcastle

There may be a cleaner solution here but I think this closely matches the previous behavior. Longer term I would've expected the timeline to reflect the values of the clock closer including the start/end times like the Animation widget does.

(We also noticed the model disappears but that's a separate issue #12282)

Issue number and link

No issue

Testing plan

  • Open the CZML Model - Node Transformation sandcastle
  • Make sure the timeline is zoomed to the correct time frame for the model's availability
  • Make sure the timeline head is still looping the full timeline correcly.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

github-actions bot commented Nov 1, 2024

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

@jjspace
Copy link
Contributor Author

jjspace commented Nov 1, 2024

I opened #12285 to discuss changing the Timeline itself which I think would've avoided this issue and may allow us to remove the linkTimelineToDataSourceClock function in its entirety but that doesn't need to hold up this PR right before the release

Copy link
Contributor

@lukemckinstry lukemckinstry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, looks like a good fix 👍

@lukemckinstry lukemckinstry merged commit 377da51 into main Nov 1, 2024
9 checks passed
@lukemckinstry lukemckinstry deleted the fix-viewer-timeline branch November 1, 2024 19:27
@jjspace
Copy link
Contributor Author

jjspace commented Nov 1, 2024

CC @jfayot No action needed but thought you might be interested in this follow up. Something I missed while reviewing your PR. Odd little side effect of the weird way these classes interact with each other and the order event handlers are triggered. If you do think of a better solution than this please let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants