Add Pomodoro timer block #453

ghedamat · 2019-10-20T16:26:06Z

Hey there,

I decided to learn me some rust and added a "pomodoro timer" block

I'm pretty sure my rust code is probably not good enough to be merged in the mainline branch but I thought I'd open a PR and if you are interested in giving me some feedback I can work on this a bit more so that others can use it

If this is of no interest feel free to close the PR immediately, just wanted to ask :)

Thanks

What it does

Adds a pomodoro block with 5 config options

[[block]]
block = "pomodoro"
length = 25
break_length = 5
message = "pomodoro done"
break_message = "back to work!"

On left click the timer goes from stop to started and if clicked again it pauses

if it completes it starts the break phase, here left click starts the new "pomodoro" phase

on right click it always resets to stopped and resets the total pomodoro count

Things to fix

commit history
in general no idea if this is decent rust code

ghedamat · 2019-10-20T16:26:50Z

@greshake not sure if you can see drafts pr (never used one :D)

atheriel · 2019-10-20T23:25:48Z

I think this could benefit from taking a simpler approach. Instead of using this complex (and hard to understand) state machine and the machine package, I would suggest a simpler implementation:

Use an enum to represent the state.
Store the current state and the timestamp of the last state change on the Pomodoro struct.
Each time the block is updated, check how much time has elapsed since the last state change and potentially transition to a new state.

Does that make sense to you?

I have left some additional comments in the code itself. Also:

Don't worry about the Git stuff. We can squash it when it's ready.
I would suggest calling the enum variant OnBreak.

ghedamat · 2019-10-21T09:36:28Z

@atheriel it was great to learn about state machines in rust but this is much simpler indeed :) thanks for taking me off the ledge!

I also noticed that you suggested using a timestamp and comparing it but I felt the "count the seconds" approach was simpler?
if you want I can refactor again into what you suggested I'm afraid it would add a bunch of complexity on the logic that shows the elapsed time though, and I would still like this to update every second

Let me know if there are more things I should improve and thanks again for the help!

(also note I didn't see any comments on the code, just the general one above so apologies if I missed something else)

shell.nix

src/blocks/pomodoro.rs

src/blocks/mod.rs

atheriel · 2019-10-20T23:16:19Z

src/blocks/pomodoro.rs

+            state: State::stopped("\u{25a0} 0:00".to_string()),
+            length: block_config.length * 60,             // convert to minutes
+            break_length: block_config.break_length * 60, // convert to minutes
+            update_interval: Duration::from_millis(1000),


The update interval is normally configurable. Is there a reason you don't want users deviating from the 1s updates?

a timer that lasts 25 minutes that doesn't update every second didn't seem a requirement. but if you think it is I can do the approach you suggested and compare times instead. it will just be a bunch more code I think

atheriel · 2019-10-20T23:17:38Z

src/blocks/pomodoro.rs

+                }
+                State::Breaking(_state) => {
+                    if seconds >= &self.break_length {
+                        std::thread::spawn(|| -> Result<()> {


Spawning a thread so that you don't have to handle errors is not a good approach. Check out some of the other uses of Command in this project for ideas on how to handle them properly.

understood (this comment was not showing in the "files" view, thanks!

ok, sorry for hammering on this point but

I don't see anything here
https://doc.rust-lang.org/std/process/struct.Command.html

that allows me to NOT deal with the return value and have a "fire" and forget version of this.

I reverted back to my original implementation that used spawn

so it's something like

Command::new("i3-nagbar").args(&["-m", &message]).spawn()?;

it does seem to "proceed" but if I don't start it in a separate thread (as the current code does) the following happens.

i3-nagbar "returns" only when the user clicks on X on the bar, so the command keeps running until then

when the command returns it gives an output similar to

libi3] libi3/font.c Using Pango font monospace, size 8 button pressed on x = 1911, y = 17 button released on x = 1911, y = 17

I'm ignoring the output in my code
but for some reason that crashes the main thread and triggers and invalid JSON format error,

I saw the spawn pattern used a lot in the rest of i3status-rust when it comes to deal with external commands so I assumed that was the way to go.

do you have tips on where to look for debugging information if you want me to make this change?

Thanks again!

the error specifically is from i3

Error:: Could not parse JSON ...

nevermind, the issues was due to the fact that spawn inherits the stdin and stdout from the parent process so when i3-nagbar returns it breaks the output

current fix is in this commit

a8238ff

I'll mark this as resolved but expect you to comment further once the pr is in review again

thanks for helping me rubber duck the issue :)

atheriel · 2019-10-20T23:19:16Z

src/blocks/pomodoro.rs

+                State::Breaking(_state) => {
+                    if seconds >= &self.break_length {
+                        std::thread::spawn(|| -> Result<()> {
+                            match Command::new("i3-nagbar").args(&["-t", "warning", "-m", "Break over! Time to work!"]).output() {


Many users will probably not want you to "nag" them with i3-nagbar. I suggest creating a configuration option to control these messages (e.g. use_nagbar) which defaults to false.

shell.nix

ghedamat · 2019-10-21T22:17:37Z

TODO

remove shell.nix
remove use of thread.spawn
make usage of i3-nagbar optional
allow custom update intervals (rework all code to not use tick()

/cc @atheriel will ping once I have the above working (or at least some of it) thanks again!

ghedamat · 2019-10-21T23:28:19Z

src/blocks/pomodoro.rs

-                        }
-                    });
+                    Command::new("i3-nagbar")
+                        .stdout(Stdio::null())


syntax taken from
https://doc.rust-lang.org/std/process/struct.Command.html#method.stdout

ghedamat · 2019-10-26T17:30:32Z

@atheriel made all the changes you requested but not the one about custom interval.

Doing the math around updates is a bit of a pain because I really wanna support pausing of the timer so storing the start time is not sufficient.

It's doable of course and maybe just I'm being lazy but I don't really see the value of adding that complexity. is it that resource intensive to update every second?

Let me know what you think and feel free to also let me know if the other fixes are good for you

thanks again!

ghedamat · 2019-10-28T22:48:06Z

also @atheriel question, cargofmt is failing on CI but I don't understand

locally for me all the files in the project BUT my file are not respecting the formatting. am I missing something?

thanks!

ghedamat · 2019-10-28T22:54:39Z

Nevermind, found that you ended up doing a big commit upstream. rebased :)

all good just need answer to

#453 (comment)

to move forward

thanks!

ghedamat · 2019-11-15T02:22:30Z

@atheriel not sure if you saw my last comment, and sorry for nagging

do you think this is good enough for merge or you really want the ability to update with a different interval then 1 second? (see #453 (comment) for context)

thanks!

atheriel · 2019-11-30T18:23:59Z

No, I don't think that is required for the initial implementation. Thanks for your patience.

ghedamat force-pushed the pomodoro branch 3 times, most recently from ca15809 to 7ddca09 Compare October 21, 2019 09:35

ghedamat marked this pull request as ready for review October 21, 2019 09:36

ghedamat commented Oct 21, 2019

View reviewed changes

shell.nix Outdated Show resolved Hide resolved

ghedamat commented Oct 21, 2019

View reviewed changes

src/blocks/pomodoro.rs Outdated Show resolved Hide resolved

ghedamat force-pushed the pomodoro branch 3 times, most recently from 59b2362 to 2f3e34f Compare October 21, 2019 10:00

ghedamat mentioned this pull request Oct 21, 2019

Pomodoro block #372

Closed

ghedamat force-pushed the pomodoro branch 2 times, most recently from 8e2fa52 to 4fe86c9 Compare October 21, 2019 10:16

atheriel requested changes Oct 21, 2019

View reviewed changes

ghedamat force-pushed the pomodoro branch from a8238ff to f408bac Compare October 21, 2019 23:27

ghedamat commented Oct 21, 2019

View reviewed changes

ghedamat force-pushed the pomodoro branch 2 times, most recently from ea5894c to c4270c1 Compare October 21, 2019 23:47

ghedamat added 7 commits October 28, 2019 18:52

initial pomodoro

a00aa81

i3-nagbar

ac818d0

icons!

60ae851

>=

c5b0a9d

new msg

38e3d98

add break state

e58dfda

add pomodoro count

d0b36a8

ghedamat added 7 commits October 28, 2019 18:52

no more todos

729f1a3

greatly simplify implementation

2ae5793

remove shell.nix

b78eb39

remove need for thread

b12b910

i3-nagbar optional

e5dd894

wooops

402abc4

use ButtonWidget to handle clicks

60a64d1

ghedamat force-pushed the pomodoro branch from 9a5648b to 96cc6db Compare October 28, 2019 22:53

cargo fmt after rebase

9ee3e16

ghedamat force-pushed the pomodoro branch from 96cc6db to 9ee3e16 Compare October 28, 2019 22:55

atheriel approved these changes Nov 30, 2019

View reviewed changes

atheriel merged commit 2c82efd into greshake:master Nov 30, 2019

atheriel added the enhancement label Jan 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Pomodoro timer block #453

Add Pomodoro timer block #453

ghedamat commented Oct 20, 2019 •

edited

ghedamat commented Oct 20, 2019

atheriel commented Oct 20, 2019

ghedamat commented Oct 21, 2019 •

edited

atheriel Oct 20, 2019

ghedamat Oct 21, 2019

atheriel Oct 20, 2019

ghedamat Oct 21, 2019

ghedamat Oct 21, 2019 •

edited

ghedamat Oct 21, 2019

ghedamat Oct 21, 2019

atheriel Oct 20, 2019

ghedamat commented Oct 21, 2019 •

edited

ghedamat Oct 21, 2019

ghedamat commented Oct 26, 2019

ghedamat commented Oct 28, 2019

ghedamat commented Oct 28, 2019

ghedamat commented Nov 15, 2019

atheriel commented Nov 30, 2019

Add Pomodoro timer block #453

Add Pomodoro timer block #453

Conversation

ghedamat commented Oct 20, 2019 • edited

What it does

Things to fix

ghedamat commented Oct 20, 2019

atheriel commented Oct 20, 2019

ghedamat commented Oct 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghedamat Oct 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghedamat commented Oct 21, 2019 • edited

TODO

Choose a reason for hiding this comment

ghedamat commented Oct 26, 2019

ghedamat commented Oct 28, 2019

ghedamat commented Oct 28, 2019

ghedamat commented Nov 15, 2019

atheriel commented Nov 30, 2019

ghedamat commented Oct 20, 2019 •

edited

ghedamat commented Oct 21, 2019 •

edited

ghedamat Oct 21, 2019 •

edited

ghedamat commented Oct 21, 2019 •

edited