Re: KGuiAddons Review
On Tuesday 04 March 2014 18:46:53 John Layt wrote: > Hi, > > Here's my first pass through KGuiAddons, focussing on the public api. > > KColorCollection > - Should probably become a QSharedDataPointer Maybe. But OTOH it's just a QList, two QStrings and a an enum. > KWorkdWrap > - "// KDE5 TODO: return a value, not a pointer, and use QSharedDataPointer." Done. > KModifierKeyInfo > - Generally looks OK > - Has lots of "bool isKeyPressed(Qt::Key)" style methods and > keyPressed(Qt::Key, bool) style signals when perhaps a KeyState enum would > be better? If it's just false/true it seems fine to me, the meaning is in the method name (so this is not the "API boolean trap"). > - Uses X11 / XKB / XCB, will need Wayland backend eventually? Yes, and Mac, and Windows. > - Perhaps really belongs in Qt? Yes, but this requires providing implementations for all tier1 platforms in one go... [snip] -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE, in particular KDE Frameworks 5 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: KGuiAddons Review
On Tue, March 4, 2014 18:46:53 John Layt wrote: > KImageCache > KSharedPixmapCacheMixin > KLocalImageCacheImplementation > - Looks OK > - KImageCache not exported, but KLocalImageCacheImplementation is, some > CMake magic involved? No CMake. :) KImageCache depends on KSharedDataCache from KCoreAddons but we didn't want to make a hard dep on KCoreAddons from KGuiAddons. So instead the differences between KImageCache are coded into a class KLocalImageCacheImplementation (which is exported as you noted). The API of KSharedDataCache that is needed is replaced by a template class KSharedPixmapCacheMixin. KImageCache itself then becomes a macro expanding to KSharedPixmapCacheMixin, which is generated by the compiler and thus doesn't need an export (this despite being an internal class marked as @internal). Doing it this way allows a sort of "weak reference" to KSharedDataCache without having to define KSharedDataCache. It might even be possible to forward-declare KSharedDataCache and use a typedef instead but I didn't even do the porting work here and that didn't occur to me when I was reviewing the patch a few months back. Regards, - Michael Pyne ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
KGuiAddons Review
Hi, Here's my first pass through KGuiAddons, focussing on the public api. KColorCollection - Should probably become a QSharedDataPointer KWorkdWrap - "// KDE5 TODO: return a value, not a pointer, and use QSharedDataPointer." KModifierKeyInfo - Generally looks OK - Has lots of "bool isKeyPressed(Qt::Key)" style methods and keyPressed(Qt::Key, bool) style signals when perhaps a KeyState enum would be better? - Uses X11 / XKB / XCB, will need Wayland backend eventually? - Perhaps really belongs in Qt, or somewhere else? KImageCache KSharedPixmapCacheMixin KLocalImageCacheImplementation - Looks OK - KImageCache not exported, but KLocalImageCacheImplementation is, some CMake magic involved? KColorMimeData KFontUtils KIconUtils - Looks OK KColorUtils - Looks OK - Qt should probably get some of these methods? KDateValidator - Looks OK - Qt should probably have one, QTime as well? And the usual documentation improvements of course. Otherwise looks in good shape. Cheers! John. ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
