Bug 31668 (tp-glib-1.0)

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)

Also move ChatStates support from TpChannel to TpTextChannel, following tp-qt4's lead.

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

*** Bug 17646 has been marked as a duplicate of this bug. ***

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

Created attachment 56410 [details] [review]
Excise Tubes from channel.xml to fix build

Created attachment 56411 [details] [review]
Remove deprecated, unused debug-ansi.h

Created attachment 56412 [details] [review]
Remove deprecated tp_message_new()

(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.

(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).

http://cgit.freedesktop.org/~jonny/telepathy-glib/log/?h=remove-deprecations

Here is some more stuff that's been deprecated to remove.

... 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?

(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.

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

*** Bug 18832 has been marked as a duplicate of this bug. ***

(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?

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.

(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?

(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.

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...

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.

Branch updated with more fixes.

(In reply to comment #22)
> Branch updated with more fixes.

Yes!

Merci !

Yet another branch to review: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=next-update-spec

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

(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.

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

(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.

This issue has been moved to https://gitlab.freedesktop.org/telepathy/telepathy-glib/issues/48 in GitLab; please submit any further comments there.