Skip to content

Support for editing alarms #321

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

Merged
merged 4 commits into from
Mar 18, 2016
Merged

Support for editing alarms #321

merged 4 commits into from
Mar 18, 2016

Conversation

languitar
Copy link
Contributor

As requested in #301, this is a work in progress pull request for editing alarms.

for a in self._vevents[self.ref].subcomponents
if a.name == 'VALARM' and self._can_handle_alarm(a)]

def updated_alarms(self, alarms):
Copy link
Member

Choose a reason for hiding this comment

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

You can use the alarms.setter decorator here, FWIW, but better to stay consistent.

But: please rename to update_alarms

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, sure. this was a typo.

@geier
Copy link
Member

geier commented Jan 11, 2016

Thank you! As far as I see, the only thing that really is left, is validation of user inputs, right?

While it doesn't make the event editor any prettier, that's not your fault at all.
We probably should improve the theming a lot, e.g. by making titles and user input field better separable.

@languitar
Copy link
Contributor Author

Yes, that's what is missing to my mind. And optionally supporting more types of alarms. It's funny that the Ical spec supports so many more types of alarms than ical on OSX supports. ;) Unfortunately I am currently a bit out of time for fixing this. I hope I find some time in a few days.

@sdx23
Copy link
Contributor

sdx23 commented Jan 31, 2016

I'm a big fan of this feature. Thanks for the good work, keep it going :)

@geier
Copy link
Member

geier commented Feb 4, 2016

I created a new widget type ValidateEdit, this should make validation really easy, just pass along a validation function and you should be good to go. If you need any help, just say so. If you don't have the time for it, I will rebase and merge your branch if that's ok with you.

@languitar
Copy link
Contributor Author

@geier great, I've seen the commit and already thought that this might be useful. I hope I will find some time at the weekend to finalize this.

@fpytloun
Copy link
Contributor

Looks great! Only few more wishes:

  • it would be good to setup default alarm for every new event in config (eg. notification 10 minute before event start)
  • it would be great to introduce some --alarm command (to be executed from cron) which would execute action at alarm time (eg. I have alarm.sh for this purpose)

@geier
Copy link
Member

geier commented Feb 14, 2016

I agree with @fpytloun first suggestion (but I'll happily merge this without it), but I won't do anything about the second one (see #148). However, if someone else wants to implement this (and keep supporting it), I'm happy to accept a PR.

Johannes Wienke added 2 commits March 17, 2016 23:57
Extends the event class with a getter and an update method for alarms.
This method only handles alarm types which can be handled by khal.
Others are not touched.
@geier
Copy link
Member

geier commented Mar 17, 2016

Hi @languitar, I took the liberty of rebasing your PR and making the EditWidgets in the DurationWidget validate input (see branch alarms). If you are okay with that, I'll merge that branch. Could you add yourself to the AUTHORS.txt file, please?

@languitar
Copy link
Contributor Author

@geier Thank you. I'm sorry, I didn't find the time to do this so far.

Should I overwrite my own branch with yours and add the authors change or what is the idea?

@geier
Copy link
Member

geier commented Mar 18, 2016

@languitar nothing to be sorry for, you did all the work. I don't mind really, you cant just do a PR against my alarms branch, another PR with just the AUTHOR change against master, rebase your own alarms branch against current master and cherry-pick my commit (or just do it by hand, I don't care about atttribution). Just do what is most convenient for you.

@languitar
Copy link
Contributor Author

Ok, I have updated my own branch.

I also changed the return statement for the unsigned_integer function as discussed.

@staticmethod
def unsigned_int(number):
"""test if `number` can be converted to a positive int"""
try:
Copy link
Member

Choose a reason for hiding this comment

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

Looks like my previous comment got lost in the void during rebasing.

IMHO, this looks simpler/cleaner:

try:
    return int(number) >= 0
except ValueError:
    return False

geier added a commit that referenced this pull request Mar 18, 2016
Support for editing alarms
@geier geier merged commit 481f72e into pimutils:master Mar 18, 2016
@geier
Copy link
Member

geier commented Mar 18, 2016

Thank you for your patience and this big improvement!

@geier geier mentioned this pull request Mar 30, 2016
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.

6 participants