After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 778544 - Adding a webcal calendar should let me specify the name and color while adding it
Adding a webcal calendar should let me specify the name and color while addin...
Status: RESOLVED OBSOLETE
Product: gnome-calendar
Classification: Applications
Component: Sources dialog
3.22.x
Other Linux
: Low enhancement
: 3.26
Assigned To: GNOME Calendar maintainers
GNOME Calendar maintainers
Depends on:
Blocks:
 
 
Reported: 2017-02-13 03:02 UTC by Jean-François Fortin Tam
Modified: 2017-11-24 22:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch (10.79 KB, patch)
2017-03-21 09:53 UTC, Kevin Lopez
none Details | Review
Patch (11.19 KB, patch)
2017-03-28 11:55 UTC, Kevin Lopez
reviewed Details | Review

Description Jean-François Fortin Tam 2017-02-13 03:02:14 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.
Comment 1 Kevin Lopez 2017-03-21 09:53:08 UTC
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!
Comment 2 Georges Basile Stavracas Neto 2017-03-21 11:18:17 UTC
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.
Comment 3 Kevin Lopez 2017-03-21 13:28:26 UTC
(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!
Comment 4 Georges Basile Stavracas Neto 2017-03-28 02:17:49 UTC
(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.
Comment 5 Kevin Lopez 2017-03-28 11:55:33 UTC
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.
Comment 6 Georges Basile Stavracas Neto 2017-04-28 14:01:31 UTC
(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.
Comment 7 Kevin Lopez 2017-05-04 01:27:56 UTC
(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!
Comment 8 Georges Basile Stavracas Neto 2017-05-04 15:29:37 UTC
Review of attachment 348882 [details] [review]:

Reviewed
Comment 9 Georges Basile Stavracas Neto 2017-11-24 22:11:10 UTC
-- 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.