- User Since
- Aug 24 2015, 11:06 AM (187 w, 22 h)
Thu, Mar 21
Looks good to me. Thanks.
Sun, Mar 17
Fri, Mar 15
Tue, Mar 5
Can be closed if I'm not mistaken, the issue has been already fixed.
Can you please rebase this change on top of master? It doesn't apply.
Mon, Mar 4
Sat, Mar 2
Good idea to remove the failed connection.
Shouldn't we maybe use QSpinBox for the port? With the spinbox you can also set min and max values so you don't need to validate it.
Can you attach output from:
- nmcli -f all connection
- nmcli -f all device show
Fri, Mar 1
Should be enough I guess, but not sure if this would filter your connections also from the kcm, did you check?
Feb 24 2019
Feb 23 2019
Feb 22 2019
Feb 14 2019
Looks good. Thank you.
Feb 13 2019
Feb 12 2019
I wonder whether this should also go to Plasma 5.15 branch, it's an improvement and doesn't introduce new strings so should be ok to be backported.
Feb 11 2019
I would keep the 15s interval, everyone does 15s. Also from what I have read, every scan drops your connection for a while, which might be a problem for bad wifi drivers, where the scan can take 15+ seconds so doing this more often is not a good idea. Other thing I would change is to try to repeat the scan if it fails only for the first time (when you open the applet), then I don't think it's necessary to keep spamming NetworkManager with our requests.
Feb 6 2019
Seems to work. Thanks.
Looks good from the portal point of view, but I would rather wait for someone who knows KRun internally to approve this review.
Feb 5 2019
This breaks connection details, try to expand details for some available connection. There appears to be some issue with heigh of the items.
Feb 4 2019
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.
Feb 3 2019
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.
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?
Feb 1 2019
If it wil be made configurable, you wouldn't be still able to use it, without depending on newer NM version.
Jan 31 2019
Looks good to me and also seems to work properly.
+1, I actually like it now with the search button. I will try it properly later and go through your code.
Jan 30 2019
Besides, it doesn't seem to work as it should:
- It removed the button in the top right corner which opens the KCM
- When I restart plasmashell, the search bar is visible even when I have only 4 connections (no scrollbar), typing some text and clearing it makes it disappear correctly
I'm wrong, it actually doesn't perform scanning when the applet is opened. It should maybe be doing that, do initial scan on applet popup and then every 15 seconds only if you keep it opened.
With this approach I will basically see it all the time, you can have just one connection hidden and the search bar will pop up, I don't want that and in my opinion it doesn't look good. Either should be placed in the toolbar or visible only when you start typing.
I don't think this should be added again, there is no reason to. Plasma-nm will do an initial scan once the applet is opened so you get the new networks immediately. This has been discussed with NetworkManager developers and everyone has to do now periodic scanning in some sane interval. Gnome does the some.
Putting the search bar in the toolbar on top is an interesting idea. There's definitely room on mine. However it does start to feel a bit cramped. +1 for the idea of a search field though.
Martin, can you please look into this?
Wouldn't be a small search bar placed next to the aiplane mode enough? That way it will not need additional vertical space. Hiding it when there is only a small amount of connections is a good idea.
Jan 29 2019
Jan 28 2019
Looks good to me, I woud personally keep the header in the popup menu, at least I know I clicked on correct connection.
Jan 25 2019
I'm not convinced this is needed at all. How often do you need to modify a connection? I also don't think many people would discover this feature, because many of them also didn't discover context menu in KCM.
Jan 18 2019
Jan 17 2019
Hi, can I get please this re-approved? It's now just again about the additional mouse support.
Jan 10 2019
Just for the info, this will need to be pushed once Plasma 5.15 is branched, because it introduces dependency on KDE Frameworks 5.55.
Drop keyboard support
Drop keyboard support from this review
Jan 9 2019
I didn't intent to push this together, but when creating a branch from master which already had first round of changes caused my second part of changes to be pushed here. I hope you don't mind that. I'm doing this in a hurry hoping to get exception from David Faure to include this with KDE Frameworks 5.54. I thought the tagging is this saturday and tarballs are made after that, but I was wrong. I would really like to push the remote desktop support into Plasma 5.15, I have almost complete support in xdg-desktop-portal-kde and krfb.
Add support for keyboard key press and release
Add support for keyboard key press and release
Bump fake interface version