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

New timezone option: seconds since unix epoch #5450

Merged
merged 6 commits into from
Mar 14, 2024

Conversation

murgeljm
Copy link
Contributor

@murgeljm murgeljm commented Mar 10, 2024

What

Added a unix epoch timestamp display option, as mentioned in issue 4866

Screenshot 2024-03-10 at 23 25 16-min

Not quite sure how compact time display should be handled:

    /// Useful when showing dates/times on a timeline
    /// and you want it compact.
    ///
    /// Shows dates when zoomed out, shows times when zoomed in,
    /// shows relative millisecond when really zoomed in.
    pub fn format_time_compact(&self, time_zone_for_timestamps: TimeZone) -> String {
...
            if let Some(datetime) = self.to_datetime() {
                let is_whole_minute = ns % 60_000_000_000 == 0;
                let time_format = if self.is_exactly_midnight() {
                    "[year]-[month]-[day]"
                } else if is_whole_minute {
                    "[hour]:[minute]"
                } else {
-                    "[hour]:[minute]:[second]"
+                    Self::get_time_prefix(&time_zone_for_timestamps)
                };
                let parsed_format = time::format_description::parse(time_format).unwrap();

                return Self::time_string(datetime, &parsed_format, time_zone_for_timestamps);
            }
...
}

Should the [hour]:[minute] be changed as well when unix timestamp is chosen?

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

@emilk
Copy link
Member

emilk commented Mar 11, 2024

format_time_compact could, for instance, show the number of seconds since epoch if we're on a whole second, else show the offset since the last second.

@murgeljm
Copy link
Contributor Author

format_time_compact could, for instance, show the number of seconds since epoch if we're on a whole second, else show the offset since the last second.

yup, that is already implemented, basically for unix epoch display everywhere there was a whole second in the format i show unix epoch instead - but if there is a time display with no seconds shown (on a whole minute) I don't know which would be better:
[ ] show unix epoch instead of hours:minute (could look busy for a compact display?)
[ ] show hours:minute regardless of the unix epoch setting (disregarding user preference)

Also another question, should I label pull request myself with include/exclude in changelog? Or is that something maintainers do?

@murgeljm murgeljm changed the title add timezone option unix epoch Add timezone option unix epoch Mar 12, 2024
@emilk
Copy link
Member

emilk commented Mar 13, 2024

I think that if the user has selected the option for "seconds since unix epoch", we should always use seconds, everywhere - no minutes or hours.

As for labels: only maintainers can add labels. I haven't found any GitHub option to allow contributors to add labels.

@murgeljm murgeljm marked this pull request as ready for review March 13, 2024 19:08
@emilk emilk added 📺 re_viewer affects re_viewer itself include in changelog labels Mar 14, 2024
@emilk emilk self-requested a review March 14, 2024 07:59
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Works great - thanks for the PR!

crates/re_log_types/src/time.rs Show resolved Hide resolved
@emilk emilk changed the title Add timezone option unix epoch Add unix epoch as timezone option Mar 14, 2024
@emilk emilk merged commit 8e24b56 into rerun-io:main Mar 14, 2024
31 of 35 checks passed
@emilk emilk changed the title Add unix epoch as timezone option New timezone option: seconds since unix epoch Apr 5, 2024
@emilk emilk added the ui concerns graphical user interface label Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog 📺 re_viewer affects re_viewer itself ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unix epoch timestamp display option; copy-able field
3 participants