GNOME Bugzilla – Bug 782755
Add support for Recurrent Events
Last modified: 2017-11-24 22:29:03 UTC
Check out the project here https://wiki.gnome.org/Outreach/SummerOfCode/2017/Projects/YashSingh_Calendar_AddRecurrentEvents
Created attachment 352047 [details] [review] event: add recurrence property and function to detect recurrence The recurrence property is not editable at this point, and is set to FALSE by default.
Review of attachment 352047 [details] [review]: Some comments about the code below. About the commit message, I think it can be improved. The title should be shorted and the body, more explanative about why this patch is needed, and what benefits we gain from it. ::: src/gcal-event.c @@ +367,3 @@ + /* Set has-recurrence to FALSE for now */ + self->has_recurrence = FALSE; You should set it to e_cal_component_get_recurrences() here. (By default, class structs are initialized to zero, thus self->has_recurrence is FALSE by default.) @@ +489,3 @@ + case PROP_HAS_RECURRENCE: + g_value_set_string (value, self->has_recurrence); Should be g_value_set_boolean() @@ +711,3 @@ + g_object_class_install_property (object_class, + PROP_HAS_RECURRENCE, + g_param_spec_string ("has-recurrence", Should be g_param_spec_boolean() @@ +1035,3 @@ + */ +gboolean +gcal_event_has_recurrence (GcalEvent *self) You also need to add the prototype at gcal-event.h @@ +1041,3 @@ + g_return_val_if_fail (GCAL_IS_EVENT (self), FALSE); + + component = self->component; I now realize you don't need to have a "component" variable here. Just pass self->component to e_cal_component_has_recurrences().
Created attachment 352110 [details] [review] event: add recurrence property & has-recurrence function Recurrence property of the event enables us to add recurrences to it in the future. It is the first step in that direction.
Review of attachment 352110 [details] [review]: Nice!
Comment on attachment 352110 [details] [review] event: add recurrence property & has-recurrence function Attachment 352110 [details] pushed as 6dd2342 - event: add recurrence property & has-recurrence function
Created attachment 352675 [details] [review] manager: enhance the -update/remove- event functions add an ECalObjModType argument to -update & remove- event functions to enable recurrent events modification in the future. Currently, the argument is set, by default, to 'E_CAL_OBJ_MOD_THIS'.
Review of attachment 352675 [details] [review]: Code-wise, the patch looks simple and good (two nitpicks below). But the commit message needs improvement. First, the title is kinda hard to understand. I suggest "manager: pass ECalObjModType to update and remove". Then, the body of the is not very descriptive. It's important to write ~why~ you're adding this code. ::: src/gcal-manager.c @@ +1754,3 @@ void +gcal_manager_update_event (GcalManager *manager, + GcalEvent *event, You should update the comment above this with the new parameter. @@ +1795,3 @@ +gcal_manager_remove_event (GcalManager *manager, + GcalEvent *event, + ECalObjModType mod) You should update the comment above this with the new parameter.
Created attachment 352683 [details] [review] manager: pass ECalObjModType to update and remove adding an ECalObjModType to these functions would allow us to modify recurrent events with more flexibility. Currently, the argument is set, by default, to 'E_CAL_OBJ_MOD_THIS'.
Created attachment 352689 [details] [review] manager: pass ECalObjModType to update and remove Currently, GcalManager always assumes E_CAL_OBJ_MOD_THIS when modifying or deleting events. Since we're landing the groundwork for recurrence management, we can't assume this anymore. This patch, then, allows GcalManager:modify_event() and :remove_event() receive the ECalObjModType instead of assuming E_CAL_OBJ_MOD_THIS. However, this commit does not add recurrence support yet, and every call to these functions are still passing E_CAL_OBJ_MOD_THIS.
Comment on attachment 352689 [details] [review] manager: pass ECalObjModType to update and remove Attachment 352689 [details] pushed as 9ffa913 - manager: pass ECalObjModType to update and remove
Created attachment 352966 [details] [review] utils: add dialog to easily modify recurring events whenever a user chooses to modify an event which is recurring, a new dialog will open automatically when the user clicks 'Done' or 'Delete' in edit-dialog, after making the desired changes. this new dialog will ask the user to choose the set of events to which the changes will be applied. the options include 'This Event Only', 'Subsequent Events' and 'All Events'.
Review of attachment 352966 [details] [review]: A few ideas and comments below. About the commit message, the title is almost perfect (remove the 'easily'). About the message body, it should follow English letter case rules, and start with uppercase. ::: src/gcal-utils.c @@ +1165,3 @@ + */ +ECalObjModType +ask_recurrence_modification_type (GtkWidget *parent) Perhaps this should be: gboolean ask_recurrence_modification_type (GtkWidget *parent, ECalObjModType *modtype) If the user cancels or close the dialog with <Alt>+F4, it returns FALSE. Otherwise, it returns TRUE and *modtype is set. This way we can call it like: if (ask_recurrence_modification_type (parent, &type)) { ... } Make sure to update the comment above. @@ +1179,3 @@ + GTK_MESSAGE_QUESTION, + GTK_BUTTONS_NONE, + "The event you are trying to modify is recurring. The changes you have selected should be applied to:"); This should be translatable. Use _(). @@ +1197,3 @@ + { + case GTK_RESPONSE_CANCEL: + break; If the user cancels the dialog, it'll still return a valid ECalObjModType and it'll still remove the event. This is wrong. See my first comment.
Created attachment 352982 [details] [review] utils: add dialog to modify recurring events Whenever a user chooses to modify an event which is recurring, a new dialog will open automatically when the user clicks 'Done' or 'Delete' in edit-dialog, after making the desired changes. This new dialog will ask the user to choose the set of events to which the changes will be applied. The options include 'This Event Only', 'Subsequent Events' and 'All Events'. The function returns true and the changes take place only if the user chooses any option other than 'Cancel' or <Alt+F4>.
Created attachment 353000 [details] [review] window: pop up recur-dialog right after edit-dialog The recur-dialog opens only if the event to be modified is a recurring event and only if the user doesn't cancel the modification in the edit-dialog. If the user presses 'Cancel' in the recur-dialog after making modifications in the edit-dialog, then no changes are applied.
Review of attachment 353000 [details] [review]: Some comments below. Don't use abbreviations in the commit (like 'recur' instead of 'recurrence'). ::: src/gcal-window.c @@ +1086,3 @@ GcalView *view; GList *widgets; + ECalObjModType *mod; Not a pointer (remove *) @@ +1094,3 @@ event = gcal_edit_dialog_get_event (edit_dialog); view = GCAL_VIEW (window->views[window->active_view]); + mod = g_malloc (sizeof (ECalObjModType)); Not needed. You can pass declare "ECalObjModType mod", initialize to E_CAL_OBJ_MOD_THIS here, and pass "&mod" to ask_recurrence_modification_type(). @@ +1098,1 @@ gtk_widget_hide (GTK_WIDGET (dialog)); Instead of two different if()/else if() statements, you can move the ask_recurrence_modofication_type() call to here like: mod = E_CAL_OBJ_MOD_THIS; if (gcal_event_has_recurrence (event) && !ask_recurrence_modification_type (dialog, &mod)) return; @@ +1106,2 @@ case GCAL_RESPONSE_SAVE_EVENT: + if (gcal_event_has_recurrence (event) == FALSE) You don't have to compare gboolean to gboolean.
Review of attachment 352982 [details] [review]: One last comment. ::: src/gcal-utils.c @@ +1182,3 @@ + GTK_MESSAGE_QUESTION, + GTK_BUTTONS_NONE, + "The event you are trying to modify is recurring. The changes you have selected should be applied to:"); Still needs the _() to mark the string as translatable.
Created attachment 353031 [details] [review] utils: add dialog to modify recurring events Whenever a user chooses to modify an event which is recurring, a new dialog will open automatically when the user clicks 'Done' or 'Delete' in edit-dialog, after making the desired changes. This new dialog will ask the user to choose the set of events to which the changes will be applied. The options include 'This Event Only', 'Subsequent Events' and 'All Events'. The function returns true and the changes take place only if the user chooses any option other than 'Cancel' or <Alt+F4>.
Created attachment 353032 [details] [review] window: pop up recurrence-dialog right after edit-dialog The recurrence-dialog opens only if the event to be modified is a recurring event and only if the user doesn't cancel the modification in the edit-dialog. If the user presses 'Cancel' in the recurrence-dialog after making modifications in the edit-dialog, then no changes are applied.
Created attachment 353372 [details] [review] utils: add dialog to modify recurring events Whenever a user chooses to modify an event which is recurring, a new dialog will open automatically when the user clicks 'Done' or 'Delete' in edit-dialog, after making the desired changes. This new dialog will ask the user to choose the set of events to which the changes will be applied. The options include 'This Event Only', 'Subsequent Events' and 'All Events'. The function returns true and the changes take place only if the user chooses any option other than 'Cancel' or <Alt+F4>.
Created attachment 353373 [details] [review] window: pop up recurrence-dialog right after edit-dialog The recurrence-dialog opens only if the event to be modified is a recurring event and only if the user doesn't cancel the modification in the edit-dialog. If the user presses 'Cancel' in the recurrence-dialog after making modifications in the edit-dialog, then no changes are applied.
Created attachment 353374 [details] [review] manager: set client upon loading event Add EClient support while loading an event. This EClient then helps us check it's capability in window. Make the rid 'NULL' when calling modify_object when user chooses 'All events' in recurrence-dialog.
Review of attachment 353374 [details] [review]: On comment, otherwise the patch is good enough. ::: src/gcal-manager.c @@ +1823,3 @@ e_cal_client_remove_object (unit->client, id->uid, + (mod == E_CAL_OBJ_MOD_ALL) ? NULL : id->rid, This was supposed to be part of another patch, right?
Review of attachment 353372 [details] [review]: Some minor comments. ::: src/gcal-utils.c @@ +1193,3 @@ + _("_All events"), + GTK_RESPONSE_YES, + NULL); "Subsequent Events" should be in the middle. @@ +1195,3 @@ + NULL); + + if (!e_client_check_capability (E_CLIENT (g_object_get_data (G_OBJECT (&source), "client")), CAL_STATIC_CAPABILITY_NO_THISANDFUTURE)) Better use an EClient *client variable. This line looks pretty wide. @@ +1198,3 @@ + gtk_dialog_add_button (GTK_DIALOG (dialog), + _("_Subsequent events"), + GTK_RESPONSE_OK); You can put these 3 args in the same line.
Review of attachment 353373 [details] [review]: Looks good!
Created attachment 353380 [details] [review] utils: add dialog to modify recurring events Whenever a user chooses to modify an event which is recurring, a new dialog will open automatically when the user clicks 'Done' or 'Delete' in edit-dialog, after making the desired changes. This new dialog will ask the user to choose the set of events to which the changes will be applied. The options include 'This Event Only', 'Subsequent Events' and 'All Events'. The function returns true and the changes take place only if the user chooses any option other than 'Cancel' or <Alt+F4>.
Created attachment 353455 [details] [review] manager: set client upon loading event Add EClient support while loading an event. This EClient then helps us check its capability in window.
Created attachment 353456 [details] [review] manager: pass NULL as 'rid' when 'All Events' chosen If user chooses 'All Events' option while deleting a recurrent event, pass NULL as the 'rid' argument to e_cal_client_remove_object().
Review of attachment 353456 [details] [review]: Looks good to me.
Created attachment 353483 [details] [review] manager: pass correct rid & uid to remove-event Passing the correct rid & uid to e_cal_client_remove_object() ensures that the deletion of recurrent events is handled correctly.
Created attachment 353484 [details] [review] manager: pass NULL as 'rid' when 'All Events' chosen If user chooses 'All Events' option while deleting a recurrent event, pass NULL as the 'rid' argument to e_cal_client_remove_object().
Created attachment 353485 [details] [review] window: pop up recurrence-dialog right after edit-dialog The recurrence-dialog opens only if the event to be modified is a recurring event and only if the user doesn't cancel the modification in the edit-dialog. If the user presses 'Cancel' in the recurrence-dialog after making modifications in the edit-dialog, then no changes are applied.
Review of attachment 353380 [details] [review]: Looks good for now. In the future, you may want to call "gtk_widget_get_toplevel (parent)" so we can reuse this for e.g. Month view.
Review of attachment 353455 [details] [review]: Sweet and short.
Review of attachment 353483 [details] [review]: ++
Review of attachment 353483 [details] [review]: Looks good (will push with style fixes)
Review of attachment 353484 [details] [review]: This should cover the update_event() function as well
Review of attachment 353485 [details] [review]: Style nit ::: src/gcal-window.c @@ +1099,1 @@ + ESource *source = gcal_event_get_source (event); All variable declarations must stay at the top of the function
Attachment 353380 [details] pushed as cbc9fe7 - utils: add dialog to modify recurring events Attachment 353455 [details] pushed as 7fd3e48 - manager: set client upon loading event Attachment 353483 [details] pushed as 6b31ac8 - manager: pass correct rid & uid to remove-event
(In reply to Georges Basile Stavracas Neto from comment #39) > Review of attachment 353485 [details] [review] [review]: > > Style nit > > ::: src/gcal-window.c > @@ +1099,1 @@ > + ESource *source = gcal_event_get_source (event); > > All variable declarations must stay at the top of the function but source needs 'event' to be assigned prior to calling get_source().
Created attachment 353510 [details] [review] window: pop up recurrence-dialog right after edit-dialog The recurrence-dialog opens only if the event to be modified is a recurring event and only if the user doesn't cancel the modification in the edit-dialog. If the user presses 'Cancel' in the recurrence-dialog after making modifications in the edit-dialog, then no changes are applied.
(In reply to Yash from comment #41) > (In reply to Georges Basile Stavracas Neto from comment #39) > > Review of attachment 353485 [details] [review] [review] [review]: > > > > Style nit > > > > ::: src/gcal-window.c > > @@ +1099,1 @@ > > + ESource *source = gcal_event_get_source (event); > > > > All variable declarations must stay at the top of the function > > but source needs 'event' to be assigned prior to calling get_source(). updated the patch. ignore the previous comment.
Review of attachment 353510 [details] [review]: One last commend ::: src/gcal-window.c @@ +1103,3 @@ + response != GTK_RESPONSE_DELETE_EVENT && + gcal_event_has_recurrence (event) && + !ask_recurrence_modification_type (GTK_WIDGET (dialog), &mod, *source)) You should pass the pointer (i.e. 'source') rather than the contents of the pointer (ie '*source')
Created attachment 353514 [details] [review] utils: add ESource* argument to ask-recurrence-modification-type Earlier the argument was ESource. Changed the argument to ESource* for the function to correctly fetch "client" data from the source.
Review of attachment 353514 [details] [review]: I should not have let that go past.
Created attachment 353515 [details] [review] window: pop up recurrence-dialog right after edit-dialog The recurrence-dialog opens only if the event to be modified is a recurring event and only if the user doesn't cancel the modification in the edit-dialog. If the user presses 'Cancel' in the recurrence-dialog after making modifications in the edit-dialog, then no changes are applied.
Review of attachment 353515 [details] [review]: One comment below. Avoid changing style outside "style fix" commits. ::: src/gcal-window.c @@ -1117,3 @@ - { - create_notification (GCAL_WINDOW (user_data), _("Event deleted"), _("Undo")); - } Why did you remove the {} here? They should stay.
Created attachment 353516 [details] [review] window: pop up recurrence-dialog right after edit-dialog The recurrence-dialog opens only if the event to be modified is a recurring event and only if the user doesn't cancel the modification in the edit-dialog. If the user presses 'Cancel' in the recurrence-dialog after making modifications in the edit-dialog, then no changes are applied.
Created attachment 353518 [details] [review] window: pop up recurrence-dialog right after edit-dialog The recurrence-dialog opens only if the event to be modified is a recurring event and only if the user doesn't cancel the modification in the edit-dialog. If the user presses 'Cancel' in the recurrence-dialog after making modifications in the edit-dialog, then no changes are applied.
Review of attachment 353518 [details] [review]: Looks good
Review of attachment 353484 [details] [review]: Nevermind. ECalClient:modify_object() does't receive RID.
Attachment 353484 [details] pushed as e13bbce - manager: pass NULL as 'rid' when 'All Events' chosen Attachment 353514 [details] pushed as a9430f5 - utils: add ESource* argument to ask-recurrence-modification-type Attachment 353518 [details] pushed as 5a5dc67 - window: pop up recurrence-dialog right after edit-dialog
Created attachment 353617 [details] [review] utils: set recurrence-dialog modal to toplevel widget Setting the recurrence-dialog modal to toplevel widget allows us to take care of ask_recurrence_modification_type() calls in the various views.
Review of attachment 353617 [details] [review]: LGTM
Comment on attachment 353617 [details] [review] utils: set recurrence-dialog modal to toplevel widget Attachment 353617 [details] pushed as 19f1069 - utils: set recurrence-dialog modal to toplevel widget
Created attachment 353706 [details] [review] utils: set recurrence-dialog transient for toplevel widget Setting the recurrence-dialog transient for toplevel widget allows us to take care of ask_recurrence_modification_type() calls in the various views.
Created attachment 353707 [details] [review] views: pop up recurrence-dialog when user drags & drops recurrent events While dragging and dropping a recurrent event, pop up the recurrence-dialog to allow the user to choose the set of events to which the changes should be applied.
Review of attachment 353706 [details] [review]: Looks good
Review of attachment 353707 [details] [review]: Some comments below. The commit title is too long. ::: src/views/gcal-month-view.c @@ +783,3 @@ gint start_year, current_year; + ECalObjModType mod; + ESource *source; I usually align variables according to the type width, and typedefs always come before native types. @@ +840,3 @@ + { + return FALSE; + } This is causing memory leaks. A better way to handle this is: if (has_recurrence (event)) { if (ask_recurrence (...)) gcal_manager_update_event (mod); } else { gcal_manager_update_event (MOD_THIS); } ::: src/views/gcal-week-grid.c @@ +959,3 @@ GcalEvent *event; + ECalObjModType mod; + ESource *source; Ditto about variable types. @@ +1021,3 @@ /* Commit the changes */ + + gcal_manager_update_event (self->manager, event, mod); See Month view's comment. ::: src/views/gcal-year-view.c @@ +1533,3 @@ + } + + gcal_manager_update_event (self->manager, event, mod); See Month view's comment.
Comment on attachment 353706 [details] [review] utils: set recurrence-dialog transient for toplevel widget Attachment 353706 [details] pushed as a7add6d - utils: set recurrence-dialog transient for toplevel widget
Created attachment 353763 [details] [review] views: pop up recurrence-dialog if recurrent events are dragged & dropped While dragging and dropping a recurrent event, pop up the recurrence-dialog to allow the user to choose the set of events to which the changes should be applied.
Review of attachment 353763 [details] [review]: Some more comments. The commit title is still too big. ::: src/views/gcal-month-view.c @@ +799,3 @@ + !ask_recurrence_modification_type (widget, &mod, source)) + { + return FALSE; This should jump to the end of the function (goto out) otherwise DnD is corrupted. ::: src/views/gcal-week-grid.c @@ +994,3 @@ + !ask_recurrence_modification_type (widget, &mod, source)) + { + return FALSE; This should jump to the end of the function (goto out) otherwise DnD is corrupted. ::: src/views/gcal-year-view.c @@ +1502,3 @@ + !ask_recurrence_modification_type (GTK_WIDGET (self), &mod, source)) + { + return FALSE; This should jump to the end of the function (goto out) otherwise DnD is corrupted. @@ +1505,3 @@ + } + + mod = E_CAL_OBJ_MOD_THIS; This overrides whatever the user choose in ask_recurrence_modification_type()
Created attachment 353764 [details] [review] views: ask for recurrence when dropping recurrent events While dragging and dropping a recurrent event, pop up the recurrence-dialog to allow the user to choose the set of events to which the changes should be applied.
Comment on attachment 353764 [details] [review] views: ask for recurrence when dropping recurrent events Attachment 353764 [details] pushed as be9da0c - views: ask for recurrence when dropping recurrent events
Created attachment 353779 [details] [review] manager: set 'rid' to NULL when 'All events' chosen Setting 'rid' to NULL in gcal_manager_update_event() when 'All events' chosen allows us to modify All and Future Instances of recurrent events correctly.
Comment on attachment 353779 [details] [review] manager: set 'rid' to NULL when 'All events' chosen Pushed with some style fixes. Attachment 353779 [details] pushed as 68c604b - manager: set 'rid' to NULL when 'All events' chosen
Linguistically, "subsequent events" excludes the selected occurrence - needs to be "this and subsequent events" or an equivalent. Should I open a new bug?
Created attachment 354803 [details] [review] recurrence: Add recurrence struct to parse recurrence-rules Currently, gnome-calendar has no component to deal with recurrence-rules of events. This missing component is however required to create and modify recurring events. This patch adds the required GcalRecurrence component which parses the recurrence-rules with details like: * Frequency of the recurring event * Duration of the recurrences: ** If they repeat until a particular date ** If they repeat a fixed number of times ** If they repeat forever and then stores these details in a suitable format in the GcalRecurrence structure. The gcal-recurrence functions simplify the task of handling and editing recurrences. This structure is the first step towards creating a new 'recurrence' property of an event.
Created attachment 354804 [details] [review] event: Add recurrence property Earlier, an event had no recurrence property. Hence, the event's recurrence pattern etc. could not be analysed, and as a result could not be changed by the user. gnome-calendar just displayed all the recurrences of a recurring event but had no 'readable' knowledge regarding the characteristics of those recurrences. This patch adds the recurrence property, which is basically a GcalRecurrence structure linked to the event.
Created attachment 354805 [details] [review] edit-dialog: Add Repeat options for simple recurrences Currently, creating recurring events is not supported in gnome-calendar. This patch adds a Repeat combobox with a list of simple recurrences such as: * Monday - Friday events * Daily events * Weekly events * Monthly events * Yearly events and enables the user to create simple recurring events. There is one limitation regarding this patch, and that is, the user can't change the 'recurrence-rules' of an already recurring event. Only new recurring events can be created.
Review of attachment 354803 [details] [review]: This is actually looking really good (including the commit message!)
Review of attachment 354804 [details] [review]: Needs some work, but overall looking good. ::: src/gcal-event.c @@ +382,3 @@ + // g_message ("RRULE -> %s", icalrecurrencetype_as_string_r (rrule)); + if (rrule) + icalrecurrencetype_clear (rrule); This is probably a leftover from your testing, right? @@ +750,3 @@ + "The recurrence property of the event", + GCAL_TYPE_RECURRENCE, + G_PARAM_READWRITE)); This is a read & write property, but you missed the "case PROP_RECURRENCE" from gcal_event_set_property() @@ +1643,3 @@ +void +gcal_event_set_recurrence (GcalEvent *self, + GcalRecurrence *recur) Style issue: align parameters like the rest of the code. @@ +1673,3 @@ + * + * Gets the recurrence struct @recur of the event. + */ Documentation comment is missing the "Returns: ..." field ::: src/gcal-event.h @@ +126,3 @@ +void gcal_event_set_recurrence (GcalEvent *event, + GcalRecurrence *recur); Style issue: align function params like the rest of the code.
Review of attachment 354805 [details] [review]: Needs some work still. I can't add a recurrence to a non-recurring event, and that's a major flow of this patch. ::: data/ui/edit-dialog.ui @@ +166,3 @@ </child> <child> + <object class="GtkLabel" id="repeat_label"> Doesn't need an ID (see the code comments). @@ +422,3 @@ + + <child> + <object class="GtkBox"> You don't need to put the frequency combobox inside a GtkBox, only the limits combobox (which is not added in this patch). ::: src/gcal-edit-dialog.c @@ +535,3 @@ gtk_widget_class_bind_template_child (widget_class, GcalEditDialog, sources_popover); + gtk_widget_class_bind_template_child (widget_class, GcalEditDialog, repeat_combo); + gtk_widget_class_bind_template_child (widget_class, GcalEditDialog, repeat_label); You won't ever touch the label, no need to bind it here. @@ +735,3 @@ + recur->limit_type = GCAL_RECURRENCE_FOREVER; + gcal_event_set_recurrence (dialog->event, recur); + } else { gcal_event_set_recurrence (dialog->event, NULL); } @@ +1164,3 @@ GCAL_GOTO (out); + gtk_combo_box_set_active (GTK_COMBO_BOX (dialog->repeat_combo), 0); The repeat combobox should be pre-filled with the event's recurrence type. @@ +1229,3 @@ + /* event_was_recurrent */ + dialog->event_was_recurrent = gcal_event_has_recurrence (event) ? TRUE : FALSE; gcal_event_has_recurrence() already returns a boolean, you don't need the "? TRUE : FALSE" here @@ +1240,3 @@ + gtk_widget_show (dialog->repeat_combo); + gtk_widget_show (dialog->repeat_label); + } No. The recurrences combobox and the label are ~always~ visible. @@ +1369,3 @@ +gcal_edit_dialog_should_ask_for_recurrence_type (GcalEditDialog *self) +{ + return self->event_was_recurrent; Should have a g_return_val_if_fail() before
Review of attachment 354804 [details] [review]: ::: src/gcal-event.c @@ +1651,3 @@ + + g_return_if_fail (GCAL_IS_EVENT (self)); + g_return_if_fail (recur != NULL); No, setting a NULL recurrence means removing the current recurrence.
Comment on attachment 354803 [details] [review] recurrence: Add recurrence struct to parse recurrence-rules Attachment 354803 [details] pushed as 27bb337 - recurrence: Add recurrence struct to parse recurrence-rules
Created attachment 355184 [details] [review] recurrence: fix GDateTime to icaltime conversion The month and year values extracted from until-date of recurrences were accidentally interchanged. This patch fixes that issue by making the correct assignment.
Created attachment 355295 [details] [review] recurrence: add a function to check equality of recurrences The gcal_recurrence_is_equal function checks if two recurrences are equal or not. When a user is trying to modify the recurrence-rules of an already recurring event, this function tells us if the recurrence-dialog should pop up or not. This patch also removes some unused code.
Created attachment 355296 [details] [review] event: add recurrence property Earlier, an event had no recurrence property. Hence, the event's recurrence pattern etc. could not be analysed, and as a result could not be changed by the user. gnome-calendar just displayed all the recurrences of a recurring event but had no 'readable' knowledge regarding the characteristics of those recurrences. This patch adds the recurrence property, which is basically a GcalRecurrence structure linked to the event.
Created attachment 355297 [details] [review] edit-dialog: add 'Repeat' options for simple recurrences Currently, creating recurring events is not supported in gnome-calendar. This patch adds a Repeat combobox with a list of simple recurrences such as: * Monday - Friday events * Daily events * Weekly events * Monthly events * Yearly events and also a list of recurrence-duration-types such as: * If an event should repeat forever * A date until which the event should repeat * Number of times an event should repeat It enables the user to create simple recurring events and even modify the recurrence-property of an already recurring event.
Created attachment 355298 [details] [review] window: pop up recurrence-dialog only when required Earlier, the recurrence-dialog would pop up even when user only wanted to change the recurrence-property of an already recurring event. This is incorrect behavior. This patch fixes this issue by making sure that recurrence-dialog doesn't pop up when the new recurrence-property chosen by the user is different from what the event already has. In this case, the user only wants to change the recurence-property of the event and there's no use of the recurrence-dialog.
Review of attachment 355184 [details] [review]: ++
Review of attachment 355295 [details] [review]: ::: src/gcal-recurrence.c @@ +105,3 @@ +{ + if (recur1 == NULL || recur2 == NULL) + return FALSE; If recur1 is NULL and recur2 is NULL, this should return TRUE. The correct check is: if (recur1 == recur2) return TRUE; else if (!recur1 || !recur2) return FALSE; @@ +115,3 @@ + if (recur1->limit_type == GCAL_RECURRENCE_UNTIL) + { + if (g_date_time_compare (recur1->limit.until, recur2->limit.until) != 0) Could use g_date_time_equal() @@ +281,3 @@ rrule->until.day = g_date_time_get_day_of_month (recur->limit.until); + rrule->until.month = g_date_time_get_month (recur->limit.until); + rrule->until.year = g_date_time_get_year (recur->limit.until); There is a problem with your rebase. This should not be part of this patch (it is already part of the previous one).
Review of attachment 355296 [details] [review]: ::: src/gcal-event.c @@ +1688,3 @@ + +void +gcal_event_remove_recurrence_properties (GcalEvent *self) This should be part of Edit dialog code. @@ +1704,3 @@ + prop = icalcomponent_get_first_property (icalcomp, ICAL_RRULE_PROPERTY); + + if (prop != NULL) if (!prop)
Review of attachment 355297 [details] [review]: Some comments below. The commit message is just great. ::: data/ui/edit-dialog.ui @@ +166,3 @@ </child> <child> + <object class="GtkLabel" id="repeat_label"> There's no need to set an id here. @@ +420,3 @@ + + + <child> Never jump 2 lines. Add a short comment before <child> with the widget below. @@ +480,3 @@ + + <child> + <object class = "GtkSpinButton" id="no_of_occurrences"> id="number_of_ocurrences_spin" @@ +487,3 @@ + + <child> + <object class = "GcalDateSelector" id="until_date"> id="until_date_selector" @@ +492,3 @@ + </child> + + <style> Don't jump line before <child> s here ::: src/gcal-edit-dialog.c @@ +83,3 @@ + GtkWidget *repeat_duration_stack; + GtkWidget *no_of_occurrences; + GtkWidget *until_date; Sort alphabetically. Add a comment before with "Recurrence widgets" @@ +148,3 @@ + gpointer user_data); + + Don't use prototypes. Put these callbacks before class_init() together with other callbacks. @@ +549,3 @@ + gtk_widget_class_bind_template_child (widget_class, GcalEditDialog, repeat_duration_stack); + gtk_widget_class_bind_template_child (widget_class, GcalEditDialog, no_of_occurrences); + gtk_widget_class_bind_template_child (widget_class, GcalEditDialog, until_date); This list of template bindings is sorted alphabetically. @@ +562,3 @@ gtk_widget_class_bind_template_callback (widget_class, sync_datetimes); + gtk_widget_class_bind_template_callback (widget_class, repeat_duration_changed); + gtk_widget_class_bind_template_callback (widget_class, repeat_type_changed); Ditto. @@ +687,3 @@ GDateTime *start_date, *end_date; + GcalRecurrenceFrequency freq; + GcalRecurrence *recur, *old_recur; Nitpick: both lines should be above "GDateTime ..." @@ +764,3 @@ + gcal_event_set_recurrence (dialog->event, recur); + gcal_edit_dialog_set_recurrence_changed (dialog, TRUE); + } "recur" is being leaked. @@ +1178,3 @@ GDateTime *date_end; + GcalRecurrence *recur; + GtkAdjustment *count_adjustment; Nitpick: both lines above "GDateTime ..." @@ +1203,3 @@ + recur = gcal_event_get_recurrence (event); + + if (recur != NULL) if (recur) @@ +1215,3 @@ + gtk_widget_show (dialog->repeat_duration_stack); + + if (recur->limit.count > 0) Better use "recur->limit_type" @@ +1218,3 @@ + gtk_spin_button_set_value (GTK_SPIN_BUTTON (dialog->no_of_occurrences), recur->limit.count); + else if (recur->limit.until != NULL) + gcal_date_selector_set_date (GCAL_DATE_SELECTOR (dialog->until_date), recur->limit.until); Wrong style. If one case needs {}s, all cases have must have {} @@ +1429,3 @@ +void +gcal_edit_dialog_set_recurrence_changed (GcalEditDialog *self, + gboolean value) You don't need this function @@ +1452,3 @@ + GcalEditDialog *self; + + self = GCAL_EDIT_DIALOG (user_data); Don't need casting. Just pass "GcalEditDialog *self" instead of "gpointer user_data". @@ +1466,3 @@ + } + + break; Don't need {} here @@ +1474,3 @@ + } + + break; Don't need {} here @@ +1489,3 @@ + GcalEditDialog *self; + + self = GCAL_EDIT_DIALOG (user_data); Don't need casting. Just pass "GcalEditDialog *self" instead of "gpointer user_data". ::: src/gcal-edit-dialog.h @@ +59,3 @@ + gboolean value); + +gboolean gcal_edit_dialog_get_recurrence_changed (GcalEditDialog *self); The entire rewrite of the style was not needed.
Review of attachment 355298 [details] [review]: ::: src/gcal-window.c @@ +1100,3 @@ + if (!gcal_edit_dialog_get_recurrence_changed (edit_dialog) && + gcal_event_has_recurrence (event)) You can just merge those checks with the already present ones.
Comment on attachment 355184 [details] [review] recurrence: fix GDateTime to icaltime conversion Attachment 355184 [details] pushed as 7ab4036 - recurrence: fix GDateTime to icaltime conversion
Created attachment 355354 [details] [review] recurrence: add a function to check equality of recurrences The gcal_recurrence_is_equal function checks if two recurrences are equal or not. When a user is trying to modify the recurrence-rules of an already recurring event, this function tells us if the recurrence-dialog should pop up or not. This patch also removes some unused code.
Created attachment 355355 [details] [review] event: add recurrence property Earlier, an event had no recurrence property. Hence, the event's recurrence pattern etc. could not be analysed, and as a result could not be changed by the user. gnome-calendar just displayed all the recurrences of a recurring event but had no 'readable' knowledge regarding the characteristics of those recurrences. This patch adds the recurrence property, which is basically a GcalRecurrence structure linked to the event.
Created attachment 355357 [details] [review] edit-dialog: add 'Repeat' options for simple recurrences Currently, creating recurring events is not supported in gnome-calendar. This patch adds a Repeat combobox with a list of simple recurrences such as: * Monday - Friday events * Daily events * Weekly events * Monthly events * Yearly events and also a list of recurrence-duration-types such as: * If an event should repeat forever * A date until which the event should repeat * Number of times an event should repeat It enables the user to create simple recurring events and even modify the recurrence-property of an already recurring event.
Created attachment 355358 [details] [review] window: pop up recurrence-dialog only when required Earlier, the recurrence-dialog would pop up even when user only wanted to change the recurrence-property of an already recurring event. This is incorrect behavior. This patch fixes this issue by making sure that recurrence-dialog doesn't pop up when the new recurrence-property chosen by the user is different from what the event already has. In this case, the user only wants to change the recurence-property of the event and there's no use of the recurrence-dialog.
Review of attachment 355354 [details] [review]: LGTM with the doc comment nit ::: src/gcal-recurrence.c @@ +93,3 @@ + * gcal_recurrence_is_equal: + * @recur1: a #GcalRecurrence + * @recur2: a #GcalRecurrence Since this function can receive NULL pointers, the doc section must have (nullable)
Review of attachment 355355 [details] [review]: Looks good with the folowing nits fixed. ::: src/gcal-event.c @@ +106,3 @@ ESource *source; + GcalRecurrence *recur; Should be renamed to recurrence. @@ +1631,3 @@ + * gcal_event_set_recurrence: + * @self: a #GcalEvent + * @recur: a #GcalRecurrence Missing the (nullable) annotation. @@ +1648,3 @@ + + rrule = g_new0 (struct icalrecurrencetype, 1); + rrule = gcal_recurrence_to_rrule (recur); You're leaking the memory returned by g_new0() (which is wrong anyway, since the only correct assignment is the to_rrule() one.) @@ +1653,3 @@ + icalcomp = e_cal_component_get_icalcomponent (comp); + + self->recur = gcal_recurrence_copy (recur); This leaks the stored GcalRecurrence
Review of attachment 355357 [details] [review]: Many improvements still. ::: src/gcal-edit-dialog.c @@ +730,3 @@ { + GcalRecurrenceFrequency freq; + GcalRecurrence *recur, *old_recur; This should be inside 'if (freq != NO_REPEAT)' @@ +808,3 @@ + gcal_event_set_recurrence (dialog->event, recur); + dialog->recurrence_changed = TRUE; + g_clear_pointer (&recur, gcal_recurrence_free); Still leaking the recurrence. @@ +1280,3 @@ + gtk_widget_hide (dialog->repeat_duration_combo); + gtk_widget_hide (dialog->repeat_duration_stack); + } There are many factorings you could've done here: - Use the enum types rather than raw unsigned integers - Use a switch instead of chained if()s - Entirely remove the if (recur), and default the frequency & limit type to NO_REPEAT and FOREVER if none is set @@ +1350,3 @@ + dialog->recurrence_changed = FALSE; + + gtk_adjustment_set_lower (gtk_spin_button_get_adjustment (GTK_SPIN_BUTTON (dialog->number_of_occurrences_spin)), 0); Should be set_value() @@ +1487,3 @@ + +static void +gcal_edit_dialog_remove_recurrence_properties (GcalEditDialog *self) This should have no gcal_edit_dialog_ prefix, and be at the top of the file ::: src/gcal-edit-dialog.h @@ +57,3 @@ +void gcal_edit_dialog_set_recurrence_changed (GcalEditDialog *self, + gboolean value); Prototype of a non-existing function
Review of attachment 355358 [details] [review]: LGTM
Created attachment 355533 [details] [review] recurrence: add a function to check equality of recurrences The gcal_recurrence_is_equal function checks if two recurrences are equal or not. When a user is trying to modify the recurrence-rules of an already recurring event, this function tells us if the recurrence-dialog should pop up or not. This patch also removes some unused code.
Created attachment 355534 [details] [review] event: add recurrence property Fixed comments.
Created attachment 355535 [details] [review] edit-dialog: add 'Repeat' options for simple recurrences Fixed comments.
Pushed. I'm leaving this bug open for future bugfixes. Attachment 355358 [details] pushed as 16bce24 - window: pop up recurrence-dialog only when required Attachment 355533 [details] pushed as 59fcd01 - recurrence: add a function to check equality of recurrences Attachment 355534 [details] pushed as 2c75e72 - event: add recurrence property Attachment 355535 [details] pushed as 067e8ae - edit-dialog: add 'Repeat' options for simple recurrences
Created attachment 355619 [details] [review] edit-dialog: remove unnecessary condition check An unnecessary condition check for assigning recurrence-changed property of edit-dialog was causing a segmentation fault. This patch removes this condition check and assigns the recurrence-changed property correctly.
Created attachment 355622 [details] [review] edit-dialog: show/hide Repeat widgets correctly Currently, the repeat widgets - repeat_duration stack and combobox are visible even when 'No Repeat' is selected in repeat combobox. This patch ensures that the repeat widgets are hidden when 'No Repeat' is selected. When not selected, repeat_duration_combo becomes visible which itself takes care of the visibilty of the stack.
Created attachment 355623 [details] [review] edit-dialog: set correct value for repeat_combo repeat_combo was assigned a value after repeat_duration combo was assigned a value, which changed the value of the repeat_duration combo due to the repeat_option_changed function. This patch fixes this issue by assigning the value to repeat_combo prior to repeat_duration_combo.
Created attachment 355625 [details] [review] edit-dialog: remove unused spin_button_set_value The number_of_occurrences was forcefully set to 0, due to spin_button_set_value being placed incorrectly. When a recurring event with limit_type as GCAL_RECURRENCE_COUNT, having some 'x' number of recurrences was opened in the edit-dialog, it showed '0' as number_of_occurrences in place of 'x', which is incorrect. This patch removes this unnecessary line of code and leads to correct assignment of number_of_occurrences.
Created attachment 355887 [details] [review] edit-dialog: hide/show repeat_limits_box Instead of hiding the individual children of the repeat_limits_box, hiding the repeat_limits_box solves alignment issues, where an undesired gap was visible below the repeat_combo upon opening the edit-dialog for the first time.
Review of attachment 355619 [details] [review]: One doubt ::: src/gcal-edit-dialog.c @@ +826,3 @@ if (!gcal_recurrence_is_equal (old_recur, recur)) + { + remove_recurrence_properties (dialog->event); So it only removes the current recurrence when changing it? What if the frequency is NO_REPEAT?
Created attachment 356307 [details] [review] edit-dialog: remove unnecessary condition check An unnecessary condition check for assigning recurrence-changed property of edit-dialog was causing a segmentation fault. This patch removes this condition check and assigns the recurrence-changed property correctly.
Attachment 355622 [details] pushed as 22fca75 - edit-dialog: show/hide Repeat widgets correctly Attachment 355623 [details] pushed as a22bec6 - edit-dialog: set correct value for repeat_combo Attachment 355625 [details] pushed as 81ce012 - edit-dialog: remove unused spin_button_set_value Attachment 355887 [details] pushed as 9a4b708 - edit-dialog: hide/show repeat_limits_box Attachment 356307 [details] pushed as 6db25ea - edit-dialog: remove unnecessary condition check
Accidentally marked it as FIXED, reopening for further bugfixing.
Created attachment 356711 [details] [review] edit-dialog: set proper limits for number of occurrences Set the lower value as 1 and upper value as G_MAXDOUBLE to make sure that any negative number, or a number greater than G_MAXDOUBLE is not accepted.
Created attachment 356712 [details] [review] edit-dialog: make number of occurrences accept only numeric input Set the numeric property of number of occurrences spin button to TRUE so that it only accepts numeric input.
Attachment 356711 [details] pushed as b7de0f3 - edit-dialog: set proper limits for number of occurrences Attachment 356712 [details] pushed as 1ad72b6 - edit-dialog: make number of occurrences accept only numeric input
*** Bug 756747 has been marked as a duplicate of this bug. ***
Created attachment 357756 [details] [review] edit-dialog: improve vertical spacing between widgets Earlier the edit-dialog had a grid row-spacing of 18. This patch changed that row-spacing to 6 and adds top-margin of 6 px above every widget group. This ensures that the related widgets are grouped together.
Created attachment 357834 [details] [review] edit-dialog: correctly assign repeat widgets' sensitivity Earlier repeat widgets were sensitive even when the event selected was read-only. This patch corrects this behavior and makes the repeat widgets insensitive when the event is read-only.
Review of attachment 357756 [details] [review]: ++
Review of attachment 357834 [details] [review]: You could've been more creative with your commit messages. All of them start with "Earlier" heh Looks good otherwise.
Attachment 357756 [details] pushed as bb58a3a - edit-dialog: improve vertical spacing between widgets Attachment 357834 [details] pushed as 29092fc - edit-dialog: correctly assign repeat widgets' sensitivity
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-calendar/issues/152.