Review Request: [RFC] Import VPN connection

[Rajeesh K Nambiar]

On Wed, May 25, 2011 at 12:33 AM, Lamarque Vieira Souza
<lamarque at gmail.com> wrote:
> Em Tuesday 24 May 2011, Rajeesh K Nambiar escreveu:
>
>> > On May 24, 2011, 5:44 p.m., Lamarque Vieira Souza wrote:
>
>> > > settings/config/manageconnectionwidget.cpp, line 519
>
>> > >
>> > > <http://git.reviewboard.kde.org/r/101419/diff/3/?file=18004#file18004li
>
>> > > ne519>
>
>> > >
>
>> > > Shouldn't you also emit a notification or a popup warning the user
>
>> > > that the import failed?
>
>>
>
>> Yes, certainly so. But I'm not sure how exactly to do it. Should I be
>
>> modifying libs/service/notificationmanager.cpp? Could you suggest/point to
>
>> how to implement this?
>
> No, notificationmanager.cpp is kind of passive, it always waits for
> something to notify but only from the networkinterface. In your case you
> should create the KNotification object and emit it yourself, something like:
>
> #include <KNotification>
>
> #include "../libs/service/events.h" // maybe you will need to change this
> path
>
> K_GLOBAL_STATIC_WITH_ARGS(KComponentData, s_networkManagementComponentData,
> ("networkmanagement", "networkmanagement",
> KComponentData::SkipMainComponentRegistration))
>
> static const int m_iconSize = 48;
>
> //end of global variables
>
> KNotification::event(Event::VpnCertificateImportFailed, i18nc("@info:status
> Notification when importing VPN certificate failed", "Certificate import
> failed"), QPixmap(), 0, KNotification::CloseOnTimeout,
> *s_networkManagementComponentDa
>
> ta)->sendEvent();
>
> Then in ../libs/service/events.h you declare
> Event::VpnCertificateImportFailed and in ../libs/service/events.cpp define
> it:
>
> const QString Event::VpnCertificateImportFailed =
> QLatin1String("vpncertificateimportfailed");
>
> In networkmanagement.notifyrc you add the info about the
> vpncertificateimportfailed notification:
>
> [Event/vpncertificateimportfailed]
>
> Name=VPN certificate import failed
>
> Comment=Importing of VPN certificate has failed

Looking at manageconnectionwidget.cpp, deleteClicked(), I was
wondering - wouldn't it be simpler and in line with other places to
use KMessageBox about VPN connection import failure; also with
cisco-decrypt errors?

>
>> > On May 24, 2011, 5:44 p.m., Lamarque Vieira Souza wrote:
>
>> > > vpnplugins/vpnc/vpnc.cpp, line 139
>
>> > >
>> > > <http://git.reviewboard.kde.org/r/101419/diff/3/?file=18014#file18014li
>
>> > > ne139>
>
>> > >
>
>> > > Please remove space at line start. Also, you do not insert the
>
>> > > password into the connection map if the executable failed to run.
>
>> > > I guess the VPN connection is not going to work in that case,
>
>> > > right? If so I think you should abort the import process. Think of
>
>> > > this function as a (SQL) transation: or everything works or the
>
>> > > entire import process is aborted.
>
>>
>
>> VPNC user password is almost uninteresting regarding the pcf file. Most of
>
>> the time it will be empty and the user has to manually input. The
>
>> important bit is Group Password/Encrypted Group Password. IMHO, it would
>
>> be better not to abort the import if password decryption fails, but to
>
>> inform the user; so that he can manually rectify it or contact the
>
>> administrator. Further, it is very rare for cisco-decrypt to fail, and
>
>> guaranteed that it will be installed since vpnc package is required
>
>> dependency for NetworkManager-vpnc which is required for
>
>> kde-plasma-networkmanagement-vpnc package.
>
> Well, NetworkManager-vpnc is not always installed. I for example did not
> have it installed when I first tried to use VPN in my notebook. It took me a
> while to figure out I should have it installed before I could use VPN.
> Warning about missing dependencies is one thing we could improve in Plasma
> NM in the future.
>
> Ok then, no need to abort the import, just add a notification about it.
>
>> > On May 24, 2011, 5:44 p.m., Lamarque Vieira Souza wrote:
>
>> > > settings/config/manageconnectionwidget.cpp, line 528
>
>> > >
>> > > <http://git.reviewboard.kde.org/r/101419/diff/3/?file=18004#file18004li
>
>> > > ne528>
>
>> > >
>
>> > > If this button is visible you should also emit a notification or a
>
>> > > popup warning that this has not been implemented yet. Or just hide
>
>> > > the export button.
>
>>
>
>> I will try to implement the functionality in next iteration; and if it
>
>> looks very difficult then we can simply hide it.
>
> Ok.
>
> --
>
> Lamarque V. Souza
>
> http://www.geographicguide.com/brazil.htm
>
> Linux User #57137 - http://counter.li.org/
>
> http://planetkde.org/pt-br



-- 
Cheers,
Rajeesh
http://rajeeshknambiar.wordpress.com