Bug 320533

Summary: Connection failed notification triggered despite successful connection
Product: Network Management Reporter: Eike Hein <hein>
Component: WirelessAssignee: Lamarque V. Souza <lamarque>
Status: RESOLVED FIXED    
Severity: normal CC: adaptee, info, jgrulich, starbuck
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Fedora RPMs   
OS: Linux   
Latest Commit: Version Fixed In: 0.9.0.9
Attachments: Suppress "connection failed" notification if connection is deleted
Partial backport of 4706c1c2 that applies cleanly to v0.9.0.8
Partial backport of 4706c1c2 and 6582b153 that applies cleanly to v0.9.0.8

Description Eike Hein 2013-05-31 12:55:39 UTC
I'm using an unpatched 0.9.0.8 from Fedora 18 repositories as far as I can see.

If I do the following:

1. While connected to my WLAN, go into "Manage Connections" and delete the active connection
2. Pick the same WLAN from the connection list in the widget
3. Enter the password in the resulting properties dialog and OK it

... then the connection succeeds, but despite this the "Connection failed" KNotify connection is triggered, resulting in a confusing and incorrect notification bubble shown in the desktop shell.

This bug seems to exist in a bunch of subtle variations and across distros; I've gotten reports from Kubuntu/Netrunner users as well for example. The general theme being that the "Connection failed" notification is triggered even if this isn't actually the case.

Reproducible: Always
Comment 1 Eike Hein 2013-05-31 12:56:51 UTC
Sorry for the typo, s/KNotify connection/KNotify notification/ of course.
Comment 2 starbuck 2013-05-31 13:00:48 UTC
not sure about kubuntu, but netrunner is reported to be affected as well (0.9.0.8-0ubuntu1)
Comment 3 Jekyll Wu 2013-05-31 14:59:18 UTC
I remember seeing such notifications too, when playing with various recently released livecd for fun :)
Comment 4 Lamarque V. Souza 2013-05-31 15:04:11 UTC
Created attachment 80219 [details]
Suppress "connection failed" notification if connection is deleted

Well, when the connection is deleted the failed connection notification should be triggered since the connection attempt really failed, but it should not wait until the next connection attempt to show up. I have tested your steps here and no notification appears, maybe because the object that triggers the notification is being deleted too fast. Can you recompile Plasma NM with the patch attached and see if it suppress the "connection failed" notification?
Comment 5 Eike Hein 2013-06-07 21:39:03 UTC
The patch doesn't appear to help, I'm still getting the notification. Note that since I was evaluating for distro inclusion I tested it against the v0.9.0.8 tag, not master.

(Also curious btw: In order for clicking a WLAN list item to bring up the dialog, I had to install into /usr, a folder in $KDEDIRS/$PATH in my $HOME didn't work as it normally does with KDE apps.)
Comment 6 Eike Hein 2013-06-07 21:43:12 UTC
Addendum: In addition to the notification I'm also getting a failed tray icon now, briefly, before it is replaced with the success one. I believe (but am not entirely sure) that's new with the patch, before it went straight to the success icon.
Comment 7 Eike Hein 2013-06-07 21:44:14 UTC
Hm, but not always either @ icon, so it may have been happening before, but is simply quite racy.

Sorry for the reply noise.
Comment 8 Lamarque V. Souza 2013-06-10 14:25:15 UTC
By the way, you have to restart kded4 (or relogin) for the patch to take effect. Are you really sure you installed it in /usr? If you install it in another path probably Plasma NM's libraries in /usr will take precedence and the patch will not take effect. The systray icon glitch is unrelated to this patch, the notification system and the tray icon are handled by two different system proccesses (kded4 and plasma-desktop).

Just for debugging, does the notification appears if you disable all Plasma NM's notifications in "Manage Connections" -> "Other" -> "Configure Notifications"?
Comment 9 Eike Hein 2013-06-10 17:26:25 UTC
- I logged in and out.
- I tried removing the system version of KNM completely, but still had to install to /usr for the dialog to work, so maybe the KNM code uses KStandardDirs incorrectly somewhere (installing to $KDEDIRS and even overriding a system copy that way is supposed to work).

I'll have a look at the notification thing later tonight, can't cut my connection at the moment.
Comment 10 Eike Hein 2013-06-11 03:14:49 UTC
Yes, the notification isn't triggered if I disable it in the KNotify settings.
Comment 11 Eike Hein 2013-06-13 09:22:31 UTC
FWIW, of the two possible KNotification callsites for this message, the one taken is notificationmanager.cpp:129.
Comment 12 Eike Hein 2013-06-13 09:24:03 UTC
So that means there's a state transition from Activating to Unknown that it declares as connection failure, which is apparently an incorrect assumption to make.
Comment 13 Lamarque V. Souza 2013-06-15 03:31:54 UTC
Can you test if the commit below fixes this problem?

http://commits.kde.org/networkmanagement/4be000f0b162c52d4c7e0ffdc2f0143b1cfffcdb

To test it just 'git pull' either master or nm09 branches and recompile it.
Comment 14 Eike Hein 2013-06-15 04:13:31 UTC
I'll test, but can you comment on whether you expect this patch to work against 0.9.0.8? If it works, I intend to ship this in a distro where this bug is considered a release blocker candidate (a confusing error message on initial connect is kind of terrible for users).
Comment 15 Lamarque V. Souza 2013-06-15 12:49:32 UTC
The patch requires NetworkManager 0.9.8, if the distribution you mentioned does not use that  NetworkManager version then you will have to upgrade. This patch applies to Plasma NM 0.9.0.8 and you can even compile Plasma NM against NetworkManager 0.9.8 and then run a lower NetworkManager version but then the patch will have no effect since the notification manager now depends on the two new connection states (deactivating and deactivated) that only NetworkManager >= 0.9.8 uses.
Comment 16 Eike Hein 2013-06-15 13:15:50 UTC
I am about to test this with a simplified patch applied to v0.9.0.8, but please note that modern compilers will (with good reason) warn about this code because the switch statement in notificationmanager.cpp doesn't handle the Deactivating (as opposed to Deactivated) value and it has no default case.
Comment 17 Eike Hein 2013-06-15 13:24:38 UTC
Created attachment 80529 [details]
Partial backport of 4706c1c2 that applies cleanly to v0.9.0.8

Yay - the attached patch that applies cleanly to v0.9.0.8 no longer incorrectly announces connection failure.

I'll hold off on shipping this for now however in case you want to have another crack at that switch statement first?
Comment 18 Lamarque V. Souza 2013-06-15 14:12:34 UTC
Git commit 6582b1534e51e959593b17c8d4e52aa494e1d28c by Lamarque V. Souza.
Committed on 15/06/2013 at 16:04.
Pushed by lvsouza into branch 'nm09'.

Distinguish between user deactivation and activation error for vpn connections
as well.
FIXED-IN: 0.9.0.9
(cherry picked from commit f5c57ab9ba95db528fa89906335f8dca44b43439)

M  +11   -4    libs/service/notificationmanager.cpp

http://commits.kde.org/networkmanagement/6582b1534e51e959593b17c8d4e52aa494e1d28c
Comment 19 Lamarque V. Souza 2013-06-15 14:12:46 UTC
Git commit f5c57ab9ba95db528fa89906335f8dca44b43439 by Lamarque V. Souza.
Committed on 15/06/2013 at 16:04.
Pushed by lvsouza into branch 'master'.

Distinguish between user deactivation and activation error for vpn connections
as well.
FIXED-IN: 0.9.0.9

M  +11   -4    libs/service/notificationmanager.cpp

http://commits.kde.org/networkmanagement/f5c57ab9ba95db528fa89906335f8dca44b43439
Comment 20 Eike Hein 2013-06-16 13:41:10 UTC
Created attachment 80552 [details]
Partial backport of 4706c1c2 and 6582b153 that applies cleanly to v0.9.0.8

Thanks Lamarque; attached is the patch we'll apply to v0.9.0.8.
Comment 21 Lamarque V. Souza 2013-07-20 22:36:32 UTC
*** Bug 322631 has been marked as a duplicate of this bug. ***
Comment 22 Isaac Mercer 2013-07-22 02:30:01 UTC
How do I Install this on Ubuntu 13.04. is there a .deb package available.
Comment 23 Isaac Mercer 2013-07-22 02:30:42 UTC
How do I Install this on Ubuntu 13.04. is there a .deb package available.