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

Added new options to use arbitrary time format. #124

Merged

Conversation

ucchiee
Copy link
Contributor

@ucchiee ucchiee commented Sep 18, 2021

Added new options because I wanted to use my original time format.

  • dracula-arbitrary-time-format : true/false, default to false
  • dracula-time-format : time format, default to "%Y-%m-%d(%a) %H:%M"

Where should I write readme??

@ucchiee ucchiee changed the title Feature/time/arbitrary format Added new options to use arbitrary time format. Sep 18, 2021
@ethancedwards8
Copy link
Member

ethancedwards8 commented Oct 9, 2021

Hmm, why are you using an if inside a case? Seems odd.
EDIT: Nevermind, makes sense, didn't look closely enough.

@ethancedwards8
Copy link
Member

Where should I write readme??

What do you mean?

@ethancedwards8
Copy link
Member

So this is a good idea, but instead of creating two options here, could we just check if dracula-time-format is empty or not and go from there?

@ucchiee
Copy link
Contributor Author

ucchiee commented Oct 15, 2021

Where should I write readme??

What do you mean?

I think I need to add some description about this new option, but couldn't find out where to write.

could we just check if dracula-time-format is empty or not and go from there?

You are right. So, how about this one ? (deleted the dracula-arbitrary-time-format option)

    if [ $plugin = "time" ]; then
      IFS=' ' read -r -a colors <<< $(get_tmux_option "@dracula-time-colors" "dark_purple white")
      if [ -n "$time_format" ]; then
        script=${time_format}
      else
        if $show_day_month && $show_military ; then # military time and dd/mm
          script="%a %d/%m %R ${timezone} "
        elif $show_military; then # only military time
          script="%a %m/%d %R ${timezone} "
        elif $show_day_month; then # only dd/mm
          script="%a %d/%m %I:%M %p ${timezone} "
        else
          script="%a %m/%d %I:%M %p ${timezone} "
        fi
      fi
    fi

@ucchiee
Copy link
Contributor Author

ucchiee commented Oct 15, 2021

I confirmed that the code above works.

Btw, the time related options are hard to configure and adding this new option would make them unnecessary. (but deleting them would break backward compatibility...) What do you think about this?

@ethancedwards8
Copy link
Member

Does the code work with time zones? Not sure if you can add it with the % syntax, or if you need to add the ${timezone} variable.

@ethancedwards8
Copy link
Member

Also, for backwards compatibility, let's keep the existing options at least for the time being. Maybe in the future they can be removed.

If you could squash all your commits into one that would be useful. Also updating your PR title.

@ethancedwards8
Copy link
Member

ethancedwards8 commented Oct 15, 2021

Yes, you need to document the options in INSTALL.md. A baby should be able to follow them ;)

@ucchiee
Copy link
Contributor Author

ucchiee commented Oct 16, 2021

Does the code work with time zones?

Yes, you can add timezones like this:

set -g @dracula-time-format "%Y-%m-%d(%a) %H:%M:%S #(date +%Z)"

Should I change to respect the @dracula-show-timezone option? I think it might make this option a little bit complicated.

Also, for backwards compatibility, let's keep the existing options at least for the time being. Maybe in the future they can be removed.

Understood.

@ethancedwards8
Copy link
Member

Should I change to respect the @dracula-show-timezone option? I think it might make this option a little bit complicated.

IMO, yes.

@NitroCao
Copy link

Hi, will this PR be merged in the future?

@ucchiee ucchiee force-pushed the feature/time/arbitrary_format branch from 307ecc4 to 597f7be Compare July 25, 2022 14:13
@ethancedwards8 ethancedwards8 merged commit 96a3984 into dracula:master Apr 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants