On Wed, Nov 7, 2012 at 9:45 PM, Pino Toscano <p...@kde.org> wrote: > Hi, > > Alle mercoledì 7 novembre 2012, Aleix Pol ha scritto: > > On Wed, Nov 7, 2012 at 12:49 AM, Pino Toscano <p...@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 >