Arfrever,
Do you still veto this patch? *Everyone* who replied to the thread
(except you) likes the idea, and some replies (e.g. Justin's) challenged
your veto -- and you have not addressed them.
Could you retract your veto and try again to convince people that you
are right about this patch -- this time, *without* using your Full
Committer powers?
Please remember that veto is an extreme measure and should be used
sparingly.
Daniel
Martin Furter wrote on Sat, 31 May 2008 at 18:47 +0200:
> 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
Received on 2008-06-06 06:42:09 CEST