plasma-nm Connection Icon not showing correct icon when using a bridge
ClosedPublic

Authored by rthomas on Feb 2 2019, 10:43 PM.

Details

Summary

This bug has already been reported in bug 397352

Whenever I'm adding a bridge interface to my system, plasma-nm changes the connection icon to a disconnected state even though an active wired or wireless connection is present. In my case, even though I have an active WiFi connection running in the background, as soon as I add the bridge interface, the connection icon changes from 'network-wireless-100' to 'network-wireless-available'.

This is probably happening because when the bridge becomes active, NetworkManager considers it the 'ActivatingConnection' and we are choosing the icon based on the 'ActivatingConnection' without running any checks on it. If the 'ActivatingConnection' returns an invalid object we move onto the 'PrimaryConnection'. I have made this diff to only consider the ActivatingConnection for the icon if the first Device for that connection is a "Wifi", "Ethernet", "Modem" or "Bluetooth". If it's not then move to the PrimaryConnection.

BUG: 397352

Diff Detail

Repository
R116 Plasma Network Management Applet
Lint
Lint Skipped
Unit
Unit Tests Skipped
rthomas created this revision.Feb 2 2019, 10:43 PM
Restricted Application added 1 blocking reviewer(s): jgrulich. · View Herald TranscriptFeb 2 2019, 10:43 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
rthomas requested review of this revision.Feb 2 2019, 10:43 PM
rthomas updated this revision to Diff 50771.Feb 3 2019, 2:59 PM
rthomas added a reviewer: Plasma.

Should have used '&&' in the if statement instead of '||'. We want to shift to the PrimaryConnection if the Device is not among Wifi, Ethernet, Modem and Bluetooth.

Reason for choosing those 4 devices is those are the only devices we set icons for (according to my understanding).

This doesn't seem to be a correct fix. We should be using icon for whatever connection is primary, not only when primary connection is wifi / ethernet / modem / bluetooth. If you check NM on dbus, what is the primary connection there? Is it the bridge one?

Correct fix would be to set icon for bridge connections in the last else branch at the end of setIcons() method, probably guarded with a condition whether the user has enabled virtual connections.

Correct fix would be to set icon for bridge connections in the last else branch at the end of setIcons() method, probably guarded with a condition whether the user has enabled virtual connections.

I'm not sure plasma-nm has ever used icons for bridges. Even at the end of setIcons() we are just ignoring every other device (https://cgit.kde.org/plasma-nm.git/tree/libs/declarative/connectionicon.cpp#n440) and calling setDisconnectedIcon(). This bug doesn't have to be limited to a bridge, even if I add a Dummy interface via NetworkManager, NetworkManager will consider that the ActivatingConnection and plasma-nm will not select the proper icon because of that.

I think virtual devices should be considered secondary and not be considered when choosing an icon.

PS: PrimaryConnection for me is always the Wifi connection for some reason, and the Bridge is always chosen as the ActivatingConnection

This doesn't seem to be a correct fix. We should be using icon for whatever connection is primary, not only when primary connection is wifi / ethernet / modem / bluetooth. If you check NM on dbus, what is the primary connection there? Is it the bridge one?

You are right, we should be using the icon for the primary connection. But currently we're choosing the icon based on the "ActivatingConnection". We only go to the primary connection, if the activating connection object is invalid. (https://cgit.kde.org/plasma-nm.git/tree/libs/declarative/connectionicon.cpp#n315)

That sounds like a bug in NetworkManager, because ActivatingConnection should be the one which will become PrimaryConnection, that's what the documentation says, that's the reason why we use it. If it's a NM bug, we still need a workaround.

In your code, you can simplify it with:

if (connection && !UiUtils::isConnectionTypeSupported(connection->type()) {
    connection = NetworkManager::primaryConnection();
}

Problem is that isConnectionTypeSupported() will return true if you have virtual connections enabled. We should maybe add additional method to to UiUtils to identify virtual connections (this new one can be then used inside isConnectionTypeSupported())

I would suggest.

bool UiUtils::isConnectionTypeVirtual(NetworkManager::ConnectionSettings::ConnectionType type)
{
    if (type == NetworkManager::ConnectionSettings::Bond ||
        type == NetworkManager::ConnectionSettings::Bridge ||
        type == NetworkManager::ConnectionSettings::Infiniband ||
        type == NetworkManager::ConnectionSettings::Team ||
        type == NetworkManager::ConnectionSettings::Vlan) {
        return true
    }
  
   return false;
}

With this you can simply use

if (connection && isConnectionTypeVirtual(connection->type()) {
    connection = NetworkManager::primaryConnection;
}
rthomas updated this revision to Diff 50899.Feb 5 2019, 1:12 AM

Hey Jan,

I've made the changes you suggested. Let me know if anything needs to be fixed.

rthomas updated this revision to Diff 50900.Feb 5 2019, 2:08 AM

Shouldn't have removed the existing condition to check if the connection object is not valid. Added it back.

jgrulich accepted this revision.Feb 5 2019, 7:06 AM
jgrulich added inline comments.
libs/uiutils.h
126 ↗(On Diff #50900)

Can you just move this method up under the isConnectionTypeSupported() method? So similar methods are together.

This revision is now accepted and ready to land.Feb 5 2019, 7:06 AM
rthomas updated this revision to Diff 51006.Feb 6 2019, 12:06 AM

Moved isConnectionTypeVirtual() below isConnectionTypeSupported()

rthomas updated this revision to Diff 51009.Feb 6 2019, 12:16 AM

I've also moved isConnectionTypeVirtual() in uiutils.cpp. Forgot to do that!

jgrulich accepted this revision.Feb 6 2019, 6:37 AM

Hey Jan, please make this commit on my behalf as I don't have commit access right now and I'm not planning to apply for it any time soon.

This revision was automatically updated to reflect the committed changes.