Add register-name-on-bus property

[TingPing]

This is useful for running in a sandbox where you do not have bus name ownership permissions.

Closes #18

[jjk-jacky]

Looks good; I do have a few comments though :

• I don't think we need G_PARAM_CONSTRUCT nor G_PARAM_STATIC_STRINGS

• like all other properties it should have a getter status_notifier_item_get_register_name_on_bus() for consistency

• having -1 to try to automatically do the right thing is nice, but since what it does is simple enough I'd document it (i.e. mention it checks for existence of /.flatpak-info and if so disables name registration), I feel it's valuable information to have/give

• the property should be marked "Since: 1.1.0"

• there's no need for a static variable is_flatpak in should_register_name(): if priv->register_bus_name is -1 then simply test for the file, and set it to 0 or 1 accordingly. (Bonus: once registered, one can get the property value to know what has actually been done, should one care about that...)

Thanks!

[TingPing]

G_PARAM_STATIC_STRINGS

You should always use that, even on your previous ones. It is just an optimization.

G_PARAM_CONSTRUCT

It means it gets automatically set on construct, sounds like what we want.

status_notifier_item_get_register_name_on_bus()

:+1:

but since what it does is simple enough I'd document it

:+1:

the property should be marked "Since: 1.1.0"

:+1:

there's no need for a static variable is_flatpak in should_register_name(): if priv->register_bus_name is -1 then simply test for the file, and set it to 0 or 1 accordingly. (Bonus: once registered, one can get the property value to know what has actually been done, should one care about that...)

Hmm, It is kinda evil to change a properties value automatically I feel but sure I like it.

[jjk-jacky]

G_PARAM_STATIC_STRINGS

You should always use that, even on your previous ones. It is just an optimization.

hmm okay then, but still: don't include it here and have a separate patch to add it everywhere.

G_PARAM_CONSTRUCT

It means it gets automatically set on construct, sounds like what we want.

Sure, but that's also/already done via G_PARAM_CONSTRUCT_ONLY, hence we only need that one.

Hmm, It is kinda evil to change a properties value automatically I feel but sure I like it.

Usually I'd agree, but here we're moving from a "determine automatically" value to an actual one, that once set isn't meant/cannot change, so I'm okay with it. (Might be worth mentionning on the doc though.)

Cheers,

[TingPing]

Sure, but that's also/already done via G_PARAM_CONSTRUCT_ONLY, hence we only need that one.

In testing I can confirm this is untrue.

All other comments have been resolved.

[jjk-jacky]

Sure, but that's also/already done via G_PARAM_CONSTRUCT_ONLY, hence we
only need that one.

In testing I can confirm this is untrue.

Not sure what you mean here. For one, if I try building with both set, I can see an error:

(process:29565): GLib-GObject-CRITICAL **: 19:13:06.110: validate_pspec_to_install: assertion '(pspec->flags & G_PARAM_CONSTRUCT_ONLY) == 0' failed

Because both aren't meant to be set at once, it should be either one or the other. (This assertion is from gobject, and makes sure that G_PARAM_CONSTRUCT_ONLY is not set if G_PARAM_CONSTRUCT is.)

And running the example does show the same warning, then things don't work as expected.

OTOH only setting G_PARAM_CONSTRUCT_ONLY and building/running the example, no more warnings and I can see the property gets set to -1 as expected.

[TingPing]

You are totally right, my error was an unrelated one. Fixed.

[jjk-jacky]

Alright, applied the patch (I did add a little mention of this on the doc for ...._register() and edited the commit message somewhat.) and just pushed to next.

Thanks!