KDE-GTK-Config to KDEReview

[Pino Toscano]

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.

> > - 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? :)

> > - QMessageBox -> KMessageBox
> 
> done

Still there...?

> > - 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.

> > - 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.
 
> > - 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?

> > - 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
[...]? :)

- 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.

- instead of have ThreadErase::run execute a KIO job synchronously, why 
not just rely on the async result of the job?

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

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20121107/5a70c252/attachment.sig>