Automatically select user timeline if no timeline was explicitly selected yet #2986
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Wumpf
added
enhancement
emilk
reviewed
Aug 15, 2023
emilk
reviewed
Aug 15, 2023
| .or_insert_with(|| TimeState::new(view.min)) | ||
| .view = Some(view); | ||
| } | ||
|
|
||
| /// The range of time we are currently zoomed in on. | ||
| pub fn reset_time_view(&mut self) { | ||
| if let Some(state) = self.states.get_mut(&self.timeline) { | ||
| if let Some(state) = self.states.get_mut(self.timeline.get()) { |
There was a problem hiding this comment.
Instead of changing all &foo to foo.get(), just implement Deref for EditableAutoValue!
There was a problem hiding this comment.
for those cases I still need get() due to the way Deref works in rust: *self.timeline gives the underlying Timeline by value. But with Deref in place *self.timeline.get() becomes *self.timeline and self.timeline.get().typ() becomes self.timeline.typ()
There was a problem hiding this comment.
could ofc also do &*self.timeline.get() if that's preferred
jleibs
reviewed
Aug 15, 2023
| // If the timeline is auto refresh it every frame, otherwise only pick a new one if invalid. | ||
| if self.timeline.is_auto() || !is_timeline_valid(self.timeline(), times_per_timeline) { | ||
| self.timeline = EditableAutoValue::Auto( | ||
| default_timeline(times_per_timeline.timelines()).map_or(Default::default(), |t| *t), |
There was a problem hiding this comment.
The alphabetical order behavior is a bit unexpected (though I agree still a net improvement).
Since times_per_timeline persists with the EntityDb, internally tracking insertion order as part of TimesPerTimeline::insert() wouldn't be all that painful though.
There was a problem hiding this comment.
Hmm I think you're right. I'll try to go the extra step tomorrow
There was a problem hiding this comment.
poked it a bit, but changing this BTreeMap of timelines right now is a bit too annoying for the moment and then it gets less viable for a patch release
jleibs
approved these changes
Aug 15, 2023
emilk
pushed a commit
that referenced
this pull request
Aug 17, 2023
…cted yet (#2986) * Fixes #2186 Slight deviation from the desired behavior from the ticket: We'll always select the user timeline which is _alphabetically first_. I deem this good enough though and an improvement in any case! Going back the the "auto select" state is not possible explicitely, but will happen if for whatever reason no user timeline is available anymore. Tested as seen in the video and then confirmed separately the described alphabetic behavior https://github.com/rerun-io/rerun/assets/1220815/0d3303d7-7b19-4854-993d-0523b3e35cbb ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/2986) (if applicable) - [PR Build Summary](https://build.rerun.io/pr/2986) - [Docs preview](https://rerun.io/preview/pr%3Aandreas%2Ffix-timeline-auto-select/docs) - [Examples preview](https://rerun.io/preview/pr%3Aandreas%2Ffix-timeline-auto-select/examples)
emilk
pushed a commit
that referenced
this pull request
Aug 17, 2023
…cted yet (#2986) * Fixes #2186 Slight deviation from the desired behavior from the ticket: We'll always select the user timeline which is _alphabetically first_. I deem this good enough though and an improvement in any case! Going back the the "auto select" state is not possible explicitely, but will happen if for whatever reason no user timeline is available anymore. Tested as seen in the video and then confirmed separately the described alphabetic behavior https://github.com/rerun-io/rerun/assets/1220815/0d3303d7-7b19-4854-993d-0523b3e35cbb ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/2986) (if applicable) - [PR Build Summary](https://build.rerun.io/pr/2986) - [Docs preview](https://rerun.io/preview/pr%3Aandreas%2Ffix-timeline-auto-select/docs) - [Examples preview](https://rerun.io/preview/pr%3Aandreas%2Ffix-timeline-auto-select/examples)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Slight deviation from the desired behavior from the ticket: We'll always select the user timeline which is alphabetically first. I deem this good enough though and an improvement in any case!
Going back the the "auto select" state is not possible explicitely, but will happen if for whatever reason no user timeline is available anymore.
Tested as seen in the video and then confirmed separately the described alphabetic behavior
Screen.Recording.2023-08-15.at.16.17.20.mov
Checklist