HaikuArchives / Calendar

:calendar: A native Calendar application for Haiku.
MIT License
29 stars 20 forks source link

Reminders #125

Closed harshit-sharma-gits closed 2 years ago

harshit-sharma-gits commented 2 years ago

For Issue: #122

This PR is supposed to get the QueryDBManager code to be independent from the be_app, yet still working in the Calendar app.

Also, to remove the 'dead' functionality from the Calendar, such as: GoogleCalendar/ utils/ICal

Both of these do not work as of now, but yes, they can serve as a starting point for implementing the same functionality. What should we do with these?

@humdingerb suggested to have a note in the README about these not working.

harshit-sharma-gits commented 2 years ago

I think we are almost there. The only thing missing is that we make sure that the default category gets updated in all the places that have a QueryDBManager, when it is changed in the preferences.

Should we use messages to send out a message to every class that needs to get the defaultCategory updated? Well, the default category is being used in the Event Window to sort out the categories and it is updated in the Preferences. There it is being updated after the change in the preferences

nielx commented 2 years ago

I have re-reviewed the change, and I think you are right about the impact it is having on the readability and the scope of the changes. I have then reviewed the code to find how and where GetAllCategories() is used, and there are a few distinct places where the default category actually matters, namely in the PreferenceWindow.cpp (line 141), EventWindow.cpp (line 455 and 637) and CategoryWindow.cpp (line 138). With this insight, would it be better to drop the member data fDefaultCategory and make it an optional parameter to QueryDbManager::GetAllCategories() instead? In that case, you don't have to worry about updating the default category everywhere when the preferences are changed, and you can instead rely on existing functionality.

harshit-sharma-gits commented 2 years ago

With this insight, would it be better to drop the member data fDefaultCategory and make it an optional parameter to QueryDbManager::GetAllCategories() instead? In that case, you don't have to worry about updating the default category everywhere when the preferences are changed, and you can instead rely on existing functionality

QueryDBManager::GetAllCategories() is being called within QueryDBManager.cpp some times (in QueryDBManager::AddCategory() and QueryDBManager::_Initialize()). Thus, we cannot give the defaultCategory as an arguement there. How can we handle this?

nielx commented 2 years ago

With this insight, would it be better to drop the member data fDefaultCategory and make it an optional parameter to QueryDbManager::GetAllCategories() instead? In that case, you don't have to worry about updating the default category everywhere when the preferences are changed, and you can instead rely on existing functionality

QueryDBManager::GetAllCategories() is being called within QueryDBManager.cpp some times (in QueryDBManager::AddCategory() and QueryDBManager::_Initialize()). Thus, we cannot give the defaultCategory as an arguement there. How can we handle this?

Please double check that the default category is not relevant for this particular use of the method. If not, then you can put the default argument of "" (empty string) in the class declaration

harshit-sharma-gits commented 2 years ago

Please double check that the default category is not relevant for this particular use of the method. If not, then you can put the default argument of "" (empty string) in the class declaration

Done!