Bug 51187

(In reply to comment #0)
> Created attachment 63152 [details]
> introspection: add many generic type arguments, change prefix to cd

Applied, thanks.

> The other issue I noticed (but did not address in the patch) is that many
> symbols are not named what GObject Introspection expects them to be so you end
> up with a lot of functions in the global namespace instead of methods (e.g.,
> "Cd.color_clear_xyz" instead of "Cd.ColorXYZ.clear").

Yes, that's a mistake on my part. We should probably rename them and then just have stubs for the old methods so we don't break ABI until the next API break.

> of colord.vapi and you'll see what I'm talking about.  AFAIK there is no way to
> resolve this using annotations alone so the only way would be creating the
> symbols G-I expects.  If you want to change it let me know and I'll put
> together a patch.

Yes please; also, I'd appreciate a Vala demo in examples/ -- it doesn't have to be wired up to automake, if that adds deps to configure. Thanks dude.

Richard.

(In reply to comment #1)
> Yes, that's a mistake on my part. We should probably rename them and then just
> have stubs for the old methods so we don't break ABI until the next API break.

Cancel that. If we just fix the names now I can break bump the soname and add some #defines. If you send me a patch to fix things up, I'll make it build :)

Richard.

Created attachment 63185 [details]
Deprecate cd_color_xyz_set in favor of cd_color_set_xyz

(In reply to comment #2)
> (In reply to comment #1)
> > Yes, that's a mistake on my part. We should probably rename them and then just
> > have stubs for the old methods so we don't break ABI until the next API break.
> 
> Cancel that. If we just fix the names now I can break bump the soname and add
> some #defines. If you send me a patch to fix things up, I'll make it build :)

That's fine with me but it would break the current GObject Introspection API (introspection doesn't handle preprocessor macros very well)...  Are you okay with that?

FWIW I actually started working on this before your second comment, I'm attaching the beginning of what I was planning on doing.  It doesn't really matter to me whether I continue with this or switch to #define.

(In reply to comment #1)
> Yes please; also, I'd appreciate a Vala demo in examples/ -- it doesn't have to
> be wired up to automake, if that adds deps to configure. Thanks dude.

Sure.  I'll open up another bug for improving Vala support later (I just noticed that there is no VAPI for colord-gtk) and put that there.

(In reply to comment #3)
> Created attachment 63185 [details]
> Deprecate cd_color_xyz_set in favor of cd_color_set_xyz

I was thinking about this more today. Basically, the object we're manipulating is a CdColorXYZ, not a CdColor abstract object. So cd_color_xyz_set makes sense as the cd_color_xyz is the root of the object, and 'set' is the action.

I wonder if splitting the objects up into cd-color-xyz.h might be enough to convince the scanner to do the right thing? It might get a bit messy with cd_color_convert_xyz_to_yxy and cd_color_convert_yxy_to_xyz being in different files (circular defs).

Richard.

> I was thinking about this more today. Basically, the object we're
> manipulating is a CdColorXYZ, not a CdColor abstract object. So
> cd_color_xyz_set makes sense as the cd_color_xyz is the root of the object,
> and 'set' is the action.

So cd_color_set_xyz should be moved to cd_color_xyz_set.  That's what the attached patch does, albeit just for that one function.  If you're okay with the basic approach I can throw together a patch which does the same thing for the rest of the functions.

I just wasn't sure whether you want wrapper functions (like this patch does) or just want to use the preprocessor (like you suggested in #c2).

> I wonder if splitting the objects up into cd-color-xyz.h might be enough to
> convince the scanner to do the right thing?

I'm fairly certain that wouldn't make any difference.

> It might get a bit messy with
> cd_color_convert_xyz_to_yxy and cd_color_convert_yxy_to_xyz being in
> different files (circular defs).

cd_color_xyz_convert_to_yxy (or cd_color_xyz_to_yxy) would be the more standard name for that function.  cd_color_convert_xyz_to_yxy will create functions, not methods, so you have to do "yxy = Cd.color_convert_xyz_to_yxy (xyz)" instead of "yxy = xyz.convert_to_yxy ()".

(In reply to comment #5)
> So cd_color_set_xyz should be moved to cd_color_xyz_set.  That's what the
> attached patch does, albeit just for that one function.  If you're okay with
> the basic approach I can throw together a patch which does the same thing
> for the rest of the functions.

Sorry, I was tying myself in knots there. cd_color_xyz_set indeed makes sense.

> I just wasn't sure whether you want wrapper functions (like this patch does)
> or just want to use the preprocessor (like you suggested in #c2).

I'm okay with either. If we add #ifdefs then that's an ABI break, and I'll have wait for me to branch (and it'll only go in F19, not F18 and before). If you do the cd-deprecated thing then I'm happy to merge it to master and include it as there's no ABI break, just added symbols.

> I'm fairly certain that wouldn't make any difference.

Right.

> cd_color_xyz_convert_to_yxy (or cd_color_xyz_to_yxy) would be the more
> standard name for that function.

Yes, either of those is fine. Is cd_color_xyz_to_yxy more conventional than cd_color_xyz_convert_to_yxy? If you make all these changes with the cd-deprecated abi-fixer then I'm happy for you to make those changes into master without running the patches past me.  I just need to know your gitorious username.

Thanks, I really appreciate this.

I've committed this:

commit b45001d8ea25da93ef1d885d31cd3e8931d81869
Author: Richard Hughes <richard@hughsie.com>
Date:   Thu Jan 3 21:35:38 2013 +0000

    Ensure the color types are methods in GObject Introspection rather than functions
    
    Many symbols are not named what GObject Introspection expects them to be so
    we end up with a lot of functions in the global namespace instead of methods
    e.g. "Cd.color_clear_xyz" instead of "Cd.ColorXYZ.clear".
    
    There is no way to resolve this using annotations alone so the only way is to
    create the symbols G-I expects. Ship a cd-deprecated.h header so we do not
    break API or ABI. We'll remove this whenever we bump the soname next.
    
    Based on a patch by Evan Nemerson <evan@coeus-group.com>, many thanks.

I'd very much appreciate a sanity check of what I've done. Thanks.