Bug 46470

Created attachment 57472 [details] [review]
[master, 1/4] TP_NUM_DBUS_ERRORS, TP_NUM_CONTACT_FEATURES: add

NUM_TP_CONTACT_FEATURES and NUM_TP_DBUS_ERRORS aren't introspectable.

Created attachment 57473 [details] [review]
[master, 2/4] Generate TP_NUM_foo for every generated enum

Unlike NUM_TP_foo, these are introspectable.

---

Not done yet: the patch to stop generating NUM_* in next. I'll do that when this has been merged to master.

Haven't I already half done this?

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

I'm not sure if you already reviewed this

(In reply to comment #3)
> Haven't I already half done this?
> 
> http://cgit.freedesktop.org/~jonny/telepathy-glib/log/?h=moar-deprecations
> 
> I'm not sure if you already reviewed this

Sorry, I must have missed that. Does it have a bug#?

Regardless:

"examples: #ifdef out Call stuff" ++

"c-constants-gen: use TP_NUM_… instead of NUM_TP_…" (which is also this bug):

The branch I prepared for this bug has both sorts of constant on master, so people can start porting early. I'd planned to delete the NUM_TP_ versions from next later.

Your patch looks fine for next. I think the ideal thing would be:

- apply my "generate both" (this bug) to master
- split out the parts of your patch that move the uses of these constants
  from NUM_TP_ to TP_NUM_, and apply those to master too
- maybe deprecate NUM_TP_ too
- merge master into next (not at all trivial - I have a branch with a
  monstrously huge merge)
- delete NUM_TP_ on next

but if you can't be bothered with all that, applying your version to next works too.

"text-channel: move chat state from channel to here" ++ with trivia:

> +   * has finished preparing the feature %TP_CHANNEL_FEATURE_CHAT_STATES.

That should say TP_TEXT_CHANNEL...

"text-channel test: add test for chat state stuff" ++ with optional trivia:

> +  tp_proxy_prepare_async (test->channel, features,
> +      proxy_prepare_cb, test);
> +
> +  g_main_loop_run (test->mainloop);
> +  g_assert_no_error (test->error);

In new code I'd prefer tp_tests_proxy_run_until_prepared(), which does all this in one line

(In reply to comment #4)
> "c-constants-gen: use TP_NUM_… instead of NUM_TP_…" (which is also this bug):
> 
> The branch I prepared for this bug has both sorts of constant on master, so
> people can start porting early. I'd planned to delete the NUM_TP_ versions from
> next later.
> 
> Your patch looks fine for next. I think the ideal thing would be:
> 
> - apply my "generate both" (this bug) to master
> - split out the parts of your patch that move the uses of these constants
>   from NUM_TP_ to TP_NUM_, and apply those to master too
> - maybe deprecate NUM_TP_ too
> - merge master into next (not at all trivial - I have a branch with a
>   monstrously huge merge)
> - delete NUM_TP_ on next
> 
> but if you can't be bothered with all that, applying your version to next works
> too.

Hmm. I'm not sure what the conclusion is. I'm happy to remove my NUM_TP/TP_NUM patch and apply my branch as is and let you fiddle with the details though? :-)

> > +   * has finished preparing the feature %TP_CHANNEL_FEATURE_CHAT_STATES.
> 
> That should say TP_TEXT_CHANNEL...

Yes, good point, updated.

> > +  tp_proxy_prepare_async (test->channel, features,
> > +      proxy_prepare_cb, test);
> > +
> > +  g_main_loop_run (test->mainloop);
> > +  g_assert_no_error (test->error);
> 
> In new code I'd prefer tp_tests_proxy_run_until_prepared(), which does all this
> in one line

Fair, done, and I updated a couple of the older tests in that file too.

I appended new commits to the end instead of amending existing commits to make things easier for you and me. :-)

(In reply to comment #5)
> Hmm. I'm not sure what the conclusion is. I'm happy to remove my NUM_TP/TP_NUM
> patch and apply my branch as is and let you fiddle with the details though?

Yes, after I've finished reviewing the rest of moar-deprecations, please do that (omit the NUM_TP part of moar-deprecations, apply the rest). I'll recycle parts of your NUM_TP patch to turn it into what I suggested.

> I appended new commits to the end instead of amending existing commits to make
> things easier for you and me. :-)

Thanks, but I don't see them in cgit or your repository? Let me know when you've pushed them somewhere, and I'll re-review.

If you rebase to get rid of the NUM_TP patch, please let me know how far along the branch I've already seen; or you could leave it in during review, and get rid of it just before applying. Either's good.

(In reply to comment #6)
> Yes, after I've finished reviewing the rest of moar-deprecations, please do
> that (omit the NUM_TP part of moar-deprecations, apply the rest). I'll recycle
> parts of your NUM_TP patch to turn it into what I suggested.

Okay cool.

> Thanks, but I don't see them in cgit or your repository? Let me know when
> you've pushed them somewhere, and I'll re-review.

Oops I forgot to push. Done now.

review+ for your moar-deprecations branch, if you drop the TP_NUM patch.

Created attachment 57928 [details] [review]
[master, 3/4] Use TP_NUM_… instead of NUM_TP_…

Based on a patch from Jonny Lamb, updated for current master.

Created attachment 57929 [details] [review]
[master, 4/4] Rename internal NUM_TP_LIST_HANDLES to TP_NUM_LIST_HANDLES for completeness

Created attachment 57930 [details] [review]
[next, after merging] Remove all NUM_TP_…

Comment on attachment 57928 [details] [review]
[master, 3/4] Use TP_NUM_… instead of NUM_TP_…

Review of attachment 57928 [details] [review]:
-----------------------------------------------------------------

I approve.

Comment on attachment 57929 [details] [review]
[master, 4/4] Rename internal NUM_TP_LIST_HANDLES to TP_NUM_LIST_HANDLES for completeness

Review of attachment 57929 [details] [review]:
-----------------------------------------------------------------

Oui.

Comment on attachment 57930 [details] [review]
[next, after merging] Remove all NUM_TP_…

Review of attachment 57930 [details] [review]:
-----------------------------------------------------------------

Looks nexty.

Fixed in git for 0.19.0, last patch applied in next instead.