GNOME Bugzilla – Bug 778544
Adding a webcal calendar should let me specify the name and color while adding it
Last modified: 2017-11-24 22:11:10 UTC
When adding a calendar from the web (ex: http://agendadulibre.qc.ca/events.ics), GNOME Calendar doesn't show a field to let me name and color the resulting calendar. Would be nice if it let me do that all in one go, instead of requiring me to then go back into the list of calendars to find the one named "Unnamed", click it, then change the name and color.
Created attachment 348390 [details] [review] Patch (In reply to Jean-François Fortin Tam from comment #0) > When adding a calendar from the web (ex: > http://agendadulibre.qc.ca/events.ics), GNOME Calendar doesn't show a field > to let me name and color the resulting calendar. Would be nice if it let me > do that all in one go, instead of requiring me to then go back into the list > of calendars to find the one named "Unnamed", click it, then change the name > and color. Hope now works ok. Regards!
Review of attachment 348390 [details] [review]: What happens when the URL has more than one Calendar? With this patch, only the last one can be edited... but is this the right approach? ::: data/ui/source-dialog.ui @@ +728,3 @@ + <property name="visible">False</property> + <property name="margin_top">10</property> + <property name="halign">GTK_ALIGN_CENTER</property> Please use 'center' (without quotes) instead of GTK_ALIGN_CENTER ::: src/gcal-source-dialog.c @@ +477,3 @@ gtk_color_chooser_get_rgba (GTK_COLOR_CHOOSER (button), &color); + /**Get in what color button is emited the signal **/ Don't use /** nor **/ - it may confuse Gtk-doc. The correct style is '/* ... */' for one line comments @@ +478,3 @@ + /**Get in what color button is emited the signal **/ + if (GTK_WIDGET (button) == self->calendar_color_button) You can change the function signature to receive a 'GtkWidget *button', and avoid this casting. @@ +487,3 @@ + ESource *remote_source; + + /**Get the last element of the list **/ Ditto about the comment style. @@ +488,3 @@ + + /**Get the last element of the list **/ + remote_source = (g_list_last (self->remote_sources))->data; I prefer to use another variable here, instead of (...)->data @@ +491,3 @@ + + extension = E_SOURCE_SELECTABLE (e_source_get_extension (remote_source, E_SOURCE_EXTENSION_CALENDAR)); + e_source_selectable_set_color (extension, gdk_rgba_to_string (&color)); You can factor these two lines out of the if. Put an 'ESource *source' var outside the if()s, and inside them you only select which source it is. @@ +696,3 @@ + const gchar *widget_id; + + widget_id = gtk_buildable_get_name (GTK_BUILDABLE (object)); Unused variable. @@ +701,1 @@ + /**Get in what entry was emited the signal **/ Ditto about the style. @@ +701,2 @@ + /**Get in what entry was emited the signal **/ + if (GTK_WIDGET (object) == self->name_entry) Ditto about function signature. @@ +716,3 @@ + ESource *remote_source; + + /**Get the last element of the list **/ Style. @@ +717,3 @@ + + /**Get the last element of the list **/ + remote_source = (g_list_last (self->remote_sources))->data; Use another var. @@ +1122,3 @@ text = gtk_entry_get_text (GTK_ENTRY (self->calendar_address_entry)); + /**On any text changed we hide the name and color options **/ Style. @@ +1356,3 @@ + gtk_widget_set_sensitive (dialog->add_button, TRUE); + + /**Only show when is an absolute file path **/ Style. @@ +1366,3 @@ + { + gtk_widget_set_sensitive (dialog->add_button, FALSE); + } To avoid branching the code, I prefer to use "gtk_widget_set_visible (..., source != NULL);" and "gtk_widget_set_sensitive (..., source != NULL);" @@ +1958,3 @@ + gtk_widget_class_bind_template_child (widget_class, GcalSourceDialog, web_calendar_options_label); + gtk_widget_class_bind_template_child (widget_class, GcalSourceDialog, web_color_dim_label); + gtk_widget_class_bind_template_child (widget_class, GcalSourceDialog, web_calendar_color_button); You should put this at the right position. This list is alphabetically sorted. @@ +1977,3 @@ gtk_widget_class_bind_template_child (widget_class, GcalSourceDialog, name_entry); + gtk_widget_class_bind_template_child (widget_class, GcalSourceDialog, web_calendar_dim_label); + gtk_widget_class_bind_template_child (widget_class, GcalSourceDialog, web_name_entry); Ditto.
(In reply to Georges Basile Stavracas Neto from comment #2) Thanks for the fast review! > Review of attachment 348390 [details] [review] [review]: > > What happens when the URL has more than one Calendar? With this patch, only > the last one can be edited... but is this the right approach? > When there is more than one calendar it doesn't show the additional options. This is why the calls to "gtk_widget_show (now gtk_widget_set_visible)" are inside the if condition that check if the file path has as prefix ".ics", because as far as I know (not trying to be rude ) ics calendars don't allow have more than one calendar. Anyway can yo provide me an url that has more than one calendar to test, all my calendars are in ics format. Thanks. Regards!
(In reply to Kevin Lopez from comment #3) > When there is more than one calendar it doesn't show the additional options. > This is why the calls to "gtk_widget_show (now gtk_widget_set_visible)" are > inside the if condition that check if the file path has as prefix ".ics", > because as far as I know (not trying to be rude ) ics calendars don't allow > have more than one calendar. You're right. ICS only allows a single calendar per file. > Anyway can yo provide me an url that has more than one calendar to test, all > my calendars are in ics format. > Thanks. I usually test with my Google / ownCloud accounts. You can try that if you have either of those.
Created attachment 348882 [details] [review] Patch (In reply to Georges Basile Stavracas Neto from comment #4) > You're right. ICS only allows a single calendar per file. Currently I don't have access to a Url with more than one calendar, so I uploaded the patch only for Ics calendars. And when I will have access to more calendars I will try to code the other patch.
(In reply to Kevin Lopez from comment #5) > Currently I don't have access to a Url with more than one calendar, so I > uploaded the patch only for Ics calendars. > > And when I will have access to more calendars I will try to code the other > patch. You can use your Google or ownCloud accounts for that (the Web entry will ask you for your username and password). I think your approach if wrong in this bug. When the user puts a like with 2+ calendars, we show a small listbox with the calendars. I think you should put entries and color pickes in that listbox instead. Otherwise, when there's just 1 calendar in the Web link, it should switch to the standard calendar editor page.
(In reply to Georges Basile Stavracas Neto from comment #6) > (In reply to Kevin Lopez from comment #5) > > > Currently I don't have access to a Url with more than one calendar, so I > > uploaded the patch only for Ics calendars. > > > > And when I will have access to more calendars I will try to code the other > > patch. > > You can use your Google or ownCloud accounts for that (the Web entry will > ask you for your username and password). > > I think your approach if wrong in this bug. When the user puts a like with > 2+ calendars, we show a small listbox with the calendars. I think you should > put entries and color pickes in that listbox instead. > > Otherwise, when there's just 1 calendar in the Web link, it should switch to > the standard calendar editor page. You are correct. I will work on that. Regards!
Review of attachment 348882 [details] [review]: Reviewed
-- 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/112.