Manuel-Kehl
commented
9 years ago @fredmwangi thanks for your contributions! Please forgive the rather long list of annotations, that follows. I really appreciate your work and like your improvements :wink:
Hiding the Done Button During Breaks
After thinking about it again, I do agree with you about hiding the Done button during breaks, to avoid distraction of the user :+1:
Implementation
I would prefer if you didn't rely on UI strings like if (task_status_lbl.label == "Take a Break!"). Those can change quickly, e.g. for translation purpose. I would not even bind it to the Gtk.Button.clicked signal, as a running change may be triggered somewhere else. Thus I'd propose adding something like this in TimerView's signal handling section:
timer.active_task_changed.connect (handle_done_button_visibility);The implemnetation of the function could be simliar to this:
private void handle_done_button_visibility (Gtk.TreeRowReference ref, bool break_active) {
if (break_active) {
done_btn.visible = false;
} else {
done_btn.visible = true;
}
}The active_task_changed signal is emmited when the active_task is changed by selecting another task, but also if the break/task status changes, because in that case the break can be considered the active_task.
Implementing it as I described above, would have the effect of the button not being hidden when the timer is not running. I personally think that is better, because imho one should be able to mark a task done, as soon as it is declared the "Active Task", regardless of the timer. What do you think about that? If you'd still prefer it to behave your way, you could probably bind it to the timer_running_changed signal and manually check timer.break_active from within the handler function.
Commit History
Is there any particular reason for the commit cf77afb? If I perceive correctly, you manually reverted the changes in the following commit. It would be awesome if you could manage to fix that, e.g. via an interactive rebase that removes cf77a5b and edits 1fc1050 accordingly.
I don't want to appear overly rigorous in such things, but I think it makes the Git history less comprehensible. Do you see my point? I am open for compromises, so try to convince me, if not :stuck_out_tongue:
Creating a helper method
I am not sure if that is really necessary. In Vala it appears to be pretty common to declare such things public, if there is need for public access, as getters and setters could be defined transparently later on. From a semantic point of view it is imho OK to expose a "Done" button from a widget as complex as the TimerView.
10
I chose a quick way to hide the Done button on Startup: I changed timer_view and done_btn from private to public.
The Done button reappears once a Task is started and also when a break from the previous Task ends (this allows a user to mark the previous Task as complete after returning from their break--without having to switch to the To-Do View).
Hiding the Done Button when a break starts prevents users from completing a task during a break (they should be relaxing instead).