GNOME Bugzilla – Bug 727827
timer: Cant enter numbers bigger than 59 in minutes/seconds field
Last modified: 2015-02-21 14:13:43 UTC
Created attachment 273798 [details] [review] The patch! I was a little disturbed that clocks didn't offer the in the subject described feature. The appended patch changes this. This also changes the behaviour of clicking the 'start' button directly after entering a 0 into one of the spin fields while the others are already zero. Before this patch the timer started and ends instantly. After this patch the timer doesn't start and the start button deactivates himself. I needed to do this to update the numbers in the fields in this case, so this is not an own bugfix.
Review of attachment 273798 [details] [review]: not sure I like the described behavior especially the part about decreasing 0: if I have to enter 1h45m and I am on 0:00, I'd click + once for the hour and - 15 times, if the hour gets decreased it is annoying imho.
Review of attachment 273798 [details] [review]: If I decrease the number in the minutes field I expect the full number of minutes to decrease. I find it rather unintuitive to click on "-" and get a higher number. I also like the behaviour of letting the user input 90 minutes instead of force him to recalculate his number if we can do this for him. If I was to input 1h45m I would never click 15 times on the minus button but rather enter the 45 via keyboard and we'd have no problem. Even if I decrease below zero with the minus and see that the hour decreases I'd be happy that the program knows about how these numbers play together. In any case the behaviour has to be consistent so we can't take half of the patch. Just a personal opinion.
So as I see this: pros of the patched version: * you can enter 90 minutes as a shortcut for 1:30 * the input fields behave like a clock which makes sense since the user thinks of it like it is a clock and not some input fields * if a user thinks of this as a clock it behaves exactly as he expects pros of the unpatched version: * when reading from left to right the user might want to enter first the hour and then the minutes. If he uses the arrow kes/+- for adjusting the minutes there will probably will be cases where he has to readjust the hours with the patch * if a user things of it as some independend input fields hes good with the unpatched version Did I forget anything?
I guess, it is a fair summary. > * when reading from left to right the user might want to enter first the hour Note that this is much more than just reading: let's do a quick test, take ten friends and ask them to set the alarm to 8:15 using their phone. My guess is that most of them will first set the 8 and then the 15. But it is a guess, I may be wrong. A possible trade off would be to allow to enter 90 with the keyboard and turn that into 1h 30m since entering 90 is a very explicit decision of the user, while not bump the hour when wrapping around using the buttons.
(In reply to Paolo Borelli from comment #4) > A possible trade off would be to allow to enter 90 with the keyboard and > turn that into 1h 30m since entering 90 is a very explicit decision of the > user, while not bump the hour when wrapping around using the buttons. This would be far better than before for me so let's do this. Since I cannot provide any data right now let's postpone the decision about clock-like input behaviour. Marking as gnome-love as 'introductory bug'.
Review of attachment 273798 [details] [review]: I've tried your patch, and frankly that's more intuitive thing to do. Because * The one who is already using clocks and is comfortable with times up-to 59 will use it that way. * Others who might be new to clocks will have an additional feature to enter time like 90 seconds instead of 1:30 minutes. Although, I found out that while setting time if I have to enter a time like "01h59m"; the regular user might enter 1 in the hour field and then hit minus for minute to get 59 minutes, but this will also change the hour to 0h, this might take time for a regular user to get used to. But even if s/he gets used to it and now s/he starts with 2h and reduce the minutes by 1 to get "01h59m", timings like "99h59m" won't be possible for him/her. cause s/he can't go up-to 100 now. So, this could be pretty frustrating, both for regular user and the new user, and I think we should keep all the fields independent (for decrement of course).
(In reply to Ankit from comment #6) > So, this could be pretty frustrating, both for regular user and the new > user, and I think we should keep all the fields independent (for decrement > of course). So what Paolo and I've been proposing was we let them act as individual controls when the user clicks plus/minus but if he enters larger numbers (thus he explicitly wants the other field influenced because thats the only thing that makes sense) we do consider the 'carry'. (E.g. 90 min -> +1 h and 30 min)
Created attachment 297450 [details] [review] Done as decided in the bugzilla thread Linked 'input' signal of minutes and seconds field to its handler. Apparently, Gtk.Entry.get_text() return the actual value entered in the spinner as opposed to the Gtk.Entry.get_value_as_int() which returns the value after evaluating the range of spinner. Then used these values to find overflow and carry to the next field.
Review of attachment 297450 [details] [review]: Thank you for the patch! I have not tested it, but the patch seems pretty good. Here are some nitpicks 1) "Done as decided in the bugzilla thread" is not a good description, the first line of the commit is the one that will be shown in the shortlog and should be descriptive of what the patch does ::: src/timer.vala @@ -147,1 +147,2 @@ [GtkCallback] + private int input_minutes ( Gtk.SpinButton spin_button, out double new_value ) { coding style: no space within the parentheses, check the surrounding code (same in a few more places below) @@ -147,1 +147,5 @@ [GtkCallback] + private int input_minutes ( Gtk.SpinButton spin_button, out double new_value ) { + int entered_value = int.parse( spin_button.get_text() ); + + // if input entered is not within bounds than it will carry the I think you mean "then" not "than" :) @@ -148,0 +148,6 @@ + private int input_minutes ( Gtk.SpinButton spin_button, out double new_value ) { + int entered_value = int.parse( spin_button.get_text() ); + ... 3 more ... coding style: space before { @@ -148,0 +148,23 @@ + private int input_minutes ( Gtk.SpinButton spin_button, out double new_value ) { + int entered_value = int.parse( spin_button.get_text() ); + ... 20 more ... coding style: space after "if"
I'm sorry I'll fix these right away. :)
Created attachment 297460 [details] [review] timer: Added ability to enter values > 59 for minutes/seconds field Linked 'input' signal of minutes and seconds field to its handler. Apparently, Gtk.Entry.get_text() returns the actual value entered in the spinner as opposed to the Gtk.Entry.get_value_as_int() which returns the value after evaluating the range of spinner. Then used these values to find overflow and carry to the next field.
Created attachment 297464 [details] [review] timer: Add ability to enter mins/secs > 59 Linked 'input' signal of minutes and seconds field to its handler. Apparently, Gtk.Entry.get_text() returns the actual value entered in the spinner as opposed to the Gtk.Entry.get_value_as_int() which returns the value after evaluating the range of spinner. Then used these values to find overflow and carry to the next field.
Review of attachment 297464 [details] [review]: Thanks! I applied the patch. Carrying from seconds to hours is not really needed since at the most you can enter two digits, but it does not hurt either. For the future when updating a patch in bugzilla make sure to also mark the previous versions as "obsolete"