Review time problems #75
Conversation
|
I like this. It solves the missing hours problem in a nice way, by adding it onto the minutes instead. I think I will be merging this request and then making a few tweaks to it to change the 3 show options as they are could be more useful. We want the time format to be more automatic based on the time being displayed. We had been thinking of making it automatic so that when the time is greater than an hour, it shows. Likewise for the other time units... So showing only seconds would show the minutes if required... However your solution makes me wonder... As your solution respects the show options and corrects them accordingly. Well, it corrects the minutes display for missing hours. Technically, the same should be true for seconds and minutes. But how far do we go with the options... In general they are pretty useless and I doubt anyone is really using them atm other than to turn on the hours. And having 3 options for show: hours, mins and secs does mean we should be looking further up as seconds depends on minutes, which depends on hours. Even if only showing hours and seconds would be unusual. Enough of my waffling... I think I'll go with your solution. |
|
After reviewing the code and considering the following:
I decided that the showUNIT options, like showHours, should directly control whether the unit is displayed in the format and it should not depend on there being a value > 0 in it. This way, the currentTime can be 0:37:42 and duration 1:24:35. ie., So both time formats match. Using your code it would be: currentTime 37:42 and duration 1:24:35. While I agree that both have their uses... Or personal preference... Consider the same case for the minutes. I have made the seconds behave in the same way now BTW, 60*min when !showMin. The display would be just the seconds for the first 59 seconds and then the 1:00 appears. Decided to do it this way mainly so that the format of the currentTime and duration always match. |
Issue #69