search

Ulauncher / ulauncher-timer

Timer Extension
50 stars 11 forks source link

Issue 20 harcoded 12h time format #23

Open brunetton opened 1 year ago

brunetton commented 1 year ago

Fix #20

gornostal commented 1 year ago

One, although a bit hacky, way would be to put an if-else there. In the if statement detect when a user wants to use 24h format, in the else - keep the am/pm format. The detection could be implement using either regex or by datetime parsing.

I haven't thoroughly thought about all nuances, but it should work, right?

On Mon, Mar 27, 2023, 16:55 Bruno Duyé @.***> wrote:

@.**** commented on this pull request.

In timer/Timer.py https://github.com/Ulauncher/ulauncher-timer/pull/23#discussion_r1149280519 :

         self.tag = None
 @property
 def description(self):
  • return f"{self.name} at {self.end_time.strftime('%-I:%M %p')}"
  • return f"{self.name} at {self.end_time.strftime('%-H:%M %p')}"

It's a good question and I searched a while to figure out. At the beginning I thought that using %H with %p would check in locale def for the locale preferences and use 12h or 24h according to it. But the doc https://docs.python.org/fr/3.6/library/datetime.html#strftime-strptime-behavior isn't clear on this point.

I digged around and I'm not sure anymore: is the 12h format automatic when locale is en_US ? Apparently this is a free interpretation. It made quite a mess in Debian when they changed it @.***/msg1662042.html> (I didn't read the whole thread).

I didn't found any "official" reference on this and this comment https://serverfault.com/a/1094385/506648 seems to go in flavor of local preferences.

I tried using specialized libs like Pendulum https://pendulum.eustace.io/docs and Arrow https://arrow.readthedocs.io/en/latest but they seems to follow the same formatting as standard DateTime: 24h format by default.

So I guess this is a matter of local choice and I suggest that we add an option to the user to use 12h format or not.

What do you think about ?

— Reply to this email directly, view it on GitHub https://github.com/Ulauncher/ulauncher-timer/pull/23#discussion_r1149280519, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJFS3272GKHHF6PH5MC2OLW6GL3ZANCNFSM6AAAAAAWHORPWI . You are receiving this because you commented.Message ID: @.***>

brunetton commented 1 year ago

To be sure to understand: if the user type something like ti 1pm we'd assume that he wants 12-h format, else we use 24h format ?

IMHO a global option for 12h format is simple to understand for users and super keep a code simple to maintain. I can do this :)

gornostal commented 1 year ago

@brunetton I took a closer look at the code. I realized I may have misunderstood your intentions. If you just want to add an option to display the time in 24h format, then it can be done by adding an option to the preferences. However, if you want to add a possibility to enter time in 24h format (what I originally thought), then it's much more complicated, see: https://github.com/Ulauncher/ulauncher-timer/blob/master/timer/query_parser.py.

I'll be happy to review PRs whether it's the first case or second or both.

brunetton commented 1 year ago

If you just want to add an option to display the time in 24h format, then it can be done by adding an option to the preferences.

Indeed :) Happy to see that we agree on that

However, if you want to add a possibility to enter time in 24h format (what I originally thought), then it's much more complicated, see: https://github.com/Ulauncher/ulauncher-timer/blob/master/timer/query_parser.py.

Yes I already read all the code I think and indeed to parse it's a little more complicated. We would really need to use a third party lib as Moment or something similar. But this is not my need for the moment as I only use timers in minutes from now

I'll be happy to review PRs whether it's the first case or second or both.

Great !