Review Request: Add KMetaDataWidget, KMetaDataModel and KMetaDataConfigurationDialog

Mon Mar 15 22:42:12 GMT 2010

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3277/#review4507
-----------------------------------------------------------

Wow, thanks, with that we are one step nearer to KDE 3 parity :)

I have only checked the two public headers so far, and I have some cosmetic comments as well as comments about possible API changes.

Just curious, does this concept require the Nepomuk database, or is it possible to get the meta information for non-indexed files (e.g. via strigi directly)?

trunk/KDE/kdelibs/kfile/kmetadatamodel.h
<http://reviewboard.kde.org/r/3277/#comment3968>

    Use <QtModule/QClass>, see http://techbase.kde.org/Policies/Kdelibs_Coding_Style#Qt_Includes

    This is especially important for public (installed) headers.

trunk/KDE/kdelibs/kfile/kmetadatamodel.h
<http://reviewboard.kde.org/r/3277/#comment3955>

    This include file isn't installed anywhere.

trunk/KDE/kdelibs/kfile/kmetadatamodel.h
<http://reviewboard.kde.org/r/3277/#comment3956>

    Hm, and I thought this was a QAbstractItemModel... maybe just KMetaData? KMetaDataProvider? KFileMetaData (since it is bound to KFileItems)? Opinions?

trunk/KDE/kdelibs/kfile/kmetadatamodel.h
<http://reviewboard.kde.org/r/3277/#comment3959>

    I don't understand this "group" concept. As far as I can see, this is merely some sort of sorting indicator, to get important meta data shown first. Would a simple int not be enough in this case?

    These can be compared faster, and don't require an allocation.

    Important groups can be assigned low numbers such as 100, 200, 300, etc., so there is still room to insert a group later... Some constants could be declared to get actual meaning for the group:

    enum Importance {
       RequiredData = 100, // name
       VeryImportantData = 200, // size/date
       ImportantData = 300,  // author&stuff
       LessImportantData = 400,  // bla...
       AdditionalData = 1000, // e.g. camera lens type
       OptionalData = 2000, // bla...
       DebuggingData = 10000 // e.g. camera firmware version
    }

    This way a programmer can limit meta data depending on some selected importance.

    Also custom groups could return less than 100 so that they are always sorted before the data Nepomuk provides.

trunk/KDE/kdelibs/kfile/kmetadatamodel.h
<http://reviewboard.kde.org/r/3277/#comment3952>

    Rejected.

    Having a virtual function in a public class declared depending on the value of HAVE_NEPOMUK is a cause for a binary incompatibility. Think about code that comes from build services vs. code that the user has compiled herself.

trunk/KDE/kdelibs/kfile/kmetadatamodel.h
<http://reviewboard.kde.org/r/3277/#comment3957>

    Q_SIGNALS: in public headers.

trunk/KDE/kdelibs/kfile/kmetadatamodel.h
<http://reviewboard.kde.org/r/3277/#comment3958>

    const missing.

trunk/KDE/kdelibs/kfile/kmetadatawidget.h
<http://reviewboard.kde.org/r/3277/#comment3963>

    There could be a bridge for widgets that operate on QVariant (ideally via the "property" concept).

    I know Nepomuk provides more types there, but many existing "data delegates" operate on QVariant.

    At least something to consider for the future (i.e. check, if the current design would allow such usage).

trunk/KDE/kdelibs/kfile/kmetadatawidget.h
<http://reviewboard.kde.org/r/3277/#comment3960>

    a file items -> file items

trunk/KDE/kdelibs/kfile/kmetadatawidget.h
<http://reviewboard.kde.org/r/3277/#comment3966>

    "data such as the comment, tags, or rating" sounds better. I had to read the sentence twice to get what you mean.

trunk/KDE/kdelibs/kfile/kmetadatawidget.h
<http://reviewboard.kde.org/r/3277/#comment3965>

    With KMetaDataModel thinking in terms of "groups", why does this widget have separate types?

    Shouldn't it be possible to filter by groups, or (if my suggestion about the "int" groups is used) by using a maximum importance group shown?

    A future implementation could hide some additional values in an "expander" type widget.

trunk/KDE/kdelibs/kfile/kmetadatawidget.h
<http://reviewboard.kde.org/r/3277/#comment3967>

    I have not checked the implementation, but is it possible for this class to pass the valueWidget that it used for this value? Otherwise the subclass has to remember all the widgets that it returned to valueWidget().

Please also have a look at http://doc.trolltech.com/4.7-snapshot/qtmultimedia.html#MetaData-enum for meta data that Qt is going to provide. Maybe this can be used to get some inspiration about what "groups" we can offer. But I guess Nepomuk already defines such groups?

- Christoph

On 2010-03-14 01:49:48, Peter Penz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3277/
> -----------------------------------------------------------
> 
> (Updated 2010-03-14 01:49:48)
> 
> 
> Review request for kdelibs, Sebastian Trueg, David Faure, and Aurélien Gâteau.
> 
> 
> Summary
> -------
> 
> The patch adds KMetaDataWidget, KMetaDataModel and KMetaDataConfigurationDialog as public classes to kdelibs/kfile. The KMetaDataWidget allows an application in an easy way to show meta data of a file (or several files). The widget also allows to change meta data like tags, comments and rating (see http://enzosworld.gmxhome.de/temp/metadatawidget.png or attached screenshot). KMetaDataConfigurationDialog allows to configure which meta tags should be hidden/shown. The classes also work without Nepomuk (and show only very basic meta data like size, permissions, ...). It is possible for applications to add custom meta data if wanted (including widgets to manipulate this meta data).
> 
> The classes have been used by Dolphin internally until now and have originally been written by Sebastian Trüg. After the request from Tom Albers and Oliver Heidbüchel to integrate the widget also in Mailody/Okular I've adjusted the classes to get them ready for a kdelibs-integration. I'd also like to to adjust KPropertiesDialog later to use this widget.
> 
> I'd ask mainly to look at the files kfile/kmetadatawidget.h, kfile/kmetadatamodel.h and kfile/kmetadataconfigurationdialog.h, the other APIs are internal.
> 
> Thanks!
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kfile/kcommentwidget_p.h PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/config-nepomuk.h.cmake PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/kcommentwidget.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/CMakeLists.txt 1102931 
>   trunk/KDE/kdelibs/kfile/kedittagsdialog.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/kedittagsdialog_p.h PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/kloadmetadatathread.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/kloadmetadatathread_p.h PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/kmetadataconfigurationdialog.h PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/kmetadataconfigurationdialog.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/kmetadatamodel.h PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/kmetadatamodel.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/kmetadatawidget.h PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/kmetadatawidget.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/knfotranslator.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/knfotranslator_p.h PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/ktaggingwidget.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kfile/ktaggingwidget_p.h PRE-CREATION 
>   trunk/KDE/kdelibs/nepomuk/core/ui/CMakeLists.txt 1102931 
>   trunk/KDE/kdelibs/nepomuk/core/ui/nepomukmassupdatejob.h 1102931 
>   trunk/KDE/kdelibs/nepomuk/core/ui/nepomukmassupdatejob.cpp 1102931 
> 
> Diff: http://reviewboard.kde.org/r/3277/diff
> 
> 
> Testing
> -------
> 
> Tested in Dolphin. An early version has been tested also in Mailody and Okular.
> 
> 
> Screenshots
> -----------
> 
> KMetaDataWidget
>   http://reviewboard.kde.org/r/3277/s/330/
> 
> 
> Thanks,
> 
> Peter
> 
>