> On July 6, 2012, 9:08 p.m., Jarosław Staniek wrote: > > libs/main/KoDetailsPane.h, line 36 > > <http://git.reviewboard.kde.org/r/105428/diff/2/?file=71401#file71401line36> > > > > If this is in public API used outside of the lib, I would suggest > > changing to static method (the same applies to the previewExtent attr). > > > > OR: why the 64 constant itself needs to be in .h, not in .cpp? > > > > If you need the value only in the library (classes that inherit from > > KoDetailsPane) then please move it to protected area (still, changing to > > const method is good idea). > > > > Explanation: > > > > http://stackoverflow.com/questions/4470802/exporting-constants-from-a-dll?answertab=active#tab-top > > > > Friedrich W. H. Kossebau wrote: > These extent values are currently only used in the library, and only by > KoDetailsPane and subclasses, not by any consumers of those classes. > So moving to protected might be a good idea (but no need to backport, > or?). But then, see below. > > About turning the constants into static methods: does the static const > integer end up in a real memory location? I always thought of "(static) const > int x = 1" as basically a "#define x 1" substitute with advantage of type > checking. I see the difference With strings, but for types which match > register sizes I thought they would be just use as defined const values, in > any case and not just with optimization flags. So that is not true? > Are enums the only way to get that? Anyone knows about that for sure and > can give a reference (while I try to search an answer myself)? > > Jarosław Staniek wrote: > OK, so moving to protected (and removing initialization from the header) > is good idea, no need to backport. > > About turning the constants into static methods: it's all related to > compatibility with msvc compilers, even when simple types are involved; if I > look at Qt API - there are no such use cases.
Okay, will push a fix then. But using enums, to get the const value terms, like done for StdSizes of KIconLoader, see http://api.kde.org/4.8-api/kdelibs-apidocs/kdeui/html/classKIconLoader.html#pub-types So fine with the following patch? Or should I better have the first letter uppercase, i.e. IconExtent and PreviewExtent? @@ -34,9 +34,6 @@ class KoDetailsPane : public QWidget, public Ui_KoDetailsPaneBase Q_OBJECT public: - static const int iconExtent = 64; - static const int previewExtent = 128; - KoDetailsPane(QWidget* parent, const KComponentData &_componentData, const QString& header); virtual ~KoDetailsPane(); @@ -67,6 +64,12 @@ protected slots: void changePalette(); +protected: + enum Extents { + iconExtent = 64, + previewExtent = 128 + }; + - Friedrich W. H. ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105428/#review15479 ----------------------------------------------------------- On July 5, 2012, 1:18 p.m., Friedrich W. H. Kossebau wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/105428/ > ----------------------------------------------------------- > > (Updated July 5, 2012, 1:18 p.m.) > > > Review request for Calligra. > > > Description > ------- > > Real reason for this patch is that KFileItem::pixmap(...) is broken for items > constructed with KFileItem(mode_t, mode_t, KUrl, bool) and no other mimetype > related call before the one to ::pixmap. It will always return a pixmap with > the "unknown" icon. > You can see this for files for which there is no preview created (for > whatever reason). > > But the patch fixes also the size mismatch between the fetched previews > (200x200) and the usual ODF preview of 128x128 as well as that the preview > just from the icon is set in size 128. And makes the code shorter. > > Also load the big previews only on-demand, not by default. Could spare up to > ~500 kB (10 files * (128*128 - 64*64) * 4 bytes) in theory, but I did not > measure what the real effect is. > > Also using KIcon instead of setting an own deep copy of the pixmap per each > KoFileListItem should spare even more memory, due to KIcon reusing existing > icon pixmaps. > > Okay to backport to 2.5 as well? > > > Diffs > ----- > > libs/main/KoDetailsPane.h 6da6133 > libs/main/KoDetailsPane.cpp 8aa5965 > libs/main/KoRecentDocumentsPane.h 98baeb8 > libs/main/KoRecentDocumentsPane.cpp 521c7e6 > libs/main/KoTemplatesPane.cpp cc76c05 > > Diff: http://git.reviewboard.kde.org/r/105428/diff/ > > > Testing > ------- > > Tried the recent template pane at least in Krita, Words, Sheets > > > Thanks, > > Friedrich W. H. Kossebau > >
_______________________________________________ calligra-devel mailing list calligra-devel@kde.org https://mail.kde.org/mailman/listinfo/calligra-devel
