Created attachment 270282 [details] [review]
patch

(only lightly tested)

Review of attachment 270282 [details] [review]:

I don't like how this patch is introducing a use of command_line.  It's always better to avoid this, if possible.

I think it would be far better to try to keep the approach of the old patch: sending action activations to the primary instance.  This could just be done from handle_options instead of local_command_line (and the support for "--gapplication-service" could be dropped).

Created attachment 270574 [details] [review]
patch

I actually updated the patch as per Ryan comments and I am attaching it here for reference, marking it as "needs-work".

The new patch does actually not work: when debugging a little bit it seems that the command line is properly parsed and the dbus messages exchanged as expected, but when the actions are activated nothing happens.

Ryan gave a quick glance at the patch and at a first look he said it looks correct, so maybe something is wrong elsewhere in how gactions are set up, but I did not have time to look further

Paolo, is this still required? Can you outline what the benefits are compared to what we do now please?

(In reply to comment #4)
> Paolo, is this still required? Can you outline what the benefits are compared
> to what we do now please?

Right now gs manages --gapplication-service manually, since it was developed before the feature was merged into GApplication itself. Beside it also uses the local_command_line vfunc, instead of the newly introduced handle_local_options vfunc

So in principle yes, I think this should still be done, since we should try to make all the core app use our foundation libraries in the best way, follow the documented best-practice and reduce the total amount of code.


On the other hand the patch as it is does not work, while the current code does, so I can understand if you are not willing to put time into it and prefer to WONTFIX it for now

Lets WONTFIX for now, and we can reopen if this becomes an issue.

(In reply to Paolo Borelli from comment #3)
> The new patch does actually not work: when debugging a little bit it seems
> that the command line is properly parsed and the dbus messages exchanged as
> expected, but when the actions are activated nothing happens.

Thanks Paolo!

I undusted the patch and pushed it as https://git.gnome.org/browse/gnome-software/commit/?id=1a83e8afb4215636985b9ae324eabeffcf6d5798 with some fixes.

Two main things that made it not work as expected:

1) When invoking remote actions, it was missing "return 0" which signals gapplication to stop processing it further. Instead, it fell through to 'return -1' which caused gapplication to run the default handler that activated the remote instance and stomped over whatever the previous remote action had done.

2) g_variant_dict_lookup for "local-filename" should have been "^&ay" to match with what gets put in the variant for G_OPTION_ARG_FILENAME; otherwise it never matches and it falls through to the default handler again.