Review Request 121467: Added support for SSTP VPN in plasma-nm (GCI task)

Discussion:

William Boren

2014-12-12 16:23:13 UTC

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121467/
-----------------------------------------------------------

Review request for Network Management and Jan Grulich.

Repository: plasma-nm

Description
-------

Added support for SSTP VPN in plasma-nm (GCI task)

Diffs
-----

plasma-nm-2/vpn/sstp/CMakeLists.txt PRE-CREATION
plasma-nm-2/vpn/sstp/Messages.sh PRE-CREATION
plasma-nm-2/vpn/sstp/sstp.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstp.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/sstpadvanced.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpprop.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpwidget.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstpwidget.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/nm-sstp-service.h PRE-CREATION
plasma-nm-2/vpn/sstp/plasmanetworkmanagement_sstpui.desktop PRE-CREATION

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

Testing
-------

Thanks,

William Boren

Jan Grulich

2014-12-12 17:01:39 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121467/#review71885
-----------------------------------------------------------

Do not use classes like KComboBox, KLineEdit or KDialog, those are removed in KDE Frameworks 5. I also don't think you managed to compile this. You need to compile master branch of libnm-qt first (you can see how to compile it in README). Also your plugin is in plasma-2/vpn subdirectory, it should be directly in vpn directory. Before you submit a new version of your patch try to format it according to https://techbase.kde.org/Policies/Kdelibs_Coding_Style.

plasma-nm-2/vpn/sstp/plasmanetworkmanagement_sstpui.desktop
<https://git.reviewboard.kde.org/r/121467/#comment50098>

You can add yourself here

plasma-nm-2/vpn/sstp/plasmanetworkmanagement_sstpui.desktop
<https://git.reviewboard.kde.org/r/121467/#comment50099>

Same here

plasma-nm-2/vpn/sstp/sstp.cpp
<https://git.reviewboard.kde.org/r/121467/#comment50100>

Use K_PLUGIN_FACTORY_WITH_JSON as other VPN plugins do. This even doesn't compile.

plasma-nm-2/vpn/sstp/sstp.cpp
<https://git.reviewboard.kde.org/r/121467/#comment50101>

This can be removed.

plasma-nm-2/vpn/sstp/sstpauth.ui
<https://git.reviewboard.kde.org/r/121467/#comment50104>

Use QLineEdit instead KLineEdit.

plasma-nm-2/vpn/sstp/sstpprop.ui
<https://git.reviewboard.kde.org/r/121467/#comment50105>

Use QLineEdit instead of KLineEdit.

plasma-nm-2/vpn/sstp/sstpprop.ui
<https://git.reviewboard.kde.org/r/121467/#comment50107>

Use QLineEdit.

plasma-nm-2/vpn/sstp/sstpprop.ui
<https://git.reviewboard.kde.org/r/121467/#comment50109>

Use QLineEdit.

plasma-nm-2/vpn/sstp/sstpprop.ui
<https://git.reviewboard.kde.org/r/121467/#comment50110>

Use QComboBox.

plasma-nm-2/vpn/sstp/sstpprop.ui
<https://git.reviewboard.kde.org/r/121467/#comment50112>

Use QLineEdit.

plasma-nm-2/vpn/sstp/sstpwidget.h
<https://git.reviewboard.kde.org/r/121467/#comment50114>

Use QComboBox instead of KComboBox.

plasma-nm-2/vpn/sstp/sstpwidget.cpp
<https://git.reviewboard.kde.org/r/121467/#comment50115>

Use QDialog instead of KDialog.

plasma-nm-2/vpn/sstp/sstpwidget.cpp
<https://git.reviewboard.kde.org/r/121467/#comment50117>

Coding style.
https://techbase.kde.org/Policies/Kdelibs_Coding_Style

- Jan Grulich

Post by William Boren
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/121467/
-----------------------------------------------------------
(Updated Pro. 12, 2014, 4:23 odp.)
Review request for Network Management and Jan Grulich.
Repository: plasma-nm
Description
-------
Added support for SSTP VPN in plasma-nm (GCI task)
Diffs
-----
plasma-nm-2/vpn/sstp/CMakeLists.txt PRE-CREATION
plasma-nm-2/vpn/sstp/Messages.sh PRE-CREATION
plasma-nm-2/vpn/sstp/sstp.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstp.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/sstpadvanced.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpprop.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpwidget.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstpwidget.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/nm-sstp-service.h PRE-CREATION
plasma-nm-2/vpn/sstp/plasmanetworkmanagement_sstpui.desktop PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/121467/diff/
Testing
-------
Thanks,
William Boren

Jan Grulich

2014-12-12 17:22:48 UTC

Permalink

Post by William Boren

Post by Jan Grulich
Do not use classes like KComboBox, KLineEdit or KDialog, those are removed in KDE Frameworks 5. I also don't think you managed to compile this. You need to compile master branch of libnm-qt first (you can see how to compile it in README). Also your plugin is in plasma-2/vpn subdirectory, it should be directly in vpn directory. Before you submit a new version of your patch try to format it according to https://techbase.kde.org/Policies/Kdelibs_Coding_Style.

One more thing, I just compiled SSTP plugin to NetworkManager to compare your UI and you are missing field for certificate, which is quite important and also advanced configuration tab is missing tab for proxy configuration.

- Jan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121467/#review71885
-----------------------------------------------------------

Lamarque Souza

2015-03-15 19:05:41 UTC

Permalink

Post by William Boren
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/121467/
-----------------------------------------------------------
(Updated Dec. 12, 2014, 4:23 p.m.)
Review request for Network Management and Jan Grulich.
Repository: plasma-nm
Description
-------
Added support for SSTP VPN in plasma-nm (GCI task)
Diffs
-----
plasma-nm-2/vpn/sstp/CMakeLists.txt PRE-CREATION
plasma-nm-2/vpn/sstp/Messages.sh PRE-CREATION
plasma-nm-2/vpn/sstp/sstp.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstp.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/sstpadvanced.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpprop.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpwidget.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstpwidget.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/nm-sstp-service.h PRE-CREATION
plasma-nm-2/vpn/sstp/plasmanetworkmanagement_sstpui.desktop PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/121467/diff/
Testing
-------
Thanks,
William Boren

Lamarque Souza

2015-03-15 19:08:48 UTC

Permalink

Post by William Boren

Post by Lamarque Souza
Any idea why this patch has not been updated in months?

By the way, bug 338521 should be added to this bug's information.

- Lamarque

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121467/#review77535
-----------------------------------------------------------

Post by William Boren
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/121467/
-----------------------------------------------------------
(Updated Dec. 12, 2014, 4:23 p.m.)
Review request for Network Management and Jan Grulich.
Repository: plasma-nm
Description
-------
Added support for SSTP VPN in plasma-nm (GCI task)
Diffs
-----
plasma-nm-2/vpn/sstp/CMakeLists.txt PRE-CREATION
plasma-nm-2/vpn/sstp/Messages.sh PRE-CREATION
plasma-nm-2/vpn/sstp/sstp.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstp.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/sstpadvanced.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpprop.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpwidget.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstpwidget.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/nm-sstp-service.h PRE-CREATION
plasma-nm-2/vpn/sstp/plasmanetworkmanagement_sstpui.desktop PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/121467/diff/
Testing
-------
Thanks,
William Boren

Jan Grulich

2015-03-16 08:15:50 UTC

Permalink

Post by Lamarque Souza

Post by Lamarque Souza
Any idea why this patch has not been updated in months?

By the way, bug 338521 should be added to this bug's information.

Because the student gave it up, he used code of someone else from github and then tried to port it to KF5, but unfortunately he failed (mean that he even didn't manage to compile it). I don't think that he will update this review.

- Jan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121467/#review77535
-----------------------------------------------------------

Post by Lamarque Souza
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/121467/
-----------------------------------------------------------
(Updated Pro. 12, 2014, 4:23 odp.)
Review request for Network Management and Jan Grulich.
Repository: plasma-nm
Description
-------
Added support for SSTP VPN in plasma-nm (GCI task)
Diffs
-----
plasma-nm-2/vpn/sstp/CMakeLists.txt PRE-CREATION
plasma-nm-2/vpn/sstp/Messages.sh PRE-CREATION
plasma-nm-2/vpn/sstp/sstp.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstp.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/sstpadvanced.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpprop.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpwidget.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstpwidget.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/nm-sstp-service.h PRE-CREATION
plasma-nm-2/vpn/sstp/plasmanetworkmanagement_sstpui.desktop PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/121467/diff/
Testing
-------
Thanks,
William Boren

Lamarque Souza

2015-03-16 13:06:23 UTC

Permalink

Post by Lamarque Souza

Post by Lamarque Souza
Any idea why this patch has not been updated in months?

By the way, bug 338521 should be added to this bug's information.
Because the student gave it up, he used code of someone else from github and then tried to port it to KF5, but unfortunately he failed (mean that he even didn't manage to compile it). I don't think that he will update this review.

So we can drop this review request, right?

- Lamarque

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121467/#review77535
-----------------------------------------------------------

Post by Lamarque Souza
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/121467/
-----------------------------------------------------------
(Updated Dec. 12, 2014, 4:23 p.m.)
Review request for Network Management and Jan Grulich.
Repository: plasma-nm
Description
-------
Added support for SSTP VPN in plasma-nm (GCI task)
Diffs
-----
plasma-nm-2/vpn/sstp/CMakeLists.txt PRE-CREATION
plasma-nm-2/vpn/sstp/Messages.sh PRE-CREATION
plasma-nm-2/vpn/sstp/sstp.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstp.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/sstpadvanced.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/sstpauth.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpprop.ui PRE-CREATION
plasma-nm-2/vpn/sstp/sstpwidget.h PRE-CREATION
plasma-nm-2/vpn/sstp/sstpwidget.cpp PRE-CREATION
plasma-nm-2/vpn/sstp/nm-sstp-service.h PRE-CREATION
plasma-nm-2/vpn/sstp/plasmanetworkmanagement_sstpui.desktop PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/121467/diff/
Testing
-------
Thanks,
William Boren

Jan Grulich

2015-03-16 14:21:34 UTC

Permalink

Post by Lamarque Souza

Post by Lamarque Souza
Any idea why this patch has not been updated in months?

Yes, but I think that only the submitter can discard it or someone with admin rights.

- Jan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121467/#review77535
-----------------------------------------------------------