Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added module stopwatch #1216

Closed
wants to merge 17 commits into from
Closed

Added module stopwatch #1216

wants to merge 17 commits into from

Conversation

onny
Copy link

@onny onny commented Jan 6, 2018

Hey,
I modified the timer module a bit, copied it and created a simple stopwatch module.
A left click starts or pauses the timer, right click resets it.

Regards,
Jonas

@lasers
Copy link
Contributor

lasers commented Jan 8, 2018

Hey @onny. Thanks for this module. If you looked at Travis (red X right there then one more red X in build jobs), you'll find few things you need to fix.

And I wonder if it's possible / better to implement this in timer instead? Perhaps with a button to toggle between timer and stopwatch? Anyhow, I'll be happy to review this next after Travis cleared you.

@onny
Copy link
Author

onny commented Jan 8, 2018

All tests should pass now. The latest is not working because of a connection timeout of Travis ...
The idea is good but personally I prefer a seperate module so I don't have to change the behaviour of the timer/stopwatch every time. If someone is willing to change that in the future, I'll be happy to check it out 👍


def stopwatch(self):

def make_2_didget(value):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put this outside of def stopwatch(self) and maybe use def _add_zero and update the names. From make_2_didget(mins) to self._add_zero(minutes).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or look at https://stackoverflow.com/a/539360 and use "%02d" formatting to add leading numbers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

# Minutes
mins, t = divmod(t, 60)
# Seconds
seconds = t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Nit). I'd like minutes here... and just kill the comments.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

'index': 'hours',
},
{
'color': '#CCCCCC',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should kill all hardcoded #CCCCCC here. index: '???' is used to adjust numbers and we're not doing that here... so they all can go too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return {
'cached_until': cached_until,
'full_text':
self.py3.safe_format(self.format, {'stopwatch': stopwatch})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Nit)

        return {
            'cached_until': cached_until,
            'full_text': self.py3.safe_format(
                self.format, {'stopwatch': stopwatch})
        }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :D

self.running = False
self.paused = True
self.time_state = int(time() - self.time_start)
self.color = '#FFFF00'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this hardcoded colors by using this instead:

  • self.color = self.py3.COLOR_GOOD
  • self.color = self.py3.COLOR_BAD

This would allows users to change colors via...

# change good/bad for all modules config
general {
    color_good = '#ccffcc'
    color_bad = '#ffccff'
}

OR

# change good/bad for this per-module config
stopwatch {
    color_good = '#ccffcc'
    color_bad = '#ffccff'
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed :)

else:
self.time_start = time()
self.running = True
self.color = '#00FF00'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put this self.color right before the # start/restart comment along with self.running = True too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if button == 2:
# reset and pause stopwatch
self.running = False
self.paused = False
Copy link
Contributor

@lasers lasers Jan 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make a def _reset_time(self) method for this one and the one in post_config_hook too.... And we should make configurable buttons too... button_toggle = 1 and button_reset = 3 instead of hardcoded buttons.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

else:
if self.time_state:
t = self.time_state
else:
Copy link
Contributor

@lasers lasers Jan 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might be able to use elif here to make it a bit more cleaner.
EDIT: Or leave this alone and put cached_until stuffs in there instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -55,6 +55,8 @@ def post_config_hook(self):
self.time_state = None
self.color = None
self.paused = False
self.button_reset = 3
self.button_toggle = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move them up to line 50 # available configuration parameters

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@onny
Copy link
Author

onny commented Jan 20, 2018

Thanks for all the useful and educational code support. Is everything fine so far?

Copy link
Contributor

@lasers lasers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there...


Configuration parameters:
format: display format for this module (default 'Stopwatch {stopwatch}')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing docs...

button_reset: mouse button to reset the stopwatch (default 3)
button_toggle: mouse button to start/stop the stopwatch (default 1)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

stopwatch.

Button 1 starts/pauses the stopwatch.
Button 2 resets stopwatch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kill the comments. The (missing) docs will take care of this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

A simple stopwatch.

This is a very basic stopwatch. You can start, pause and reset the
stopwatch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write a bit more... A nice long description about your module. :-)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay done


if button == self.button_reset:
# reset and pause stopwatch
self._reset_time()
Copy link
Contributor

@lasers lasers Jan 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        elif button == self.button_reset:
            self._reset_time()
        else:
            self.py3.prevent_refresh()

Prevent users from updating this module with clicks/scrolling if they feel like doing it. EDIT: And users have no reasons to play with this anyway.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay done

@author Jonas Heinrich

SAMPLE OUTPUT
{'full_text': 'Stopwatch 0:01:00'}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No color 0:00:00. And change paused 54 to 58. Different number for screenshot.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :D

self._reset_time()

def stopwatch(self):

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kill this line... And keep def post_config_hook at top for consistency with other modules.

  • post_config_hook
  • _reset_time
  • stopwatch
  • on_click

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay done

def _reset_time(self):
self.running = False
self.paused = False
self.time_state = None
self.color = None

def post_config_hook(self):
self.time_start = None
self._reset_time()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing self._reset_time() in new location. Broke the module.

@onny
Copy link
Author

onny commented Jan 20, 2018

Hope it's all right now :)

@lasers
Copy link
Contributor

lasers commented Jan 21, 2018

@onny I gave you another review in case you didn't notice. This module is cool and we still can make it more cool if we want to. 👍 EDIT: With only a little more work.

Copy link
Contributor

@lasers lasers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tobes This is ready. However, I think it would be better if we default button_reset back to 2 and to set COLOR_DEGRADED instead of COLOR_BAD to stay in consistency with timer module...

I'm also thinking that it'd be good to reserve button_3 for maybe in the future, a button_lap = 3 to spawn new stopwatch (laps).

@onny Feel free to switch COLOR and button_reset if you agree and/or played with both stopwatch and timer. Nice job. Cheers. 👍

@onny
Copy link
Author

onny commented Jan 21, 2018

I'm okay with that, we can change that 👍

Copy link
Contributor

@lasers lasers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Nit) If we want to colorize everything same, then this patch would be better. Otherwise, the colons should not have colors if we want it to be identical to timer.

diff --git a/py3status/modules/stopwatch.py b/py3status/modules/stopwatch.py
index 7c433a9a..e10374af 100644
--- a/py3status/modules/stopwatch.py
+++ b/py3status/modules/stopwatch.py
@@ -62,7 +62,6 @@ class Py3status:
         self.color = None
 
     def stopwatch(self):
-
         if self.running:
             cached_until = self.py3.time_in(0, offset=1)
             t = int(time() - self.time_start)
@@ -77,35 +76,14 @@ class Py3status:
         minutes, t = divmod(t, 60)
         seconds = t
 
-        composites = [
-            {
-                'full_text': str(hours),
-                'color': self.color,
-            },
-            {
-                'color': self.color,
-                'full_text': ':',
-            },
-            {
-                'full_text': '%02d' % (minutes),
-                'color': self.color,
-            },
-            {
-                'color': self.color,
-                'full_text': ':',
-            },
-            {
-                'full_text': '%02d' % (seconds),
-                'color': self.color,
-            },
-        ]
-
-        stopwatch = self.py3.composite_create(composites)
+        stopwatch = self.py3.safe_format('\?color=%s %d:%02d:%02d' % (
+            self.color, hours, minutes, seconds))
 
         return {
             'cached_until': cached_until,
             'full_text': self.py3.safe_format(
-                self.format, {'stopwatch': stopwatch})
+                self.format, {'stopwatch': stopwatch}
+            )
         }
 
     def on_click(self, event):

@onny
Copy link
Author

onny commented Jan 23, 2018

Okay this looks better now 👍

@tobes tobes added the new module ✨ I am proposing a new module label Jan 31, 2018
@tobes tobes self-assigned this Jan 31, 2018
@onny
Copy link
Author

onny commented Mar 23, 2018

Any updates on this?

@lasers
Copy link
Contributor

lasers commented Mar 28, 2018

@onny This module is up to standards. However, stopwatch contains roughly a half of timer code. If you can implement stopwatch functionality in timer instead, then that's where we can really shine here... and is likely to make more people happy by not worrying about switching between timer and stopwatch modules versus using built-in functionality.

@tobes
Copy link
Contributor

tobes commented Aug 26, 2018

I'm with @lasers here really we should combine the two - unless we are just going for how many modules can we have - but I'm for less is more

@ultrabug
Copy link
Owner

but I'm for less is more

Same, and after all this time I guess we should face the fact that we really want this in timer instead

Closing. Please @onny if you feel like it, move this logic to timer and open a new PR that we'll be glad to integrate

Thanks again for contributing !

@ultrabug ultrabug closed this Dec 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new module ✨ I am proposing a new module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants