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 790037 - Make telepathy dependency optional
Make telepathy dependency optional
Status: RESOLVED FIXED
Product: gnome-contacts
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: GNOME Contacts maintainer(s)
GNOME Contacts maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2017-11-07 21:41 UTC by Cosimo Cecchi
Modified: 2018-01-01 03:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Make telepathy dependency optional (11.61 KB, patch)
2017-11-07 21:41 UTC, Cosimo Cecchi
none Details | Review
Make telepathy dependency optional (11.01 KB, patch)
2017-12-31 11:36 UTC, Cosimo Cecchi
committed Details | Review

Description Cosimo Cecchi 2017-11-07 21:41:01 UTC
See attached patch.
Comment 1 Cosimo Cecchi 2017-11-07 21:41:03 UTC
Created attachment 363179 [details] [review]
Make telepathy dependency optional

Telepathy is on the way out; remove the mandatory dependency from
gnome-contacts.
Comment 2 Niels De Graef 2017-12-28 21:38:49 UTC
Review of attachment 363179 [details] [review]:

Thanks a lot for the patch! :-)
Wholeheartedly agree with making Telepathy optional.

Now, I did some cleaning up in the last few days and there are some things that changed or got removed, so would you mind doing some edits to the patch?

::: configure
@@ +16,3 @@
 DIRECTORY_OPTIONS='--@(prefix|bindir|sbindir|libexecdir|datadir|sysconfdir|libdir|mandir|includedir)'
 # Keep this in sync with meson_options.txt
+PROJECT_OPTIONS='--@(with-cheese|with-telepathy|with-manpage)'

I know this is not the case yet for the other options, but could you drop the "with-" prefix?

(I want to change it at an exact moment, so Continuous doesn't break)

::: meson_options.txt
@@ +1,2 @@
 option('with-cheese', type: 'combo', choices: ['yes', 'no', 'auto'], value: 'auto', description: 'enable cheese webcam support')
+option('with-telepathy', type: 'combo', choices: ['yes', 'no', 'auto'], value: 'auto', description: 'enable Telepathy call/chat support')

I'm hesitant about keeping the "auto" situation in Cheese actually.
In this case I'd say we drop it and set the default value on 'yes' (for now).

Also the same remark as in configure.

::: src/contacts-contact.vala
@@ +24,3 @@
+#if HAVE_TELEPATHY
+using TelepathyGLib;
+#endif

It's better to just remove this statement and to use the namespace in the code.

@@ +704,3 @@
+#else
+      if (p.store.can_remove_personas == MaybeBool.TRUE) {
+	return true;

Please only use spaces for indentation.

@@ +721,3 @@
+#else
+      if (p.store.can_remove_personas == MaybeBool.TRUE) {
+	personas.add (p);

Please only use spaces for indentation.

::: src/contacts-store.vala
@@ +24,2 @@
 using TelepathyGLib;
+#endif

It's better to just remove this statement and to use the namespace in the code.

@@ +329,3 @@
   // TODO: listen for changes in Account#URISchemes
   private async void check_call_capabilities () {
+#if HAVE_TELEPATHY

Maybe include the whole method (and when it's called a few lines above separately) in the #if-directive. That way, someone can immediately see that it's Telepathy-specific

@@ +347,3 @@
   }
 
+#if HAVE_TELEPATHY

Merge this with the previous #if-directive.
Comment 3 Cosimo Cecchi 2017-12-31 11:36:50 UTC
Created attachment 366120 [details] [review]
Make telepathy dependency optional

--

Thanks for the review, Niels. Here's an updated and rebased patch that should address your comments.
Comment 4 Niels De Graef 2017-12-31 12:01:55 UTC
Review of attachment 366120 [details] [review]:

Looks good. Thanks! :-)
Comment 5 Niels De Graef 2017-12-31 12:01:56 UTC
Review of attachment 366120 [details] [review]:

Looks good. Thanks! :-)
Comment 6 Cosimo Cecchi 2018-01-01 03:07:06 UTC
Attachment 366120 [details] pushed as edb3283 - Make telepathy dependency optional

--

Thanks, pushed to master.