Reminders for Events #123

harshit-sharma-gits · 2022-08-03T11:09:24Z

For Issue: #122
Started working on the calendar_daemon
Currently I tested out sending Notifications from the Application with the help of BNotification

I'm submitting this PR for regular updates regarding the coding and their review

…ReminderEventList

humdingerb

There's no need to put the code in an additional "main" folder.
The main Calendar app, howver, should move into a "main" folder, i.e. rename "src" to "main".

The events should be gathered with a query instead of reading the whole "events" folder. After all, in the end we only want to have events with a "Reminder" attribute in our schedule.

humdingerb · 2022-08-03T18:24:02Z

Also, I suggest to use a BAlert instead of a BNotification.
Currently, notifications cannot be made "sticky", i.e. they automatically disappear after the set amount of seconds. It would be too easy to miss such a notification. BAlerts stay until the user clicks a button.

nexus6-haiku · 2022-08-03T18:56:51Z

I disagree, the BAlert does not seem an option to me. All major OS use notifications coupled with a sound and so do third party apps like Outlook. Notifications seem the best way to, well... notify the user, IMO.
As a side note, I suggest to take inspiration from MacOS and maybe implement something similar to the Notification Center to visualise any missed notification (at the OS level, of course).

humdingerb · 2022-08-04T12:30:03Z

We should use what works with the task at now. Once Haiku's notification services have improved, it's trivial to replace the BAlert with a BNotification.
Unfortunately, while such work was started, there hasn't been any progress for a long while. See https://discuss.haiku-os.org/t/notifications-preflet-changes

…ugh the whole Events directory

harshit-sharma-gits · 2022-08-05T13:12:05Z

Hey @humdingerb ! Can you please check out the folder structure now?

humdingerb · 2022-08-05T18:22:39Z

Personally, I'd remove the "src" folders and have the source right under main | daemon.

humdingerb

There are quite few stray whitespaces in the code. You can configure Koder to show them, if that's your editor of choice.

daemon/Makefile

daemon/src/CalendarDaemon.cpp

harshit-sharma-gits · 2022-08-08T18:10:22Z

Hello!
Completed a lot of work! Now the Daemon is in working condition!

Let me summarize what I did:

Node Monitoring: We were not supposed to do Node Monitoring, but Live Queries. I tried to get the Live Query working ,but in vain. I used the SetTarget(be_app_messenger), so that we can receive B_QUERY_UPDATE message whenever the Events were updated. But the message wasn't getting received. So, for the app to work now I used Node Monitoring. It is successfully Refreshing the fEventList whenever an event is added/removed (But is not working for event time change!)
EventLoop Thread: The Loop for firing alerts/reminders is working in a separate thread and is successfully showing the alerts/reminders for the events (and yes, the Node Monitoring is working in sync with it!)
Semaphores: fEventLock and fNotify are the semaphores responsible for syncing between the "Alert Showing" thread and the "Node Monitoring"

I have tested the program. It is showing notifications on the Event Start time and is taking the Adding/Removing of events fruitfully (But the event changes is not Refreshing the list)

nielx · 2022-08-08T20:00:49Z

So your next job is to find out why live queries are not working.
How did you test it?
Did you find an example in the Haiku Source Code that can teach you something?

nielx · 2022-08-08T20:03:27Z

Also... I was looking at the Calendar source code. Can you tell me what QueryDBManager::GetEventsToNotify() does and where it is used?

harshit-sharma-gits · 2022-08-09T06:27:36Z

How did you test it?
Did you find an example in the Haiku Source Code that can teach you something?

I did not find an example, but this documentation. And I tested it out as follows:

In the constructor -

CalendarDaemon::CalendarDaemon()
{
	BQuery query;
	query.SetVolume(&fQueryVolume);
	query.PushAttr(START_ATTR);
	query.PushUInt32(time(NULL));
	query.PushOp(B_GE);

	if(query.SetTarget(be_app_messenger) != B_OK)
		std::cout << "Query Target not set" << std::endl;

	query.Fetch();
...
}

As the target is now set, the query can send the B_QUERY_UPDATE message whenever there is some change in the events. So in the CalendarDaemon::MessageReceived() I put this case to see if the Live Query if working-

CalendarDaemon::MessageReceived(BMessage* message)
{
	switch(message->what)
	{
		case B_QUERY_UPDATE:
			std::cout << "Events Changed - Live Query" << std::endl;
			break;
		...
	}
}

But the message is not receiving, in any change case of the events (Adding, Removing, or Attr Change)
I also tried to make the BQuery query; a class member BQuery fQuery;, so that if the scope of the query is only limited to the constructor, then it can be fixed.

Still doesn't work.

nielx · 2022-08-09T06:53:00Z

There are two reasons it is not working:

The first thing is that query is a local object, and as such the live query is ended as soon as you are out of the scope of the constructor. So you should definitely approach it with BQuery being a class member.
Secondly, you are targeting be_app_messenger. This is a global 'convenience' object that quickly helps you target messages to the BApplication object. However, at this stage you are in the constructor of that BApplication object. Thus the messenger is likely invalid and therefore you are not receiving messages. At this stage, you can target this.

harshit-sharma-gits · 2022-08-09T07:19:15Z

Also... I was looking at the Calendar source code. Can you tell me what QueryDBManager::GetEventsToNotify() does and where it is used?

QueryDBManager::GetEventsToNotify() just returns the UNNOTIFIED events list
There is an implementation for showing the Notifications when the Event starts. I also tested it out, but it is very buggy.
Although there is a seperate thread for the NotificationLoop, but the Notifications doesn't show up unless the app is running.
The Notifications also doesn't show up, when the start time of an event is changed.

Though this might be a useful utility, but it certainly needs some work. Also, when the calendar_daemon is deployed, we can also set up the Event Start Notifications from there as well.

harshit-sharma-gits · 2022-08-09T10:16:47Z

Secondly, you are targeting be_app_messenger. This is a global 'convenience' object that quickly helps you target messages to the BApplication object. However, at this stage you are in the constructor of that BApplication object. Thus the messenger is likely invalid and therefore you are not receiving messages. At this stage, you can target this.

Thanks for the suggestion! I moved the query "build-up" to ReadyToRun() function; as the messenger is set-up after the constructor.

I still am not able to use the be_app_messenger but with this, it works fine!

harshit-sharma-gits · 2022-08-09T11:23:17Z

The Query is only able to fetch the Addition and Removal of Events, as only B_ENTRY_CREATED and B_ENTRY_REMOVED are available as op-codes in B_QUERY_UPDATE

So, the Refreshing is not working when an Event's start time is changed.

How can we approach to solve this?

humdingerb · 2022-08-09T17:14:24Z

How can we approach to solve this?

One possibility is to node-monitor the events folder and if some event changed, check if it's a "remindable" event and update the list of events returned by your query.

Another is to send a BMessage from the main Calendar app to the daemon, telling it which event needs to be updated.

The second approach feels simpler, however, if a user changes an event's attribute in Tracker or Terminal or any other way outside the Calendar app, only the first approach works 100%.

harshit-sharma-gits · 2022-08-09T17:28:31Z

One possibility is to node-monitor the events folder and if some event changed, check if it's a "remindable" event and update the list of events returned by your query

For node-monitoring the events folder, we need to watch the events directory using the B_WATCH_DIRECTORY Flag, which is only able to post B_ENTRY_CREATED and B_ENTRY_REMOVED messages (no support for attribute changes?)

Another thing is to watch using the B_WATCH_ATTR Flag, but it only watches a single file, not a whole directory.

humdingerb · 2022-08-09T17:42:11Z

I suppose you'll have to watch every entry returned by your query.
Should be doable, because nobody has thousands of events planned in advance. I think... :)
Maybe, when you have it set up, you can test with various amounts of events (100, 1,000, 10,000) just to see how it impacts the system.

harshit-sharma-gits · 2022-08-11T14:39:54Z

A simply approached towards this as follows:

While adding the event to the list, called for watch_node(&nodeRef, B_WATCH_ATTR, be_app_messenger) for each event added.
Afterwards when the RefreshEventList() is called, firstly it'll stop monitoring all the nodes by stop_watching(be_app_messenger) and then again adding updated events with Node Watching.

But a wierd error is happening and I am not able to figure this out: Whenever the ATTR of any event is changed, that event (updated) is added two times in the list!

Also, after the node monitoring, the update for any change (adding/removing) is happening multiple times

humdingerb · 2022-08-11T15:33:39Z

calendar_daemon crashes on launch:
calendar_daemon-23627-debug-11-08-2022-15-32-06.report.txt

humdingerb · 2022-08-21T17:46:29Z

The SQLiteManager is still needed to migrate older data to the new attribute system. Only after a few releases, when can assume most users have updated, the migrating code the SQLiteManager can be retired.

harshit-sharma-gits · 2022-08-23T13:51:44Z

The SQLiteManager is still needed to migrate older data to the new attribute system. Only after a few releases, when can assume most users have updated, the migrating code the SQLiteManager can be retired.

Yes, that seems to be the case, but what should be done with the other classes?
I think the choices are: remove them or make necessary changes in the 'Event' declarations there?

And I haven't made any progress on the Node Monitor issue, can you help me with that?

humdingerb · 2022-08-23T15:52:39Z

Don't know about GoogleCalendar and ICal. AFAIK, both currently don't work. Maybe the code can still be used as a starting point for a future effort to support them? Maybe a note about both not working should be added to the ReadMe.

WRT node monitoring issues, even if I probably won't be capable of helping, we need a detailed description of the problem. (Or did I just miss that? It's a bit unfortunate that everything is in this one big PR...)

nielx · 2022-08-24T15:33:07Z

What is the node monitoring issue again? I tried to look through the thread here, but all I am finding is that sometimes you get multiple notifications. How is that an issue?

harshit-sharma-gits · 2022-08-26T18:54:56Z

Hey! Sorry for being inactive here for some period. I was working on resolving some confusing bugs on Saving/Loading of Events with/without Reminder.

Now the Saving/Loading bug is resolved. And The Calendar only writes Event:Reminder attribute for the events whose reminder is set by the user, elsewise the Attribute is not saved in the event file.

harshit-sharma-gits · 2022-08-26T19:30:53Z

Here I'm having a weird bug, when I try to fetch the events using the START_ATTR it works fine. But when I try to do the same with REMINDER_ATTR it doesn't fetch any events.

It should be optimum to fetch the events with this condition: Reminder Date-Time > Current Time
Which is present in the code as follows:

	fQuery.PushAttr(REMINDER_ATTR);
	fQuery.PushUInt32(time(NULL));
	fQuery.PushOp(B_GE);

But it isn't fetching any event. Yet when it is used with the START_ATTR (which denotes the starting time of the event), it fetches the events.

humdingerb · 2022-08-27T07:41:52Z

The new attribute needs to be added to the index to be queryable. See QueryDBManager::_AddIndices()

humdingerb · 2022-08-27T07:43:22Z

... and the MIME type, see QueryDBManager::_EventMimetype() above it.

harshit-sharma-gits · 2022-08-27T10:03:00Z

The new attribute needs to be added to the index to be queryable. See QueryDBManager::_AddIndices()
... and the MIME type, see QueryDBManager::_EventMimetype() above it.

Thanks for the suggestion!

harshit-sharma-gits · 2022-08-27T11:53:10Z

How to test the daemon?

Currently the main calendar application is present in Calendar/main and the daemon is present in Calendar/daemon. We can build them by giving a simple make command from their respective directories.
Then we can access them from their builds.
Open the CalendarDaemon from its build; try to get it running in the terminal so that you can see the Refreshed Event List there every time when either a Event is added/removed or some event's attribute is changed.
Now you can Launch the Calendar App, and add some events there with reminder, the Daemon watches for the events and add the appropriate events to the list, then executes them one by one on their respective times.

Also, it is not necessary to get the Daemon running before the Calendar. The Daemon can read through the events while starting itself as well.

humdingerb · 2022-08-27T17:34:01Z

From a quick test, the daemon picks up remindable events fine.
I think, however, that since we can count on the daemon running, the main Calendar app now shouldn't do any notifications and leave it all to the daemon. Which means that additionally to the "notify x minutes before the event" the daemon should also do the "notify the event is now".

The alert is missing the event name and place. The text could also be improved, suggestion:

Calendar notification!

Event: %GetEventName()%
Place: %GetEventPlace()%

The event starts in x minutes.
                                      [OK]

daemon/src/CalendarDaemon.cpp

humdingerb · 2022-08-27T17:41:54Z

daemon/src/CalendarDaemon.cpp

+
+			time_t deltaTime = event->GetStartDateTime() - event->GetReminderTime();
+
+			BString buff = "Remider!\n\n";


Needs to be localizable.

Localization done in code!

humdingerb · 2022-08-27T17:48:35Z

daemon/src/CalendarDaemon.cpp

+			buff << "\n\n";
+			if (deltaTime%3600 == 0) {
+				deltaTime /= 3600;
+				buff << "Is starting in " << deltaTime << " hours!";


To be properly translatable, including possible plural forms, you'll nee to use BStringFormat(). For a very terse intro, see https://www.haiku-os.org/community/getting-involved/translating/3rdparty/

Sure, thanks for the reference!

daemon/src/CalendarDaemon.cpp

daemon/src/CalendarDaemon.rdef

harshit-sharma-gits · 2022-08-27T20:03:19Z

I have done the Localization in the code, please check that out!
Also, should I create the en.catkeys? And add LOCALE = en in the MakeFile as well?

humdingerb · 2022-08-28T07:30:54Z

Also, should I create the en.catkeys? And add LOCALE = en in the MakeFile as well?
Yes, please.

humdingerb · 2022-08-28T07:35:58Z

@nielx: I find this huge PR increasingly unwieldy. You think it'd be OK to merge after Hashit's next changes and have smaller, separate PRs to address open issues?

nielx · 2022-08-30T06:24:20Z

@nielx: I find this huge PR increasingly unwieldy. You think it'd be OK to merge after Hashit's next changes and have smaller, separate PRs to address open issues?

Agreed that the PR is unwieldy. I also don't know what I am reviewing and I feel like it is spiraling out of control, especially the changes in the original code. I don't want to merge this as is though, it needs to be cleaned up, but it CANNOT get any new changes added. In the email last week I outlined what I think the scope of the PR should be. I will review it accordingly.

nielx

I started the review, but I left some fundamental questions that should be answered before going through every line of code.

nielx · 2022-08-30T06:25:51Z

daemon/Makefile

+#	"#include <header>". Directories that contain the files in SRCS are
+#	NOT auto-included here.
+SYSTEM_INCLUDE_PATHS = \
+	$(shell findpaths -e B_FIND_PATH_HEADERS_DIRECTORY private/interface) \


Are these includes necessary for the daemon? Please remove them one by one and keep the ones you need.

nielx · 2022-08-30T06:26:50Z

daemon/README.md

@@ -0,0 +1,2 @@
+# calendar_daemon
+A daemon app which will be responsible for sending reminders for the Calendar Application on Haiku OS


This is fine for now, but make a note to expand either the top level readme in the future, or this one, once you have figured out and implemented how this is going to be started/stopped and triggered.

nielx · 2022-08-30T06:28:22Z

daemon/locales/en.catkeys

@@ -0,0 +1,22 @@
+1	English	application/x-vnd.CalendarDaemon	3971020027


I think you should remove the locale changes in the daemon for now, as I think there is a better way. We will discuss this separately.

Okay. Should I remove only the catkeys or the code changes as well?

nielx · 2022-08-30T06:28:56Z

daemon/src/CalendarDaemon.cpp

+#include "CalendarDaemon.h"
+
+#include <csignal>
+#include <iostream>


Is this include needed? See more comments below.

nielx · 2022-08-30T06:31:06Z

daemon/src/CalendarDaemon.cpp

+
+void signalHandler(int signum)
+{
+	std::cout << "Interrupted Signal: " << signum << std::endl;


We can leave this debug output here for this PR, but it must be improved as part of this project. We need to make a decision on what to log to the system log, and under what conditions we want debug output to the standard output or to stderr. We can discuss this separately.

nielx · 2022-08-30T06:32:23Z

daemon/src/CalendarDaemon.cpp

+void signalHandler(int signum)
+{
+	std::cout << "Interrupted Signal: " << signum << std::endl;
+	exit(signum);


Is this the way to go? Would there be a reason to cleanly exit the BApplication by requesting it to quit?
(This is not a rhetorical question!)

nielx · 2022-08-30T06:35:12Z

daemon/src/CalendarDaemon.cpp

+{
+	std::cout << "Creating Daemon" << std::endl;
+
+	fEventLock = create_sem(1, "EventLock");


I am still not convinced you need semaphores and a separate thread in an object that is designed to be a message based event loop! See my earlier comments. Focus on reworking this. I understand you previously had issues with node monitoring and with Pulse, but we should resolve those as they are most likely programming errors and not fundamental issues.

You're right, this can be handled more effectively through the BMessageRunner itself. I'll work on that.
Should I start working on this in this PR, or should we merge this and create a separate PR for removing the separate thread?

nielx · 2022-08-30T06:36:35Z

daemon/src/CalendarDaemon.cpp

+
+	fDBManager = new QueryDBManager();
+
+	fEventLoop = spawn_thread(EventLoop, "EventLoop", B_NORMAL_PRIORITY, this);


See comments on event loop earlier

nielx · 2022-08-30T06:42:18Z

daemon/src/CalendarDaemon.cpp

+{
+	LockEvents();
+
+	fQuery.SetVolume(&fQueryVolume);


Is this really necessary?

The way I see it, the daemon needs to know one thing: what is the next event I need to warn for, and when is it? It should of course accommodating for the chance that there are two or more events that are raised simultaneously, but that's the only edge case I can think off.

Do you then need a running query? Or is this more of a single call to the QueryDBManager, which is meant to be the abstract parent anyway?

So in pseudo code

daemon start

list nextEvents = fQueryDbManager->GetNextReminders()

sleep until reminder

remind

list nextEvents = fQueryDbManager->GetNextReminders()

sleep until reminder

remind

If the data on the disk changes, then it should be a trigger to reload the next reminders list. That's what node monitoring is for.

So I am wondering why you need the live query.

And I think humdinger already alluded to it, you don't need to use Pulse, you can use BMessageRunner to set a timer to wake up your application when it is time to notify.

So I am wondering why you need the live query.

TBH, we do not seem to need the Live Query to gather the events, Node Monitoring will suffice for that. But it was the idea from the beginning to use the Live Queries, so here we are. Should we remove the Live Query?

nielx · 2022-08-30T06:43:00Z

daemon/src/CalendarDaemon.cpp

+		}
+		case B_NODE_MONITOR:
+		{
+			std::cout << "\nAttribute Changed - Node Monitor" << std::endl;


Shouldn't node monitoring also be called when new events are added or events are removed (files created/removed)?

Files created/removed is handled by the Live Query. For now, The Node Monitoring is only used for tracking the changes in the attributes.
Should we remove the live query and get the work done with Node Monitor only?

scottmc · 2022-10-01T23:08:02Z

What's the current status on this one?

nielx · 2022-10-02T05:38:46Z

Closing; this PR has been superseeded by individual PRs

Tested out sending notifications from the app

f120e92

harshit-sharma-gits changed the title ~~Tested out sending notifications from the app~~ Reminders for Events Aug 3, 2022

Added ReminderEvent Class and now the proram loads the Events into a …

ea80a06

…ReminderEventList

humdingerb reviewed Aug 3, 2022

View reviewed changes

Folder structure changes and now using Queris instead of reading thro…

27192fb

…ugh the whole Events directory

humdingerb reviewed Aug 5, 2022

View reviewed changes

daemon/Makefile Outdated Show resolved Hide resolved

daemon/src/CalendarDaemon.cpp Show resolved Hide resolved

daemon/src/CalendarDaemon.cpp Outdated Show resolved Hide resolved

Added Node Monitoring and Seperate thread for Eventsloop

3d7b13c

Fixed Live Queries

8e628d3

harshit-sharma-gits force-pushed the ReminderFeature branch from 0b11b2b to 8e628d3 Compare August 9, 2022 10:19

Added Node Monitor for all events to track changes in attributes

78d7b52

harshit-sharma-gits force-pushed the ReminderFeature branch from cc82019 to 78d7b52 Compare August 11, 2022 14:38

Fixed empty list bug

0cb5dce

harshit-sharma-gits added 2 commits August 26, 2022 15:03

Dead code cleanup

5e0cb87

Event Saving/Loading Bug Resolved

dac6796

Fetching events with Reminder Attribute

e1d9bbb

Query Bug Resolved

d611268

Reminder Box layout changes

b57c108

humdingerb requested changes Aug 27, 2022

View reviewed changes

Minor Code Style changes and Localization

d4430de

Proper plural handling using BStringView

4f59b6c

harshit-sharma-gits force-pushed the ReminderFeature branch from 752eee5 to 4f59b6c Compare August 28, 2022 08:41

Created en catkeys in locales and added en to Makefile

8c268c9

nielx requested changes Aug 30, 2022

View reviewed changes

harshit-sharma-gits mentioned this pull request Sep 6, 2022

Set up a basic daemon with node monitoring #126

Merged

nielx closed this Oct 2, 2022


		time_t deltaTime = event->GetStartDateTime() - event->GetReminderTime();

		BString buff = "Remider!\n\n";

		@@ -0,0 +1,2 @@
		# calendar_daemon
		A daemon app which will be responsible for sending reminders for the Calendar Application on Haiku OS

		@@ -0,0 +1,22 @@
		1 English application/x-vnd.CalendarDaemon 3971020027


		fDBManager = new QueryDBManager();

		fEventLoop = spawn_thread(EventLoop, "EventLoop", B_NORMAL_PRIORITY, this);

Reminders for Events #123

Reminders for Events #123

Conversation

harshit-sharma-gits commented Aug 3, 2022

humdingerb left a comment

Choose a reason for hiding this comment

humdingerb commented Aug 3, 2022

nexus6-haiku commented Aug 3, 2022

humdingerb commented Aug 4, 2022

harshit-sharma-gits commented Aug 5, 2022

humdingerb commented Aug 5, 2022

humdingerb left a comment

Choose a reason for hiding this comment

harshit-sharma-gits commented Aug 8, 2022 • edited Loading

nielx commented Aug 8, 2022

nielx commented Aug 8, 2022

harshit-sharma-gits commented Aug 9, 2022

nielx commented Aug 9, 2022

harshit-sharma-gits commented Aug 9, 2022

harshit-sharma-gits commented Aug 9, 2022

harshit-sharma-gits commented Aug 9, 2022

humdingerb commented Aug 9, 2022

harshit-sharma-gits commented Aug 9, 2022

humdingerb commented Aug 9, 2022

harshit-sharma-gits commented Aug 11, 2022

humdingerb commented Aug 11, 2022

humdingerb commented Aug 21, 2022

harshit-sharma-gits commented Aug 23, 2022

humdingerb commented Aug 23, 2022

nielx commented Aug 24, 2022

harshit-sharma-gits commented Aug 26, 2022

harshit-sharma-gits commented Aug 26, 2022

humdingerb commented Aug 27, 2022

humdingerb commented Aug 27, 2022

harshit-sharma-gits commented Aug 27, 2022

harshit-sharma-gits commented Aug 27, 2022

How to test the daemon?

humdingerb commented Aug 27, 2022

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

harshit-sharma-gits commented Aug 27, 2022

humdingerb commented Aug 28, 2022

humdingerb commented Aug 28, 2022

nielx commented Aug 30, 2022

nielx left a comment

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmc commented Oct 1, 2022

nielx commented Oct 2, 2022

harshit-sharma-gits commented Aug 8, 2022 •

edited

Loading