GNOME Bugzilla – Bug 134595
postponing a voluntary sets the timeout down to 3mins
Last modified: 2008-06-15 13:57:31 UTC
To reproduce: Set work/break times to, say, 40 minutes. The timer should show 40 minutes remaining. Now choose "take a break", then immediately postpone it. Observe that the indicator is now flashing, and shows just three minutes remaining. This seems silly. I think I would prefer that when a temporary break is cancelled, the timer just went back to what it was before. Thanks -- this is a great program.
Good idea, but will have to wait until after 2.6, since we're in freeze now.
Created attachment 35944 [details] [review] Patch fixes the bug.
This is not what the bug reporter requested... he wanted just a break he manually started to behave like this (at least as I understood it). I don't want to make all postponed breaks go to start, that would make it way too easy to "cheat".
Created attachment 36804 [details] [review] Patch fixes the bug.
Below is the table which shows different scenarios handled in the patch. Scenario Action Result -------- ------ ------- Manual break Postpone break Timer should go back to what it was before. Manual break Complete the break Timer should go back to what it was before. Complete interval Postpone Goes back to warn state. Complete interval Complete the break Goes to Start.
Hi, thanks a lot! I have a suggestion, maybe it's better to go back to Start after a complete manual break? Also, couldn't the type be added to the struct instead of using set_data?
Created attachment 36856 [details] [review] Patch fixes the bug as per suggestions given.
Nice, thanks! I modified the patch slightly, to use the typedef for the added enum instead of a gboolean, and I also made the tooltip update immediately after a break is completed.
Actually the patch is not correct after all, I revert the commit. The patch does regular breaks not work, it makes the break window never show.
Richard, apologies for coming back late here. With this patch, typing break is working fine on my machine for the test cases I mentioned above. You suggest that you slightly modified the patch, if by any chance you still have that patch around, could you please upload it here so that I can play around with it.
Created attachment 39315 [details] [review] Proposed Patch This was bugging me as well, I also made a patch. It's kinda messy but it does work.
Thanks for the patch Benjamin, I think it's a bit bad to duplicate the code like that though. I think fixing the original patch would be better, it shouldn't be that difficult. Vinay, if I remember correctly, your patch had that bug as well, but I'll try it again to make sure.
I think the table should be: Scenario Action Result -------- ------ ------- Manual break Postpone break Timer should go back to what it was before. Manual break Complete the break Goes to Start. Complete interval Postpone Goes back to warn state. Complete interval Complete the break Goes to Start. The whole point (to me) of taking a manual break is that you have reached a good point to take your break, and if you complete it, you should then be able to work for n minutes without interruption. If I manually took a break 5 minutes before the counter ran out it would be silly to make me take another in 5 minutes.
I think that's exactly what we've already arrived at above ;)
Created attachment 39365 [details] [review] Rework of the last patch This one is much cleaner code but lacks the ability to go back to the original time minus the break time
*** Bug 339116 has been marked as a duplicate of this bug. ***
Richard, could you review the last patch please, and commit (before 2.19.91) if it looks ok to you?
I don't have a 2.19 development environment set up, sorry. But the patch looks alright. If someone wants to test it and it seems to work, I wouldn't mind if it was applied.
*** Bug 536066 has been marked as a duplicate of this bug. ***
Created attachment 112731 [details] [review] Save working time patch I offer this patch. It save time before break and if user cancel break, restore time.
In my limited testing this patch seems to work quite well. Thanks, Andrey. In the future, please also use the -p parameter to diff which helps enormously when reviewing patches. 2008-06-15 Jens Granseuer <jensgr@gmx.net> Patch by: Andrey Gusev <ronne@list.ru> * drwright.c: (update_icon), (blink_timeout_cb), (maybe_change_state), (update_tooltip), (break_window_postpone_cb): when postponing a voluntary break, go back to the state before taking the break instead of going to warn state as we do when a regular break is postponed (bug #134595)