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

feat: timeslider scroll event updates bar position #3336

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

KartikWatts
Copy link
Contributor

@KartikWatts KartikWatts commented Feb 24, 2024

closes #2931

Description:

This pull request enhances the functionality of Widget_Timeslider by implementing scroll event updates. It achieves this by introducing repositioning of the timeline bar within the on_scroll_event method and updating the existing zoom functionality. Additionally, it enables smooth scrolling through the panel, adjusting based on the central position of the TimeModel. These updates significantly improve the user experience and enhance the overall usability of the Widget_Timeslider.

Changes:

  • Implemented scroll event updates for Widget_Timeslider.
  • Updated the zooming functionality hotkey as: ctrl + scroll.
  • Added repositioning of the timeline bar within the on_scroll_event method.
  • Enabled smooth panel scrolling, dynamically adjusting based on the central position of the TimeModel.

@yoSachinkr
Copy link

@KartikWatts I've just tested this PR. It's working perfectly. 🚀

@rodolforg
Copy link
Contributor

Time zooming is also important.
This PR removes it.

Wouldn't be good to keep this previous feature with Ctrl + scrolling?

@yoSachinkr
Copy link

Wouldn't be good to keep this previous feature with Ctrl + scrolling?

@rodolforg ctrl+scroll works in the inside area of the timeline panel and I think it's consistent with ctrl+shift+scroll shortcut to zoom vertically in the graph panel as well.
image

@rodolforg
Copy link
Contributor

So the timeslider could have it too, right?

  • scroll: set time
  • ctrl + scroll: zoom time view

@yoSachinkr
Copy link

So the timeslider could have it too, right?

  • scroll: set time
  • ctrl + scroll: zoom time view

It can have but I'm wondering is it really needed to have it on the timeline strip. 👀

@rodolforg
Copy link
Contributor

@KartikWatts could you please re-add the old feature but now when user presses ctrl+scroll?

@KartikWatts
Copy link
Contributor Author

Sure, I'll update the PR soon. Regards.

scroll event updates to override current zooming behaviour with moving the timeline-bar position
modifies timeline-bar position, and scroll through the panel based on center
@KartikWatts KartikWatts force-pushed the feat/scroll-on-timeline_bar-updates-current_time branch from b41306f to 86a7979 Compare June 19, 2024 17:27
@KartikWatts
Copy link
Contributor Author

@rodolforg @yoSachinkr Sorry for the delay. Updated the PR, keeping the zoom functionality now with ctrl + scroll. And timeline scrolling with scroll.

Comment on lines +395 to +396
Time scroll_time = time_model->get_time(); //scroll is based on track time
Time zoom_time = time_plot_data->get_t_from_pixel_coord(event->x); //zoom is based on time represented by pixel
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note here, after some thought I kept the separate time representation for scroll and zoom functionalities.
As zooming would ideally remain unchanged when based on time_plot_data->get_t_from_pixel_coord but for scrolling I felt it better suited with time_model->get_time() as calculation based on actual timeline-bar position on panel, kindly let me know your thoughts.

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.

Feature request: Scroll to change the current time on the Timeline bar
3 participants