GNOME Bugzilla – Bug 772767
Merge external "gsd-printer" binary into daemon
Last modified: 2019-03-20 11:37:29 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.
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.
Felipe, can you review this?
(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.
(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.
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.
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.
-- 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.