Fix PackageKit not emitting network state changed signal when stopped #30
Conversation
PackageKit can be configured to shutdown automatically after x seconds of inactivity. If a computer doesn't have a network connection and PackageKit is shut down (for example because a laptop user takes too long to enter the wifi password) then PackageKit never emits the properties changed signal (it can't since it's not running) so PackageKit-Qt never emits the networkStateChanged signal and applications like the plasma pk update applet never updates its status. With this commit, a NetworkManagerMonitor class is added that connects directly to the NetworkManager's StateChanged signal and emits a networkStateChanged signal, so we can react to a network state change even when PackageKit is stopped. In this case, we update all properties, which triggers PK to run and emits the appropiate PK signals as expected.
|
Hmm I'm not very happy with adding code that talks to another daemon, I'm confused because IIRC PK-QT advises about PK running status. |
I understand your concerns, but the way I see it, PK-Qt has a contract to emit the networkStateChanged signal when the network state changes, and it doesn't do that because of the configuration of PK. If PK was configured not to shutdown, it would emit the signal, so this is a way to encapsulate the internals of PK and always emit the signal when it should. Also, note that the PR doesn't just forward the signal from NM, but it uses the signal just to reload the PK properties and to emit the corresponding signals without adding complexity to the code. Btw, just so that you have all the information, this PR is part of the fix for https://bugzilla.opensuse.org/show_bug.cgi?id=1103678, which is basically a polish of the plasma-pk-update applet user experience related to network connectivity changes. Each of the fixes mentioned in that bugzilla issue is independent and fixes separate problems.
Yes, that's true, but still, IMHO fixing it for NM users is better than nothing and I don't think this can be fixed for non-NM users right now.
The update applet relies on the networkStateChanged signal for a number of tasks:
|
…cations. Summary: Hide actions that can't be taken if the system doesn't have a network connection. Add its own messageChanged NOTIFY signal to the message property The message property also changes when the network state changes, not only when isActiveChanged is emitted, so let's create its own signal that is emitted in both cases. Delay PkUpdates::checkUpdates calls if the network state is offline If PkUpdates::checkUpdates is called and the network state is offline, delay the check for updates until the network is online again. This fixes the problem that when the user logs in, the applet is run and just after the PkUpdates object is created, checkUpdates is called (from main itself). But at that point the user might have not entered the wifi password so the check would fail. Now, if we detect there's no network, we just delay the check until the network state is online. Note that some of these fixes may also need either one or more of the following fixes depending on your system: https://gitlab.gnome.org/GNOME/glib/merge_requests/719 https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/138 PackageKit/PackageKit-Qt#30 Test Plan: Reboot a laptop with no network connection. The applet showed network failure notifications before the commits but not after them. Also, connect and disconnect and check the applet contents. Before the commits are applied the applet contained options that make no sense without network. After the commits are applied it just shows a "Network is offline" message which makes more sense. Reviewers: jgrulich Reviewed By: jgrulich Differential Revision: https://phabricator.kde.org/D19862
|
ping? JFYI, the related fixes in glib2, NetworkManager and the plasma applet are already merged in their respective repositories, so only this would be left to have nicer behavior of the update notifications in the desktop. Is there anything I can do to improve the PR? |
|
To be honest, I agree that the fix is at the wrong level. PackageKit-Qt is supposed to be an interface to what PackageKit does, and this adds autonomous behaviour modifications. Such changes could be either implemented in clients if necessary or maybe it would make sense to have some mechanisms to have PackageKit wake up upon network status change, but that's something that could be implemented for PacakgeKit itself as it doesn't have much to do with Qt itself. |
| NM_DBUS_INTERFACE, | ||
| QLatin1String("state")); | ||
|
|
||
| QDBusMessage reply = QDBusConnection::systemBus().call(message); |
There was a problem hiding this comment.
Yes, if you check the commit, NetworkManagerMonitor::state is not really used. I just added it for completeness (and for debug purposes). If it bothers you that someone might change the code in the future and call it, we can remove the method without any problem
| void DaemonPrivate::getAllPropertiesIfPackageKitNotRunning() | ||
| { | ||
| if (!running) | ||
| getAllProperties(); |
There was a problem hiding this comment.
If PK is not running I believe getAllProperties() is likely to wake it up again, thus, talking to NM useless if you don't let it stop.
There was a problem hiding this comment.
Well, that's the whole point of this PR. We talk to NM because, when PK is not running, it doesn't emit the propertiesChanged signal when the network state changes, so we use NM to have an alternate way to find out we need to update PK properties while PK is not running. Once we got notified the network is up, we call getAllPropertiesIfPackageKitNotRunning, which, as you said, wakes PK so it can give us the updated properties through the usual channels.
There was a problem hiding this comment.
Note that the check for !running is important since that controls that we only do something different than without this PR when PK is stopped. Apart from that, the PR is just signal connections
|
Yeah, maybe you could open an issue in PK so that we could discuss there what could be done at PK level. |
Well, we would have to duplicate those changes in plasma-pk-updates, discover and any other app that use this library. If we can avoid code duplication and just fix it once and for all here, I think it's better.
PackageKit is not running, so who would wake it up? With this PR, it's PackageKit-Qt that takes care of transparently do it. Do you have any better suggestion? |
|
We could have a script on connectivity-change together in /etc/NetworkManager/dispatcher.d. This could call pkcon to do whatever is necessary. Does that help? |
…cations. Summary: Hide actions that can't be taken if the system doesn't have a network connection. Add its own messageChanged NOTIFY signal to the message property The message property also changes when the network state changes, not only when isActiveChanged is emitted, so let's create its own signal that is emitted in both cases. Delay PkUpdates::checkUpdates calls if the network state is offline If PkUpdates::checkUpdates is called and the network state is offline, delay the check for updates until the network is online again. This fixes the problem that when the user logs in, the applet is run and just after the PkUpdates object is created, checkUpdates is called (from main itself). But at that point the user might have not entered the wifi password so the check would fail. Now, if we detect there's no network, we just delay the check until the network state is online. Note that some of these fixes may also need either one or more of the following fixes depending on your system: https://gitlab.gnome.org/GNOME/glib/merge_requests/719 https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/138 PackageKit/PackageKit-Qt#30 Test Plan: Reboot a laptop with no network connection. The applet showed network failure notifications before the commits but not after them. Also, connect and disconnect and check the applet contents. Before the commits are applied the applet contained options that make no sense without network. After the commits are applied it just shows a "Network is offline" message which makes more sense. Reviewers: jgrulich Reviewed By: jgrulich Differential Revision: https://phabricator.kde.org/D19862
PackageKit can be configured to shutdown automatically after x seconds
of inactivity. If a computer doesn't have a network connection and PackageKit
is shut down (for example because a laptop user takes too long to enter the
wifi password) then PackageKit never emits the properties changed signal
(it can't since it's not running) so PackageKit-Qt never emits the
networkStateChanged signal and applications like the plasma pk update applet
never updates its status.
With this commit, a NetworkManagerMonitor class is added that connects
directly to the NetworkManager's StateChanged dbus signal and emits a
networkStateChanged qt signal, so we can react to a network state change
even when PackageKit is stopped. In this case, we update all properties,
which triggers PK to run and emits the appropiate PK signals as
expected.