After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 772767 - Merge external "gsd-printer" binary into daemon
Merge external "gsd-printer" binary into daemon
Status: RESOLVED OBSOLETE
Product: gnome-settings-daemon
Classification: Core
Component: printers
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Marek Kašík
gnome-settings-daemon-maint
Depends on:
Blocks:
 
 
Reported: 2016-10-11 16:09 UTC by Bastien Nocera
Modified: 2019-03-20 11:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Merge gsd-printer into print notifications plugin (55.17 KB, patch)
2017-05-15 09:43 UTC, Marek Kašík
none Details | Review
Merge gsd-printer into print notifications plugin (54.97 KB, patch)
2017-08-08 10:13 UTC, Marek Kašík
none Details | Review

Description Bastien Nocera 2016-10-11 16:09:49 UTC
Now that we've split up gnome-settings-daemon into smaller daemons, we shouldn't need to run gsd-printer separately from the gsd-print-notifications daemon.

This might be slightly problematic if gsd-printer hangs though, so we might want to wait until we use systemd for the session handling.
Comment 1 Marek Kašík 2017-05-15 09:43:49 UTC
Created attachment 351874 [details] [review]
Merge gsd-printer into print notifications plugin

Hi,

the attached patch merges the gsd-printer into the print notifications plugin. It does not convert the sync calls to async. If you want to change them let me know and I will do it.
Comment 2 Bastien Nocera 2017-05-15 19:03:35 UTC
Felipe, can you review this?
Comment 3 Bastien Nocera 2017-05-15 19:05:10 UTC
(In reply to Marek Kašík from comment #1)
> Created attachment 351874 [details] [review] [review]
> Merge gsd-printer into print notifications plugin
> 
> Hi,
> 
> the attached patch merges the gsd-printer into the print notifications
> plugin. It does not convert the sync calls to async. If you want to change
> them let me know and I will do it.

I'd say that if at any point the front-ends will fail because a sync call is in progress (say, notifications not showing up for a printer because another one is getting installed), then we need to port to async. But this can be done separately if the combined patch would be too complicated to review.
Comment 4 Marek Kašík 2017-05-16 10:24:14 UTC
(In reply to Bastien Nocera from comment #3)
> (In reply to Marek Kašík from comment #1)
> > Created attachment 351874 [details] [review] [review] [review]
> > Merge gsd-printer into print notifications plugin
> > 
> > Hi,
> > 
> > the attached patch merges the gsd-printer into the print notifications
> > plugin. It does not convert the sync calls to async. If you want to change
> > them let me know and I will do it.
> 
> I'd say that if at any point the front-ends will fail because a sync call is
> in progress (say, notifications not showing up for a printer because another
> one is getting installed), then we need to port to async. But this can be
> done separately if the combined patch would be too complicated to review.

Ok, lets get this in and I'll look at the async calls if needed.
Comment 5 Rui Matos 2017-05-24 15:19:43 UTC
Review of attachment 351874 [details] [review]:

I'm a bit puzzled at how this is supposed to work. You say

"Acquire com.redhat.NewPrinterNotification and
com.redhat.PrinterDriversInstaller names on the session bus"

but I see the code owning names on the system bus. Also

"Don't handle Gnome session signals (which we needed for gsd-printer binary)
as these are already handled."

but the code is handling session presence signals. In fact I suspect (see below) that session presence is the wrong thing to be using here

::: plugins/print-notifications/gsd-print-notifications-manager.c
@@ +1340,3 @@
+
+        if (new_status == PRESENCE_STATUS_IDLE ||
+            new_status == PRESENCE_STATUS_AVAILABLE) {

what is this trying to achieve? I suspect this should be using the SessionIsActive property instead if what we want is to withdraw our DBus services when users switch to a different VT.

@@ +1342,3 @@
+            new_status == PRESENCE_STATUS_AVAILABLE) {
+                unregister_objects (manager);
+                unown_names (manager);

the correct order to avoid races is to drop the name first and only then drop objects registration

@@ +1409,3 @@
+
+        if (manager->priv->npn_owner_id == 0)
+                manager->priv->npn_owner_id = g_bus_own_name (G_BUS_TYPE_SYSTEM,

I must say that connecting to the system bus as a *service* from a session component is completely backwards from DBus best practices. 

But since this seems to be a design decision from system-config-printer we can't do much about it here.
Comment 6 Marek Kašík 2017-08-08 10:13:47 UTC
Created attachment 357179 [details] [review]
Merge gsd-printer into print notifications plugin

(In reply to Rui Matos from comment #5)
> Review of attachment 351874 [details] [review] [review]:
> 
> I'm a bit puzzled at how this is supposed to work. You say
> 
> "Acquire com.redhat.NewPrinterNotification and
> com.redhat.PrinterDriversInstaller names on the session bus"
> 
> but I see the code owning names on the system bus. Also

You are right, the text should say "system bus".


> "Don't handle Gnome session signals (which we needed for gsd-printer binary)
> as these are already handled."
> 
> but the code is handling session presence signals. In fact I suspect (see
> below) that session presence is the wrong thing to be using here

This was an inaccurate comment about handling of session presence signals in the removed binary gsd-printer. We still need to handle session presence in the plugin.


> ::: plugins/print-notifications/gsd-print-notifications-manager.c
> @@ +1340,3 @@
> +
> +        if (new_status == PRESENCE_STATUS_IDLE ||
> +            new_status == PRESENCE_STATUS_AVAILABLE) {
> 
> what is this trying to achieve? I suspect this should be using the
> SessionIsActive property instead if what we want is to withdraw our DBus
> services when users switch to a different VT.

Ok, I've changed it the way the "color" and the "power" plugins use the SessionIsActive property.


> @@ +1342,3 @@
> +            new_status == PRESENCE_STATUS_AVAILABLE) {
> +                unregister_objects (manager);
> +                unown_names (manager);
> 
> the correct order to avoid races is to drop the name first and only then
> drop objects registration

Thanks for spotting this.


> @@ +1409,3 @@
> +
> +        if (manager->priv->npn_owner_id == 0)
> +                manager->priv->npn_owner_id = g_bus_own_name
> (G_BUS_TYPE_SYSTEM,
> 
> I must say that connecting to the system bus as a *service* from a session
> component is completely backwards from DBus best practices. 
> 
> But since this seems to be a design decision from system-config-printer we
> can't do much about it here.

Yes, you are right.
Comment 7 GNOME Infrastructure Team 2019-03-20 11:37:29 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/issues/307.