KDE-GTK-Config to KDEReview

[Pino Toscano]

Hi,

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

- all around in UI strings there is a spelling confusion between "GTK" 
and "Gtk"... and actually, it'd seem that the proper spelling is "GTK+", 
but I guess that could be too technical to users? anyway, please uniform

- in gui.ui, there is a groupbox labelled "Behaviour", but the options 
would seem more "look" than behaviour; my suggestion would be to relabel 
it to "fine tuning" (similar to what is used in the style kcm) or 
something similar, and remove also the ugly separator between the combo 
and the checkboxes

- 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

- QMessageBox -> KMessageBox

- the extension checks in DialogInstaller::installTheme/installThemeIcon 
are a bit ugly, wouldn't be better checking the mimetype of the file 
(evne just doing the filename-only check) and allow application/x-
compressed-tar files?

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

- AppearanceGTK2::installedThemes and AppearanceGTK3::installedThemes 
need to be ported to KStandardDirs, yes

- port AppearanceGTK3::defaultConfigFile to 
KStandardDirs::localxdgconfdir

- there are #include <QtCore> / #include <QtGui> global includes which 
should go in favour of #include's of actually used classes

- in GTKConfigKCModule::defaults there is:
  icons << KIconTheme(KIconTheme::current()).name() << "GNOME";
why not just KIconLoader::global()->theme()->name()?

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

- what Installer does would just be matter of KArchive::copyTo...

- you don't need FindMSGFMT.cmake, since cmake and kdelibs have 
FindGettext.cmake

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

I think that's enough for now...

-- 
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/b400e43b/attachment.sig>