Discussion:
Review Request 128707: Add support for captive portals
Jan Grulich
2016-08-18 10:15:27 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------

Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.


Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417


Repository: plasma-nm


Description
-------

Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.


Diffs
-----

CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41

Diff: https://git.reviewboard.kde.org/r/128707/diff/


Testing
-------

Tested with three different captive portals and it worked perfectly.


Thanks,

Jan Grulich
Sebastian Kügler
2016-08-18 10:24:01 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98459
-----------------------------------------------------------




kded/portalmonitor.h (line 43)
<https://git.reviewboard.kde.org/r/128707/#comment66338>

QScopedPointer?



kded/portalmonitor.cpp (line 47)
<https://git.reviewboard.kde.org/r/128707/#comment66342>

if it's Portal, and m_view has been created, maybe it should be shown?



kded/portalmonitor.cpp (line 51)
<https://git.reviewboard.kde.org/r/128707/#comment66339>

Why? Shouldn't we let the window manager decide this? On my 4K screen, it doesn't make sense to maximize, for example.



kded/portalmonitor.cpp (line 52)
<https://git.reviewboard.kde.org/r/128707/#comment66340>

parent the lambda to this



kded/portalmonitor.cpp (line 56)
<https://git.reviewboard.kde.org/r/128707/#comment66344>

nullptr, no need to use the Qt macro



kded/portalmonitor.cpp (line 64)
<https://git.reviewboard.kde.org/r/128707/#comment66345>

parent to this



kded/service.cpp (line 49)
<https://git.reviewboard.kde.org/r/128707/#comment66346>

coding style, PortalMonitor *portalMonitor

(should be at least consistent for new code)



kded/service.cpp (line 60)
<https://git.reviewboard.kde.org/r/128707/#comment66347>

nullptr


- Sebastian KÃŒgler
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 10:15 a.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Kai Uwe Broulik
2016-08-18 10:27:25 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98460
-----------------------------------------------------------



While I really appreciate that we get support for this I don't think just opening a webview automatically is a good idea.

Can we perhaps show a notification [1] "log in to this network" with a button that will then open the web view? Would also be nice if we could indicate that we're behind a portal in the network applet icon (note the exclamation mark on the wifi icon) - we even already have "limited" icons in NM.

[1] Loading Image...


kded/portalmonitor.cpp (lines 40 - 42)
<https://git.reviewboard.kde.org/r/128707/#comment66341>

delete nullptr is fine, no need for an if check



kded/portalmonitor.cpp (line 50)
<https://git.reviewboard.kde.org/r/128707/#comment66343>

That is a pretty technical description


- Kai Uwe Broulik
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 10:15 vorm.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Jan Grulich
2016-08-18 11:04:05 UTC
Permalink
Post by Jan Grulich
Post by Kai Uwe Broulik
While I really appreciate that we get support for this I don't think just opening a webview automatically is a good idea.
Can we perhaps show a notification [1] "log in to this network" with a button that will then open the web view? Would also be nice if we could indicate that we're behind a portal in the network applet icon (note the exclamation mark on the wifi icon) - we even already have "limited" icons in NM.
[1] https://4.bp.blogspot.com/-dw9MiaM5GmE/VaFd4t_QmZI/AAAAAAAAFuM/OTMfLTcdUeA/s1600/starbucks.png
We already kinda show exclamation mark on the wifi icon, it's just that the connectivity is not updated everytime and needs to be perhaps forced everytime we connect to a new network.

Showing a notification is a good idea, that's why I added more people (including usability) to even start a discussion about that and implement it the best possible way. For now I implemented it the same way as Gnome does.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98460
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Srp. 18, 2016, 10:58 dop.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Thomas Pfeiffer
2016-08-18 20:18:26 UTC
Permalink
Post by Jan Grulich
Post by Kai Uwe Broulik
While I really appreciate that we get support for this I don't think just opening a webview automatically is a good idea.
Can we perhaps show a notification [1] "log in to this network" with a button that will then open the web view? Would also be nice if we could indicate that we're behind a portal in the network applet icon (note the exclamation mark on the wifi icon) - we even already have "limited" icons in NM.
[1] https://4.bp.blogspot.com/-dw9MiaM5GmE/VaFd4t_QmZI/AAAAAAAAFuM/OTMfLTcdUeA/s1600/starbucks.png
We already kinda show exclamation mark on the wifi icon, it's just that the connectivity is not updated everytime and needs to be perhaps forced everytime we connect to a new network.
Showing a notification is a good idea, that's why I added more people (including usability) to even start a discussion about that and implement it the best possible way. For now I implemented it the same way as Gnome does.
I agree with Kai Uwe: Detecting that a wifi needs login is a background event, not necessarily the user's current main task, therefore a notification is the right way to inform the user about it and allow the appropriate action.


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98460
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 12:47 p.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Jan Grulich
2016-08-18 10:58:46 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------

(Updated Srp. 18, 2016, 10:58 dop.)


Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.


Changes
-------

Fixes mentioned issues.

I'm just not sure about my usage of QScopedPointer as I have never used it before. I removed initialization/deinitialization of m_view pointer in connectivityChanged() slot and I just show it or hide it instead.


Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417


Repository: plasma-nm


Description
-------

Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.


Diffs (updated)
-----

CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41

Diff: https://git.reviewboard.kde.org/r/128707/diff/


Testing
-------

Tested with three different captive portals and it worked perfectly.


Thanks,

Jan Grulich
Sebastian Kügler
2016-08-18 11:08:32 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98462
-----------------------------------------------------------




kded/service.cpp (line 49)
<https://git.reviewboard.kde.org/r/128707/#comment66348>

PortalMonitor is not known when webengine support is off at build time



kded/service.cpp (line 86)
<https://git.reviewboard.kde.org/r/128707/#comment66349>

This would also need a an #if (or you can make webengine required dep, of course)


- Sebastian KÃŒgler
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 10:58 a.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Jan Grulich
2016-08-18 11:42:10 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------

(Updated Srp. 18, 2016, 11:42 dop.)


Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.


Changes
-------

Fix compilation when QWebEngine is not available.


Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417


Repository: plasma-nm


Description
-------

Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.


Diffs (updated)
-----

CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41

Diff: https://git.reviewboard.kde.org/r/128707/diff/


Testing
-------

Tested with three different captive portals and it worked perfectly.


Thanks,

Jan Grulich
Sebastian Kügler
2016-08-18 12:06:34 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98467
-----------------------------------------------------------


Fix it, then Ship it!





kded/portalmonitor.cpp (line 51)
<https://git.reviewboard.kde.org/r/128707/#comment66350>

Perhaps "Redirecting..." since that's what's supposedly happens. It's more telling than "captive portal" IMO.


- Sebastian KÃŒgler
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 11:42 a.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Lamarque Souza
2016-08-18 12:37:01 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98468
-----------------------------------------------------------




kded/portalmonitor.cpp (line 25)
<https://git.reviewboard.kde.org/r/128707/#comment66354>

Nitpick: keep includes in lexicography order as much as possible.



kded/portalmonitor.cpp (line 44)
<https://git.reviewboard.kde.org/r/128707/#comment66353>

You should delete m_view here in case this module is unloaded from kded5.



kded/portalmonitor.cpp (line 63)
<https://git.reviewboard.kde.org/r/128707/#comment66351>

Missing watcher->deleteLater()



kded/portalmonitor.cpp (line 64)
<https://git.reviewboard.kde.org/r/128707/#comment66352>

You should check if reply returned error or this line can lead to crash in that case.


- Lamarque Souza
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 11:42 a.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Jan Grulich
2016-08-18 12:46:58 UTC
Permalink
Post by Jan Grulich
kded/portalmonitor.cpp, line 44
<https://git.reviewboard.kde.org/r/128707/diff/3/?file=474715#file474715line44>
You should delete m_view here in case this module is unloaded from kded5.
Isn't this purpose of QScopedPointer? I suppose there is no need for that as QScopedPointer will do that instead of me.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98468
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Srp. 18, 2016, 11:42 dop.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Lamarque Souza
2016-08-18 14:18:08 UTC
Permalink
Post by Jan Grulich
kded/portalmonitor.cpp, line 44
<https://git.reviewboard.kde.org/r/128707/diff/3/?file=474715#file474715line44>
You should delete m_view here in case this module is unloaded from kded5.
Isn't this purpose of QScopedPointer? I suppose there is no need for that as QScopedPointer will do that instead of me.
Yeah, you are right.


- Lamarque


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98468
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 12:47 p.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Sebastian Kügler
2016-08-18 12:45:51 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98470
-----------------------------------------------------------




kded/portalmonitor.cpp (line 44)
<https://git.reviewboard.kde.org/r/128707/#comment66356>

That shouldn't be necessary when using a scoped pointer.


- Sebastian KÃŒgler
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 11:42 a.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Jan Grulich
2016-08-18 12:47:23 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------

(Updated Srp. 18, 2016, 12:47 odp.)


Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.


Changes
-------

Fixes++.


Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417


Repository: plasma-nm


Description
-------

Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.


Diffs (updated)
-----

CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41

Diff: https://git.reviewboard.kde.org/r/128707/diff/


Testing
-------

Tested with three different captive portals and it worked perfectly.


Thanks,

Jan Grulich
Lamarque Souza
2016-08-18 14:18:14 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98475
-----------------------------------------------------------




kded/service.cpp (line 31)
<https://git.reviewboard.kde.org/r/128707/#comment66357>

#if WITH_WEBENGINE_SUPPORT


- Lamarque Souza
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 12:47 p.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Martin Gräßlin
2016-08-19 06:23:01 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98490
-----------------------------------------------------------



I'm not happy with the QWebEngineView as that's going to crash on Wayland. Every application using QWebEngine needs to be on xcb unfortunately. So that means that we cannot really use anything of QWebEngine in Plasma (which is supposed to work on Wayland)

- Martin GrÀßlin
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 2:47 p.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Martin Gräßlin
2016-08-19 08:12:18 UTC
Permalink
Post by Jan Grulich
Post by Martin Gräßlin
I'm not happy with the QWebEngineView as that's going to crash on Wayland. Every application using QWebEngine needs to be on xcb unfortunately. So that means that we cannot really use anything of QWebEngine in Plasma (which is supposed to work on Wayland)
Just tried a bit, here is IRC log:
[10:07] <mgraesslin_yoga> notmart:
[10:07] <mgraesslin_yoga> [0819/100656:ERROR:gl_surface_qt.cpp(187)] GLContextHelper::getXConfig() failed.
[10:07] <mgraesslin_yoga> Segmentation fault (core dumped)
[10:08] <notmart> woha
[10:08] <mgraesslin_yoga> notmart: that was the demobrowser
[10:08] <mgraesslin_yoga> same with minimal
[10:08] <notmart> the qml one as well?
[10:08] <notmart> (which defies a bit the point of qml bindings for the thing)
[10:08] <mgraesslin_yoga> quicknanobrowser?
[10:09] <notmart> that or angelfish
[10:09] <mgraesslin_yoga> sec, has missing deps
[10:10] <mgraesslin_yoga> notmart: yep, also asserts and prints the getXConfig warning
[10:10] <mgraesslin_yoga> notmart: adding --platform xcb makes it start, but that's of course no option
[10:11] <notmart> yep, not for stuff inside plasma for sure


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98490
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 2:47 p.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Martin Gräßlin
2016-08-19 08:27:19 UTC
Permalink
Post by Martin Gräßlin
Post by Martin Gräßlin
I'm not happy with the QWebEngineView as that's going to crash on Wayland. Every application using QWebEngine needs to be on xcb unfortunately. So that means that we cannot really use anything of QWebEngine in Plasma (which is supposed to work on Wayland)
[10:07] <mgraesslin_yoga> [0819/100656:ERROR:gl_surface_qt.cpp(187)] GLContextHelper::getXConfig() failed.
[10:07] <mgraesslin_yoga> Segmentation fault (core dumped)
[10:08] <notmart> woha
[10:08] <mgraesslin_yoga> notmart: that was the demobrowser
[10:08] <mgraesslin_yoga> same with minimal
[10:08] <notmart> the qml one as well?
[10:08] <notmart> (which defies a bit the point of qml bindings for the thing)
[10:08] <mgraesslin_yoga> quicknanobrowser?
[10:09] <notmart> that or angelfish
[10:09] <mgraesslin_yoga> sec, has missing deps
[10:10] <mgraesslin_yoga> notmart: yep, also asserts and prints the getXConfig warning
[10:10] <mgraesslin_yoga> notmart: adding --platform xcb makes it start, but that's of course no option
[10:11] <notmart> yep, not for stuff inside plasma for sure
now reported upstream: https://bugreports.qt.io/browse/QTBUG-55384


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98490
-----------------------------------------------------------
Post by Martin Gräßlin
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 2:47 p.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Kai Uwe Broulik
2016-08-31 14:00:01 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98809
-----------------------------------------------------------



I just logged into an airport wifi and although I couldn't access the internet and was redirected to a login site, NM would *not* detect it as Portal... what am I missing?

- Kai Uwe Broulik
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 12:47 nachm.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Jan Grulich
2016-09-01 06:36:11 UTC
Permalink
Post by Jan Grulich
Post by Kai Uwe Broulik
I just logged into an airport wifi and although I couldn't access the internet and was redirected to a login site, NM would *not* detect it as Portal... what am I missing?
I guess this - https://plus.google.com/u/0/+HonzaGrulich/posts/YmmCsmodYUc


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98809
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Srp. 18, 2016, 12:47 odp.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Lamarque Souza
2016-09-05 15:47:16 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98901
-----------------------------------------------------------




kded/portalmonitor.h (line 22)
<https://git.reviewboard.kde.org/r/128707/#comment66585>

This should go inside if #ifdef section below.



kded/portalmonitor.cpp (line 50)
<https://git.reviewboard.kde.org/r/128707/#comment66586>

I am wondering how hard this waill affect kde.org web server. This patch has the potentional to create a DDoS attack-like effect on it. Maybe someone from sysadmin should evaluate this.


- Lamarque Souza
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 12:47 p.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Jan Grulich
2016-09-06 07:28:43 UTC
Permalink
Post by Jan Grulich
kded/portalmonitor.cpp, line 50
<https://git.reviewboard.kde.org/r/128707/diff/4/?file=474728#file474728line50>
I am wondering how hard this waill affect kde.org web server. This patch has the potentional to create a DDoS attack-like effect on it. Maybe someone from sysadmin should evaluate this.
I don't think it will affect it, because when you are behind a captive portal you will get redirected immediately so kde.org won't be loaded at all. At least this is my understanding.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98901
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Srp. 18, 2016, 12:47 odp.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Sebastian Kügler
2016-09-06 08:49:50 UTC
Permalink
Post by Jan Grulich
kded/portalmonitor.cpp, line 50
<https://git.reviewboard.kde.org/r/128707/diff/4/?file=474728#file474728line50>
I am wondering how hard this waill affect kde.org web server. This patch has the potentional to create a DDoS attack-like effect on it. Maybe someone from sysadmin should evaluate this.
I don't think it will affect it, because when you are behind a captive portal you will get redirected immediately so kde.org won't be loaded at all. At least this is my understanding.
The user-friendlier portals will redirect you to the site you were originally going to, but I agree, kde.org can handle that (it's a static page request, should be cheap enough).

What could be pretty cool is if the user would be sent to a page that allows donating, kde.org does that, so there may be a net advantage, still.


- Sebastian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98901
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Sept. 6, 2016, 7:37 a.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Lamarque Souza
2016-09-06 08:52:17 UTC
Permalink
Post by Jan Grulich
kded/portalmonitor.cpp, line 50
<https://git.reviewboard.kde.org/r/128707/diff/4/?file=474728#file474728line50>
I am wondering how hard this waill affect kde.org web server. This patch has the potentional to create a DDoS attack-like effect on it. Maybe someone from sysadmin should evaluate this.
I don't think it will affect it, because when you are behind a captive portal you will get redirected immediately so kde.org won't be loaded at all. At least this is my understanding.
The user-friendlier portals will redirect you to the site you were originally going to, but I agree, kde.org can handle that (it's a static page request, should be cheap enough).
What could be pretty cool is if the user would be sent to a page that allows donating, kde.org does that, so there may be a net advantage, still.
Looking at that angle it is Ok from my side :-)


- Lamarque


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98901
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Sept. 6, 2016, 7:37 a.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Jan Grulich
2016-09-06 07:37:15 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------

(Updated Zář. 6, 2016, 7:37 dop.)


Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.


Changes
-------

Fix mentioned issues.


Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417


Repository: plasma-nm


Description
-------

Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.


Diffs (updated)
-----

CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41

Diff: https://git.reviewboard.kde.org/r/128707/diff/


Testing
-------

Tested with three different captive portals and it worked perfectly.


Thanks,

Jan Grulich
Ben Cooksley
2016-09-06 09:06:46 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review98920
-----------------------------------------------------------



From my perspective using www.kde.org for the browser part of the process is fine. Using it to detect whether or not you are behind a captive portal (which afaik is handled by NetworkManager and the URL for doing that is set by distributions).

Please note that the index of www.kde.org is not static - it is dynamic content, generated via PHP.

- Ben Cooksley
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Sept. 6, 2016, 7:37 a.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
CMakeLists.txt a27c1f2
kded/CMakeLists.txt 1f0613e
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Jan Grulich
2016-09-14 07:05:22 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------

(Updated Zář. 14, 2016, 7:05 dop.)


Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.


Changes
-------

Use a bit different approach suggested by David Edmunson. Instead of opening our own web view using QWebEngine, we can use user's browser directly, with that we have a benefit of stored passwords. I also added a notification which will be opened when a captive portal is detected and user can decide whether to open his browser and log in or not.


Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417


Repository: plasma-nm


Description
-------

Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.


Diffs (updated)
-----

kded/CMakeLists.txt 1f0613e
kded/networkmanagement.notifyrc f95d94b
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41

Diff: https://git.reviewboard.kde.org/r/128707/diff/


Testing
-------

Tested with three different captive portals and it worked perfectly.


Thanks,

Jan Grulich
Kai Uwe Broulik
2016-09-14 08:36:07 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review99159
-----------------------------------------------------------




kded/portalmonitor.cpp (line 35)
<https://git.reviewboard.kde.org/r/128707/#comment66759>

KNotification self-deletes when closed, so you need to create a new notification whenever you want to send it.

(if you need to store it as a member, which I don't think you need to here, use QPointer)



kded/portalmonitor.cpp (line 39)
<https://git.reviewboard.kde.org/r/128707/#comment66758>

Perhaps dialog-password is a better icon.

Also use setIconName instead of pixmap



kded/portalmonitor.cpp (line 41)
<https://git.reviewboard.kde.org/r/128707/#comment66757>

I don't think the average user knows what a "Captive Portal" is.

Perhaps "You need to log in to this network." and use the network name as notifcation title or so.



kded/portalmonitor.cpp (line 58)
<https://git.reviewboard.kde.org/r/128707/#comment66760>

Is it ensured that this notification doesn't constantly pop up, ie. does connectivity only change once I connect to a wifi (turns to "captive") and then again when I am authenticated? Ie. does it not cycle between various states at times?



kded/service.cpp (lines 57 - 60)
<https://git.reviewboard.kde.org/r/128707/#comment66761>

You could have just initialized the members in the class definition above, ie.

class Foo
{
public:
Foo *bar = nullptr;
}

(just mentioning it, was there before this patch)


- Kai Uwe Broulik
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Sept. 14, 2016, 7:05 vorm.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
kded/CMakeLists.txt 1f0613e
kded/networkmanagement.notifyrc f95d94b
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Jan Grulich
2016-09-14 09:01:35 UTC
Permalink
Post by Jan Grulich
kded/portalmonitor.cpp, line 58
<https://git.reviewboard.kde.org/r/128707/diff/6/?file=476842#file476842line58>
Is it ensured that this notification doesn't constantly pop up, ie. does connectivity only change once I connect to a wifi (turns to "captive") and then again when I am authenticated? Ie. does it not cycle between various states at times?
That should be fine. Once you log in the connectivity state should just change to full connectivity. Even if we get notified from NM about changed state while the state is still same, we don't emit connectivityChanged() signal in nm-qt anyway.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review99159
-----------------------------------------------------------
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Zář. 14, 2016, 7:05 dop.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
kded/CMakeLists.txt 1f0613e
kded/networkmanagement.notifyrc f95d94b
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Jan Grulich
2016-09-14 09:02:22 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------

(Updated Zář. 14, 2016, 9:02 dop.)


Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.


Changes
-------

Fix mentioned issues.


Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417


Repository: plasma-nm


Description
-------

Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.


Diffs (updated)
-----

kded/CMakeLists.txt 1f0613e
kded/networkmanagement.notifyrc f95d94b
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41

Diff: https://git.reviewboard.kde.org/r/128707/diff/


Testing
-------

Tested with three different captive portals and it worked perfectly.


Thanks,

Jan Grulich
Kai Uwe Broulik
2016-09-14 10:50:11 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review99161
-----------------------------------------------------------


Fix it, then Ship it!




Make sure to update your commit message to reflect the new way this patch works


kded/networkmanagement.notifyrc (line 779)
<https://git.reviewboard.kde.org/r/128707/#comment66763>

Also a tip. If you just set the IconName properly here, you don't need to explicitly set it programmatically later ;)



kded/portalmonitor.h (line 28)
<https://git.reviewboard.kde.org/r/128707/#comment66764>

Unused



kded/portalmonitor.cpp (line 55)
<https://git.reviewboard.kde.org/r/128707/#comment66765>

You're still using setPixmap here (and then you can also remove KIconLoader include)


- Kai Uwe Broulik
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Sept. 14, 2016, 9:02 vorm.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
kded/CMakeLists.txt 1f0613e
kded/networkmanagement.notifyrc f95d94b
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Sebastian Kügler
2016-09-14 10:55:26 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/#review99162
-----------------------------------------------------------


Ship it!




Good solution, makes total sense.

- Sebastian KÃŒgler
Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------
(Updated Sept. 14, 2016, 9:02 a.m.)
Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.
Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417
Repository: plasma-nm
Description
-------
Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.
Diffs
-----
kded/CMakeLists.txt 1f0613e
kded/networkmanagement.notifyrc f95d94b
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41
Diff: https://git.reviewboard.kde.org/r/128707/diff/
Testing
-------
Tested with three different captive portals and it worked perfectly.
Thanks,
Jan Grulich
Jan Grulich
2016-09-14 11:04:56 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128707/
-----------------------------------------------------------

(Updated Sept. 14, 2016, 11:04 a.m.)


Status
------

This change has been marked as submitted.


Review request for Network Management, Plasma, KDE Usability, and Lamarque Souza.


Changes
-------

Submitted with commit 4ead743a22d0d5b2264cf0aee5b00fa1d77153ea by Jan Grulich to branch master.


Bugs: 365417
http://bugs.kde.org/show_bug.cgi?id=365417


Repository: plasma-nm


Description
-------

Adds portal monitor to our kded module, which checks NetworkManager connectivity. If the value gets changed to NM_CONNECTIVITY_PORTAL (means we are behind a captive portal), then we open a QWebEngineView trying to load "http://kde.org" page which is supposed to be redirected to the captive portal page. Once user logs in and url changes, we re-check the connectivity again and close the web view if we are no longer behind the captive portal.


Diffs
-----

kded/CMakeLists.txt 1f0613e
kded/networkmanagement.notifyrc f95d94b
kded/portalmonitor.h PRE-CREATION
kded/portalmonitor.cpp PRE-CREATION
kded/service.cpp 18ffd41

Diff: https://git.reviewboard.kde.org/r/128707/diff/


Testing
-------

Tested with three different captive portals and it worked perfectly.


Thanks,

Jan Grulich

Loading...