Bug 46470

Summary: supersede all NUM_TP_foo with TP_NUM_foo
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: tp-glibAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: jonny.lamb
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
Whiteboard: review+++++++++++++++++++++++++++++++++++
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 31668    
Attachments: [master, 1/4] TP_NUM_DBUS_ERRORS, TP_NUM_CONTACT_FEATURES: add
[master, 2/4] Generate TP_NUM_foo for every generated enum
[master, 3/4] Use TP_NUM_… instead of NUM_TP_…
[master, 4/4] Rename internal NUM_TP_LIST_HANDLES to TP_NUM_LIST_HANDLES for completeness
[next, after merging] Remove all NUM_TP_…

Description Simon McVittie 2012-02-22 10:33:06 UTC
GObject-Introspection gets upset with our NUM_TP_FOO_STATES #defines, whereas it'd be perfectly happy with TP_NUM_FOO_STATES.

I think we should generate both in 0.x, and only the TP_NUM_* form in 1.0.
Comment 1 Simon McVittie 2012-02-22 10:33:50 UTC
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.
Comment 2 Simon McVittie 2012-02-22 10:34:41 UTC
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.
Comment 3 Jonny Lamb 2012-02-28 12:27:48 UTC
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
Comment 4 Simon McVittie 2012-02-29 03:34:25 UTC
(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
Comment 5 Jonny Lamb 2012-02-29 11:06:08 UTC
(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. :-)
Comment 6 Simon McVittie 2012-03-01 05:45:19 UTC
(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.
Comment 7 Jonny Lamb 2012-03-01 08:17:28 UTC
(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.
Comment 8 Simon McVittie 2012-03-02 08:22:34 UTC
review+ for your moar-deprecations branch, if you drop the TP_NUM patch.
Comment 9 Simon McVittie 2012-03-02 08:51:02 UTC
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.
Comment 10 Simon McVittie 2012-03-02 08:51:30 UTC
Created attachment 57929 [details] [review]
[master, 4/4] Rename internal NUM_TP_LIST_HANDLES to TP_NUM_LIST_HANDLES for completeness
Comment 11 Simon McVittie 2012-03-02 08:51:53 UTC
Created attachment 57930 [details] [review]
[next, after merging] Remove all NUM_TP_…
Comment 12 Jonny Lamb 2012-04-11 09:16:17 UTC
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 13 Jonny Lamb 2012-04-11 09:20:23 UTC
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 14 Jonny Lamb 2012-04-11 09:21:45 UTC
Comment on attachment 57930 [details] [review]
[next, after merging] Remove all NUM_TP_…

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

Looks nexty.
Comment 15 Simon McVittie 2012-04-12 04:27:51 UTC
Fixed in git for 0.19.0, last patch applied in next instead.

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.