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

Saving/Loading for events and GUI Changes for Calendar #127

Merged
merged 4 commits into from
Sep 9, 2022

Conversation

harshit-sharma-gits
Copy link
Contributor

For issue: #122

The scope of this PR: Get the events to be saved with the Reminder as per the user's requirement

The GUI looks like this:

EventManagerGUI Revised

Copy link

@nielx nielx left a comment

Choose a reason for hiding this comment

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

Some notes/comments. Could you explain in the commit message/PR description what the data model is going to be like? When is a reminder set? How to change/remove a reminder?

main/src/EventWindow.cpp Outdated Show resolved Hide resolved
main/src/EventWindow.cpp Show resolved Hide resolved
main/src/EventWindow.cpp Outdated Show resolved Hide resolved
main/src/EventWindow.cpp Outdated Show resolved Hide resolved
main/src/EventWindow.cpp Show resolved Hide resolved
main/src/EventWindow.cpp Outdated Show resolved Hide resolved
@harshit-sharma-gits
Copy link
Contributor Author

Could you explain in the commit message/PR description what the data model is going to be like? When is a reminder set?

While creating the event, the user can (if needed) set the reminder, let's say x hours/minutes/seconds before the event start time.
If the reminder is set by the user, it gets stored in the event file as EVENT:REMINDER attribute with the value start time minus x
And if, the reminder is not set by the user then no such attribute is added to the file.

How to change/remove a reminder?

Simply click on the event inside the Calendar. It'll open the Event Window with the Event's details in it. The reminder is also modifiable there.

Copy link

@nielx nielx left a comment

Choose a reason for hiding this comment

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

Some more notes. I see two edge cases that need to be handled:

  • Entering invalid characters in the reminder time.
  • Updating the event time should update the reminder time.

main/src/EventWindow.cpp Show resolved Hide resolved
main/src/EventWindow.cpp Show resolved Hide resolved
@harshit-sharma-gits
Copy link
Contributor Author

Some more notes. I see two edge cases that need to be handled:

  • Entering invalid characters in the reminder time.

Now this is sorted by TextView()'s DisallowChar()

  • Updating the event time should update the reminder time.

This already is being handled. If the user changes the event's start time from the Calendar, the Reminder time changes as well.

@nielx
Copy link

nielx commented Sep 9, 2022

I did some testing and I found two issues. The first I want you to fix as part of this commit. The second you can create a ticket for to solve later.

  1. When you create an entry with a reminder, and save it. And then you reopen the event and untick the reminder box, the reminder does not get deleted when you close the event. It will still be there when you reopen. Please fix.
  2. If you put in a large number for the reminder, the data gets saved wrong. There should be some sort of validation/limits on how/when can be reminded. This can be deferred to later, though please create a follow up

@harshit-sharma-gits
Copy link
Contributor Author

  1. When you create an entry with a reminder, and save it. And then you reopen the event and untick the reminder box, the reminder does not get deleted when you close the event. It will still be there when you reopen. Please fix.

This issue is now fixed.

  1. If you put in a large number for the reminder, the data gets saved wrong. There should be some sort of validation/limits on how/when can be reminded. This can be deferred to later, though please create a follow up

I'll open an issue for this, and also mention this issue in the wrap-up blog post.

@nielx nielx merged commit de85b86 into HaikuArchives:master Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants