Bug 31668 (tp-glib-1.0)

Summary: break C API to drop deprecated stuff
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: tp-glibAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED MOVED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: bpepple, xclaesse
Version: git master   
Hardware: Other   
OS: All   
URL: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=next-remove-deprecated
Whiteboard:
i915 platform: i915 features:
Bug Depends on: 17112, 17115, 21795, 21796, 22207, 23148, 27687, 33410, 39189, 39191, 39248, 45842, 46358, 46470, 46835, 46978, 47040, 47100, 47322, 47942, 48230, 48760, 49213, 49215, 49338, 49366, 49370, 49371, 49372, 49373, 49375, 49384, 49477, 49593, 49594, 49596, 49648, 49899, 50689, 50834, 50886, 51441, 54321, 59024, 69773, 75204, 76462    
Bug Blocks: 68660    
Attachments: Excise Tubes from channel.xml to fix build
Remove deprecated, unused debug-ansi.h
Remove deprecated tp_message_new()

Description Simon McVittie 2010-11-16 09:34:52 UTC
When we break D-Bus API we should also break C API:

- remove tp_get_bus (Bug #18832)

- don't emit TpProxy::invalidated in dispose (Bug #22119)

- remove all _run_ methods (Bug #22207)

- don't allow get_contact_statuses to fail (Bug #21796)

- get rid of everything deprecated

- replace TpChannelManagerIface.foreach_channel_class with a renamed
  TpChannelManagerIface.type_foreach_channel_class

- replace the two original TpClientChannelFactory methods with a renamed obj_*

- return void from tp_proxy_add_interface_by_id (Bug #21795)

Other possible things:

- get TpContact objects in a more GIO-ful way (Bug #27687), although this might be superseded by fixing handles

- make all async functions not re-enter (using an idle if necessary)

- check everything for introspectability and fix troublesome APIs as much as possible

-------------------------

Possibly to do, or not, depending how long the above take:

- cut as much as possible of the dbus-glib out of our API/ABI

- move to GDBus, which is probably also an ABI break, but might have to be delayed if the above take too long (Bug #28782)
Comment 1 Simon McVittie 2010-11-25 03:55:59 UTC
Additional minor things from FIXME.out:

* change tp_error_quark()'s quark to tp-error-quark

* change a GValue* in the middle of a struct to a GValue in properties-mixin.h (or just delete the entire properties mixin)
Comment 2 Simon McVittie 2010-12-16 04:13:48 UTC
Also move ChatStates support from TpChannel to TpTextChannel, following tp-qt4's lead.
Comment 3 Simon McVittie 2010-12-17 05:06:16 UTC
13:04 < cassidy> tomeu, smcv: do you know about this kind of warnings : 
                 _gen/telepathy-enums.h:26: Warning: TelepathyGLib: 
                 symbol='NUM_TP_HANDLE_TYPES': Unknown namespace for symbol 
                 'NUM_TP_HANDLE_TYPES' 
13:05 < smcv> cassidy: it wanted us to use TP_NUM_HANDLE_TYPES or some such
13:05 < smcv> cassidy: tbh just ignore it, and make sure we use the latter in 
              the 1.0 codegen
Comment 4 Will Thompson 2011-01-07 08:41:36 UTC
*** Bug 17646 has been marked as a duplicate of this bug. ***
Comment 5 Simon McVittie 2012-01-31 12:25:11 UTC
We now have a 'next' branch where we're doing this. \o/

Some things that need changing in 'next':

* --generate-all-reentrants should be --separate-reentrants or something

* Version should be < 1.0 at first (0.99.n where n is a snapshot number incremented when it's halfway stable for a bit? 0.2012.n because this is the end of the world? ...) and become 1.0 when the core library reaches API/ABI stability

* Commented-out Call stuff would be better #ifdef'd?

* "Presence Presence interface"

* tp_message_to_text should still return whether there's non-text content
Comment 6 Simon McVittie 2012-01-31 12:31:14 UTC
Created attachment 56410 [details] [review]
Excise Tubes from channel.xml to fix build
Comment 7 Simon McVittie 2012-01-31 12:31:35 UTC
Created attachment 56411 [details] [review]
Remove deprecated, unused debug-ansi.h
Comment 8 Simon McVittie 2012-01-31 12:32:01 UTC
Created attachment 56412 [details] [review]
Remove deprecated tp_message_new()
Comment 9 Jonny Lamb 2012-01-31 12:38:15 UTC
(In reply to comment #6)
> Created attachment 56410 [details] [review] [review]
> Excise Tubes from channel.xml to fix build

I've already pushed a fix for this, admittedly as a fixup! commit and then forgot to autosquash. Oh well.

(In reply to comment #7)
> Created attachment 56411 [details] [review] [review]
> Remove deprecated, unused debug-ansi.h

Haven't you forgotten to actually remove debug-ansi.h in this commit?

(In reply to comment #8)
> Created attachment 56412 [details] [review] [review]
> Remove deprecated tp_message_new()

You forgot to remove tp_message_new from telepathy-glib-sections.txt.
Comment 10 Jonny Lamb 2012-01-31 13:17:12 UTC
(In reply to comment #9)
> (In reply to comment #7)
> > Created attachment 56411 [details] [review] [review] [review]
> > Remove deprecated, unused debug-ansi.h
> 
> Haven't you forgotten to actually remove debug-ansi.h in this commit?

And you forgot to remove all the debug-ansi stuff from the docs (at least -docs.sgml).
Comment 11 Jonny Lamb 2012-01-31 14:10:18 UTC
http://cgit.freedesktop.org/~jonny/telepathy-glib/log/?h=remove-deprecations

Here is some more stuff that's been deprecated to remove.
Comment 12 Jonny Lamb 2012-01-31 14:48:48 UTC
... and some more fixes:

(In reply to comment #5)
> * Version should be < 1.0 at first (0.99.n where n is a snapshot number
> incremented when it's halfway stable for a bit? 0.2012.n because this is the
> end of the world? ...) and become 1.0 when the core library reaches API/ABI
> stability

I've kept this at 0.99.0 for now.

> * "Presence Presence interface"

Fixed.

> * tp_message_to_text should still return whether there's non-text content

What would you prefer?

    gboolean tp_message_to_text (TpMessage *message, gchar **text);

or

    gchar * tp_message_to_text (TpMessage *message, gboolean *non_text_content);

where the former returns TRUE if there *is* text content (i.e. @text is set), and the latter returns NULL and sets @non_text_content to TRUE if there *isn't* text content... I think the former is nicer.

Also:

> * Commented-out Call stuff would be better #ifdef'd?

#ifdef'd on what?
Comment 13 Simon McVittie 2012-02-01 03:59:15 UTC
(In reply to comment #11)
> http://cgit.freedesktop.org/~jonny/telepathy-glib/log/?h=remove-deprecations
> 
> Here is some more stuff that's been deprecated to remove.

This branch looks ideal, please merge.

(In reply to comment #12)
> > * tp_message_to_text should still return whether there's non-text content
> [...] returns TRUE if there *is* text content (i.e. @text is set),
> and the latter returns NULL and sets @non_text_content to TRUE if there *isn't*
> text content... I think the former is nicer.

There's a subtle distinction between what I'm asking for and what you seem to be suggesting: the point of the NON_TEXT_CONTENT flag was not "there is no text here", it was "by condensing this Message into plain-text, you are losing something".

Consider this composite message:

    text/plain: "hey look at this funny picture lol"
    image/jpeg: ...

This would "text-ize" as:

    ("hey look at this funny picture lol", NON_TEXT_CONTENT)

which a text-only UI could display as something like

    hey look at this funny picture lol
    [Non-text content not displayed, sorry!]

Meanwhile, this message (which could be produced automatically by a sufficiently clever CM receiving only the HTML version):

    text/html: "look at this <a href="http://...">website</a>", alternative for AAAA
    text/plain: "look at this website <http://...>", alternative for AAAA

would "text-ize" as:

    ("look at this website <http://...>", 0)

which a text-only UI would display without the apology.

I'm not sure how useful this would really be in practice, though...

> > * Commented-out Call stuff would be better #ifdef'd?
> 
> #ifdef'd on what?

I don't know, #ifdef FIXME_REINSTATE_WHEN_CALL_IS_STABLE or something. Or you could leave them commented-out (or #if 0) and have an additional comment that's something distinctive we can grep for, or references a new bug that says "reinstate Call-related things in tp-glib when it's stable", or something.

I do agree with you that it's a good idea not to follow the usual "don't comment out code" rule in this case.
Comment 14 Simon McVittie 2012-02-01 10:48:51 UTC
http://cgit.freedesktop.org/~smcv/telepathy-glib/log?h=easy-deprecations supersedes the patches here (or it would if cgit was up).

ssh://people.freedesktop.org/~smcv/telepathy-glib easy-deprecations
Comment 15 Simon McVittie 2012-02-01 10:49:23 UTC
*** Bug 18832 has been marked as a duplicate of this bug. ***
Comment 16 Jonny Lamb 2012-02-02 12:16:17 UTC
(In reply to comment #14)
> http://cgit.freedesktop.org/~smcv/telepathy-glib/log?h=easy-deprecations
> supersedes the patches here (or it would if cgit was up).
> 
> ssh://people.freedesktop.org/~smcv/telepathy-glib easy-deprecations

This all looks good.

I wonder if we should make TpBaseChannel check there are no duplications after calling get_interfaces?
Comment 17 Jonny Lamb 2012-02-02 14:45:24 UTC
http://cgit.freedesktop.org/~jonny/telepathy-glib/log/?h=moar-deprecations

This:

 · #ifdefs out the Call stuff. I reckon we should start generating Call code
   soon.

 · replaces NUM_TP_… defines with TP_NUM_…

 · moves chat states to TpTextChannel and adds a chat states test.
Comment 18 Simon McVittie 2012-03-02 08:59:44 UTC
(In reply to comment #17)
> http://cgit.freedesktop.org/~jonny/telepathy-glib/log/?h=moar-deprecations
> 
> This:
> 
>  · #ifdefs out the Call stuff. I reckon we should start generating Call code
>    soon.
> 
>  · moves chat states to TpTextChannel and adds a chat states test.

I reviewed these on Bug #46470 - sorry, missed them here.

>  · replaces NUM_TP_… defines with TP_NUM_…

Superseded by Bug #46470 which adds the new version on master, then deletes the old on next. I think that's generally a good thing to do, for things that won't cause massive bloat/effort?
Comment 19 Simon McVittie 2012-03-02 09:50:51 UTC
(In reply to comment #16)
> > ssh://people.freedesktop.org/~smcv/telepathy-glib easy-deprecations
> 
> This all looks good.

I merged everything except the last commit, which needs updating (there's now one more example). I'll shunt those onto a new bug I think.

> I wonder if we should make TpBaseChannel check there are no duplications after
> calling get_interfaces?

That might not be a bad idea. Having said that, it'd be O(n**2) at the moment... so perhaps not worth it.
Comment 20 Xavier Claessens 2012-05-02 15:38:58 UTC
I've searched all open bug to see all those requesting API breaks and have set them as blocking this bug. I've also reported all things I could think. Now I'm pretty confident the "Depends on" list is complete... scary how long it is...
Comment 21 Jonny Lamb 2012-07-12 14:05:23 UTC
Xavier merged master into next and removed some deprecations here:

http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=next-remove-deprecated

Some comments:

02050c89a381c9e9e:

You could use tp_asv_new (NULL, NULL) instead of g_hash_table_new_full (blah blah blah).

cdb02889684c082:

+ test->sms_channel = TP_TEXT_CHANNEL (tp_client_factory_ensure_channel (
+     factory, test->connection, chan_path, props, &test->error));
  g_assert_no_error (test->error);
+ g_assert (TP_IS_TEXT_CHANNEL (test->channel));

The second assertion should be test->sms_channel, no? It's also redundant (there's another too) as the TP_TEXT_CHANNEL cast will typecheck, no?

Otherwise, seems good.
Comment 22 Xavier Claessens 2012-08-31 14:26:18 UTC
Branch updated with more fixes.
Comment 23 Jonny Lamb 2012-09-07 16:44:30 UTC
(In reply to comment #22)
> Branch updated with more fixes.

Yes!
Comment 24 Xavier Claessens 2012-09-07 17:06:32 UTC
Merci !
Comment 25 Xavier Claessens 2012-09-07 18:27:10 UTC
Yet another branch to review: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=next-update-spec
Comment 26 Guillaume Desmottes 2012-09-13 08:51:15 UTC
http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/commit/?h=next-update-spec&id=b78224495b798464759f1c7ae8bf4372dc0415b5

+  self->priv->alias_flags = tp_asv_get_uint32 (properties, "AliasFlags", NULL);
You should check valid and raise an error if !valid.

Why are you changing the presence mixin in this commit?


http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/commit/?h=next-update-spec&id=1dcdb3b38c99257a581c13861720770f5b4b38b4
looks good
Comment 27 Xavier Claessens 2012-09-13 09:02:18 UTC
(In reply to comment #26)
> http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/commit/?h=next-update-spec&id=b78224495b798464759f1c7ae8bf4372dc0415b5
> 
> +  self->priv->alias_flags = tp_asv_get_uint32 (properties, "AliasFlags",
> NULL);
> You should check valid and raise an error if !valid.

done

> Why are you changing the presence mixin in this commit?

my mistake, splitted the commit

Branch merged.
Comment 28 Xavier Claessens 2012-09-15 13:58:41 UTC
Remove deprecated stuff, again... http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=next-remove-deprecated
Comment 29 Guillaume Desmottes 2014-05-07 13:19:15 UTC
(In reply to comment #28)
> Remove deprecated stuff, again...
> http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=next-
> remove-deprecated

This branch doesn't exist; I guess it has been merged.
Comment 30 Daniel Stone 2019-12-03 20:06:02 UTC
This issue has been moved to https://gitlab.freedesktop.org/telepathy/telepathy-glib/issues/48 in GitLab; please submit any further comments there.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.