Re: [RFC,PATCH] Port libsvn_auth_kwallet to KDE3.

On Fri, 30 May 2008, Daniel L. Rall wrote:

> On Thu, 29 May 2008, Martin Furter wrote:
>
>>
>> Hello
>>
>> Here is an untested Patch to get libsvn_auth_kwallet to compile with KDE3
>> and QT3. I'll test with kwallet as soon as I've found out how to use it...
>>
>> I send it now to get some general comments about the patch.
>
> +1 on the idea.
>
>> [[[
>> Port libsvn_auth_kwallet to KDE3.
>>
>> * subversion/libsvn_auth_kwallet/kwallet.cpp
>> (kwallet_password_get): Port to KDE3.
>> (kwallet_password_set): Ditto.
>>
>> * configure.ac
>> Add support for KDE3 and QT3. Define SVN_HAVE_QT3 if using QT3.
>> ]]]
>
>> Index: subversion/libsvn_auth_kwallet/kwallet.cpp
>> ===================================================================
>> --- subversion/libsvn_auth_kwallet/kwallet.cpp (revision 31484)
>> +++ subversion/libsvn_auth_kwallet/kwallet.cpp (working copy)
>> @@ -32,8 +32,15 @@
>>
>> #include "svn_private_config.h"
>>
>> +#ifdef SVN_HAVE_QT3

I guess I should rename this define to SVN_HAVE_KDE3.

>> +#include <qstring.h>
>> +#include <qwidget.h>
>> +/* TODO: where is ki18n in KDE3? */
>> +#define ki18n(x) x
>
> How about using klocale.h's i18n(), which returns a QString (which appears
> to have a char * cast that calls its ascii() method)?

Thanks, done.

>> +#else
>> #include <QtCore/QString>
>> #include <QtGui/QWidget>
>> +#endif
>>
>> #include <kapplication.h>
>> #include <kcmdlineargs.h>
...
>> @@ -129,14 +152,24 @@
>> return FALSE;
>> }
>>
>> +#ifdef SVN_HAVE_QT3
>> KCmdLineArgs::init(1,
>> (char *[1]) { "svn" },
>> "Subversion",
>> "subversion",
>> + ki18n("Version control system"),
>> + SVN_VER_NUMBER,
>> + true);
>> +#else
>> + KCmdLineArgs::init(1,
>> + (char *[1]) { "svn" },
>> + "Subversion",
>> + "subversion",
>> ki18n("Subversion"),
>> SVN_VER_NUMBER,
>> ki18n("Version control system"),
>> KCmdLineArgs::CmdLineArgKDE);
>> +#endif
>
> Factor this large duplicate section (repeated above) into a helper routine.

See below.

> ...
>> --- configure.ac (revision 31484)
>> +++ configure.ac (working copy)
> ...
>> +dnl KWallet3 -------------------
>> +
>> +AC_ARG_WITH(kwallet3,
>> + [AS_HELP_STRING([[--with-kwallet3[=PATH]]],
>> + [Enable use of KWallet (KDE 43) for auth credentials])],
>
> 43?

42 plus taxes ;)
Fixed too.

>> + [with_kwallet3="$withval"],
>> + [with_kwallet3=no])
>> +

Looking a bit closer at the code I saw that 95% of those two functions are
identical so much more than that call to KCmdLineArgs::init() can be
fatored out.

To do that I created a new class with all the needed variables as
members.. Its static init function is just to make sure that
KCmdLineArgs::init() is called before an instance of KApplication is
created.

The code still does the same thing with 2 changes:
1. wallet->createFolder() returns true for success and false if it failed,
    so another check with wallet->hasFolder() seems unnecessary.
2. KWallets documentation sais that the wallet has to be deleted after use
    so I delete it in the destrcutor of the new class.

[[[
Move the get/set code of libsvn_auth_kwallet into a class, add support for
KDE3 and fix a memleak.

* subversion/libsvn_auth_kwallet/kwallet.cpp
   (SvnKWallet): New class containing the code of kwallet_password_get
     and kwallet_password_set with support for KDE3 added.
   (kwallet_password_get): Use SvnKWallet to do the work.
   (kwallet_password_set): Ditto.

* configure.ac
   Add support for KDE3 and QT3. Define SVN_HAVE_QT3 if using QT3.
]]]

Now the commit message sounds like it should be split into 3 separate
patches.
What do you think?

Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org