KDE-GTK-Config to KDEReview

[Aleix Pol]

On Wed, Nov 7, 2012 at 12:49 AM, Pino Toscano <pino at kde.org> wrote:
> 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
Is that really a big problem? I don't really like the jumping from and
to Q/K classes.

>
> - 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.
I changed all Gtk to GTK

>
> - 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 removed the separator. About Behaviour, I had Fine Tuning in the
past and some people said they preferred it this way. I'd prefer not
to change it back.

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

>
> - QMessageBox -> KMessageBox
done

>
> - the extension checks in DialogInstaller::installTheme/installThemeIcon
> are a bit ugly, wouldn't be better checking the mimetype of the file
> (even just doing the filename-only check) and allow application/x-
> compressed-tar files?
done, although I'm unsure I did it right

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

>
> - port AppearanceGTK3::defaultConfigFile to
> KStandardDirs::localxdgconfdir
done

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

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

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

>
> - what Installer does would just be matter of KArchive::copyTo...
right, changed.

>
> - you don't need FindMSGFMT.cmake, since cmake and kdelibs have
> FindGettext.cmake
removed, we weren't using it anymore.

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

>
> I think that's enough for now...
>
> --
> Pino Toscano