Review Request 123465: Add support for SSH VPN plugin

[Lamarque Souza]

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



vpn/ssh/sshadvanced.ui (line 32)
<https://git.reviewboard.kde.org/r/123465/#comment54184>

    Max port value is 65535



vpn/ssh/sshauth.h (line 38)
<https://git.reviewboard.kde.org/r/123465/#comment54179>

    This commented code block can be removed.



vpn/ssh/sshauth.h (line 42)
<https://git.reviewboard.kde.org/r/123465/#comment54180>

    Same here.



vpn/ssh/sshauth.cpp (line 35)
<https://git.reviewboard.kde.org/r/123465/#comment54181>

    Code style, it should be one variable per line:
    
    : SettingWidget(setting, parent)
    , d_ptr(new ...)



vpn/ssh/sshwidget.cpp (line 161)
<https://git.reviewboard.kde.org/r/123465/#comment54182>

    This can be simplified to:
    
    if (type & NetworkManager::Setting::AgentOwned || type == NetworkManager::Setting::None)
    
    NetworkManager::Setting::None == 0, so "type & NetworkManager::Setting::None" is always false and using "type == NetworkManager::Setting::None" is more explicit then "!type" in my oppinion.



vpn/ssh/sshwidget.cpp (line 256)
<https://git.reviewboard.kde.org/r/123465/#comment54183>

    Code style: I prefer to use switch() in situations like this one.



vpn/ssh/sshwidget.cpp (line 349)
<https://git.reviewboard.kde.org/r/123465/#comment54185>

    You should use SettingWidget::EnumPasswordStorageType::{Store,AlwaysAsk,NotRequired} as index for all secret flags comboboxes throughout Plasma NM. Here you sorted them as AgentOwned, NotRequired, AlwaysAsk, but in other comboboxes in Plasma NM the order is AgentOwned, AlwaysAsk, NotRequired. If you use SettingWidget::EnumPasswordStorageType::{Store,AlwaysAsk,NotRequired} the order will be same for all comboboxes and there are a lot of them in Plasma NM.


- Lamarque Souza


On April 22, 2015, 1:06 p.m., Jan Grulich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123465/
> -----------------------------------------------------------
> 
> (Updated April 22, 2015, 1:06 p.m.)
> 
> 
> Review request for Network Management, Lukáš Tinkl and Lamarque Souza.
> 
> 
> Bugs: 341070
>     http://bugs.kde.org/show_bug.cgi?id=341070
> 
> 
> Repository: plasma-nm
> 
> 
> Description
> -------
> 
> This patch adds support for SSH VPN plugin (according to nm-connection-editor).
> 
> 
> Diffs
> -----
> 
>   vpn/ssh/sshwidget.ui PRE-CREATION 
>   vpn/ssh/sshwidget.h PRE-CREATION 
>   vpn/ssh/sshwidget.cpp PRE-CREATION 
>   vpn/ssh/sshauth.ui PRE-CREATION 
>   vpn/ssh/sshauth.cpp PRE-CREATION 
>   vpn/ssh/sshadvanced.ui PRE-CREATION 
>   vpn/ssh/sshauth.h PRE-CREATION 
>   vpn/ssh/ssh.cpp PRE-CREATION 
>   vpn/ssh/ssh.h PRE-CREATION 
>   vpn/ssh/nm-ssh-service.h PRE-CREATION 
>   vpn/ssh/plasmanetworkmanagement_sshui.desktop PRE-CREATION 
>   vpn/ssh/Messages.sh PRE-CREATION 
>   vpn/ssh/CMakeLists.txt PRE-CREATION 
>   vpn/CMakeLists.txt 714fff0 
>   libs/editor/simpleipv4addressvalidator.h 9a4e270 
>   libs/editor/simpleipv6addressvalidator.h 934dc99 
> 
> Diff: https://git.reviewboard.kde.org/r/123465/diff/
> 
> 
> Testing
> -------
> 
> I tried to create a new SSH VPN connection in both (kde5-)nm-connection-editor and re-open it in another editor to check whether all values were stored/loaded properly.
> 
> 
> Thanks,
> 
> Jan Grulich
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20150422/f9cb2489/attachment-0001.html>