KDE-GTK-Config to KDEReview

[Aleix Pol]

On Wed, Nov 7, 2012 at 9:45 PM, Pino Toscano <pino at kde.org> wrote:

> Hi,
>
> Alle mercoledì 7 novembre 2012, Aleix Pol ha scritto:
> > On Wed, Nov 7, 2012 at 12:49 AM, Pino Toscano <pino at kde.org> wrote:
> > > Alle lunedì 5 novembre 2012, Aleix Pol ha scritto:
> > >> Some months ago I pushed this project quite intensively so that
> > >> right now we have a stable KCM for editing the GTK settings. This
> > >> is being used already in some distributions, so I think it should
> > >> be moved to extragear/base.
> > >>
> > >> So here I open the process for kde:kde-gtk-config to move in
> > >> extragear. Any thoughts?
> > >
> > > Sure, here are mine:
> > >
> > > - please convert the two QDialog in .ui files to QWidget and put
> > > them in KDialog, so they have a simplier layout and they don't
> > > have to relayout (and retranslate) buttons
> >
> > Is that really a big problem? I don't really like the jumping from
> > and to Q/K classes.
>
> Where would be this "jumping from and to"?
> Using KDialog for dialogs makes them follow the KDE style and settings,
> and avoid retranslating standard texts.
>
Ok, not using QDialog anymore...


>
> > > - I've just simplified some ui strings, but there are still few
> > > more which use the qt rich text format, forcing a font face and
> > > size; please apply the font enlarging at runtime on the font
> > > active at that time
> >
> > Yep..
>
> Meaning it is going to be fixed? :)
>
Yes, it should be fixed. I left one in gui.ui which is in a tooltip and I
think it's ok like that.
It's only like this because we wanted to have links in it, if it's wrong,
we can change that of course.and I'll


>
> > > - QMessageBox -> KMessageBox
> >
> > done
>
> Still there...?
>
Not anymore.


>
> > > - related to the above, there's the job of ThreadAnalisysTheme::run
> > > and ThreadAnalisysThemeIcon::run, which currently spawn `tar` to
> > > extract the archive... what about porting them to KArchive (so you
> > > can potentially rely on all the formats supported by that)?
>
> `tar` usage still there.
>
not anymore!


>
> > > - AppearanceGTK2::installedThemes and
> > > AppearanceGTK3::installedThemes need to be ported to
> > > KStandardDirs, yes
> >
> > done
>
>   foreach(const QString& themesDir, KGlobal::dirs()->findDirs \
>     ("xdgdata-apps", "../themes")) {
> This is a bit ich-y, but at the moment I'm not sure how to do it
> properly.
>
I have no idea either, if anybody has an idea, please don't hesitate to
suggest it :).


>
> > > - port AppearanceGTK3::defaultConfigFile to
> > > KStandardDirs::localxdgconfdir
> >
> > done
>
> I don't think localxdgconfdir returns an empty directory.
>
> > > - IconThemesModel really needs to use KStandardDirs instead of
> > > looking for XDG envvars and looking for paths on its own... or,
> > > even, just use KIconLoader/KIconTheme...
> >
> > true, changed
>
> Why not just use KIconLoader/KIconTheme to simplify even more?
>
Because the code we have now just works and I don't have much experience
with KIcon* classes.
I know it's a lousy explanation, but I think that it could become a waste
of time...


>
> > > - the differences between gtkproxies/preview.c and
> > > gtk3proxies/preview3.c seem minimal; is there really no way to have
> > > just one source with at most 1-2 «#if GTK_IS_VERSION(...)» blocks?
> > > also, doesn't glib/gtk offer some kind of functions to parse
> > > command line arguments (similar to getopt, KCmdLineArgs, etc)?
> >
> > Probably, on the other hand, it just works now and I don't think it
> > would add much value to spend time improving that, as it's only for
> [...]? :)
>
If nobody has a big problem with having the preview codes duplicated, I'd
prefer to leave it like that.


>
> - there are icons in .ui files which are badly set, see:
> $ grep '<pixmap>' src/ui/*
> so better remove them from .ui files and set them via code.
>
removed. It was already being set from the code.


>
> - instead of have ThreadErase::run execute a KIO job synchronously, why
> not just rely on the async result of the job?
>
Well it's a thread and the thread needs to wait for the job to finish for
finishing the thread itself, so it would end up being synchronous anyway.

I ported the EraseThread to KJob anyway, because it made sense there.


>
> - AbstractAppearance::hasProperty should be const, and just check
> QString::isEmpty?
>
True, fixed


>
> --
> Pino Toscano
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20121108/17a7b05e/attachment.htm>