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 Trigger cmd Support. #270

Merged
merged 6 commits into from
Jan 6, 2022
Merged

Added Trigger cmd Support. #270

merged 6 commits into from
Jan 6, 2022

Conversation

Liupold
Copy link
Contributor

@Liupold Liupold commented Jan 4, 2022

This Implements the feature request of #269.

An example section of .ini (Manually tested) is given below:

[Trigger]
work_state_cmd = ["notify-send", "work_state_cmd"]
work_paused_state_cmd = ["notify-send", "work_paused_state_cmd"]
long_break_state_cmd = ["notify-send", "long_break_state_cmd"]
small_break_state_cmd = ["notify-send", "small_break_minutes"]

This sends a notification when certain events are triggered.

Extra Modules

  • ast
  • subprocess

@JaDogg
Copy link
Owner

JaDogg commented Jan 4, 2022

Love it. Will take a look when I'm free.
Can you also add your details to the README in Contributors

@Liupold
Copy link
Contributor Author

Liupold commented Jan 4, 2022

Love it. Will take a look when I'm free.

Thanks

Can you also add your details to the README in Contributors

Done.

pydoro/pydoro_core/tomato.py Outdated Show resolved Hide resolved
@@ -106,6 +113,14 @@ def _ini_load(self):
self.long_break_minutes = float(self._conf["Time"]["long_break_minutes"])
self.alarm_seconds = int(self._conf["Time"]["alarm_seconds"])
self.key_bindings = self._conf["KeyBindings"]
self.work_state_cmd = \
Copy link
Owner

Choose a reason for hiding this comment

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

This is not backwards compatible with what we already got in .ini. Perhaps put this section in a try catch

and if there's a KeyError set the values to empty arrays [] 👍🏽

Copy link
Owner

@JaDogg JaDogg Jan 4, 2022

Choose a reason for hiding this comment

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

try:
	self.work_state_cmd = \        
		ast.literal_eval(self._conf["Trigger"]["work_state_cmd"])
	self.work_paused_state_cmd = \
		ast.literal_eval(self._conf["Trigger"]["work_paused_state_cmd"])
	self.small_break_state_cmd = \
		ast.literal_eval(self._conf["Trigger"]["small_break_state_cmd"])
	self.long_break_state_cmd = \
		ast.literal_eval(self._conf["Trigger"]["long_break_state_cmd"])
except KeyError:
	self.work_state_cmd = []
	self.work_paused_state_cmd = []
	self.small_break_state_cmd = []
	self.long_break_state_cmd = []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

try:
	self.work_state_cmd = \        
		ast.literal_eval(self._conf["Trigger"]["work_state_cmd"])
	self.work_paused_state_cmd = \
		ast.literal_eval(self._conf["Trigger"]["work_paused_state_cmd"])
	self.small_break_state_cmd = \
		ast.literal_eval(self._conf["Trigger"]["small_break_state_cmd"])
	self.long_break_state_cmd = \
		ast.literal_eval(self._conf["Trigger"]["long_break_state_cmd"])
except KeyError:
	self.work_state_cmd = []
	self.work_paused_state_cmd = []
	self.small_break_state_cmd = []
	self.long_break_state_cmd = []

I Think setting the default value of the config and updating them when a key is present in .ini file is a better solution. This will also help if a line gets accidentally deleted in the config, and the config file can be short and user-specific.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm it doesn't work the way you think it does.

Default values are written to config file if the file does not exist.

If some user already has pydoro and updates to a new version, it will not be able to find Trigger section as it was not there in previous versions of pydoro, when it wrote the .ini file.

Hope that make sense.

Copy link
Owner

@JaDogg JaDogg Jan 5, 2022

Choose a reason for hiding this comment

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

Alternatively we can do it like this. This way if a line is accidently deleted it wll not work and user can edit fix the .ini manually. but also backwards compatible for users who already got pydoro and their .ini file.

if not self._conf["Trigger"]:
	self._conf["Trigger"] = {}
	self._conf["Trigger"]["work_state_cmd"] = "[]"
	self._conf["Trigger"]["work_paused_state_cmd"] = "[]"
	self._conf["Trigger"]["long_break_state_cmd"] = "[]"
	self._conf["Trigger"]["small_break_state_cmd"] = "[]"

self.work_state_cmd = \
	ast.literal_eval(self._conf["Trigger"]["work_state_cmd"])
self.work_paused_state_cmd = \
	ast.literal_eval(self._conf["Trigger"]["work_paused_state_cmd"])
self.small_break_state_cmd = \
	ast.literal_eval(self._conf["Trigger"]["small_break_state_cmd"])
self.long_break_state_cmd = \
	ast.literal_eval(self._conf["Trigger"]["long_break_state_cmd"])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check out c4a13f8, with this the behaviour remains same, if no config file is present, but partial config files are also supported.

Copy link
Contributor Author

@Liupold Liupold Jan 5, 2022

Choose a reason for hiding this comment

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

Now The following is a valid config file:

[Trigger]
work_state_cmd = ["notify-send", "work_state_cmd"]
work_paused_state_cmd = ["notify-send", "work_paused_state_cmd"]
long_break_state_cmd = ["notify-send", "long_break_state_cmd"]
small_break_state_cmd = ["notify-send", "small_break_minutes"]

this will also prevent any backwards compatibility issues down the line.

pydoro/pydoro_core/tomato.py Outdated Show resolved Hide resolved
pydoro/pydoro_core/tomato.py Outdated Show resolved Hide resolved
pydoro/pydoro_core/tomato.py Outdated Show resolved Hide resolved
@JaDogg JaDogg merged commit 43fba4e into JaDogg:develop Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants