Review Request: Add KMetaDataWidget, KMetaDataModel and KMetaDataConfigurationDialog

Peter Penz peter.penz at gmx.at
Tue Mar 16 07:33:57 GMT 2010 

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



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

    OK



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

    Oh, you are right. I'm not sure whether it is needed anyhow, as probably we'll have to move this class to kdelibs/nepomuk (see later comment for loadData()).



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

    IMO Qt has no ownership to the MVC-pattern ;-) However KMetaDataProvider might be OK for me too, but I'd prefer to leave it as it is. But I've no strong opinion here.



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

    Yes, it is a sorting indicator. I used ints initially, but beside being slower [1] I found it also too abstract: This group() only acts as internal prefix and IMO it's more comfortable to add a prefix "size" to the meta data for "Width" and "Height" to group them instead of e. g. return a 7. It's mainly a functionality too group items together and not really a global sort criteria. Is the comment unclear or do you still think that ints would be preferable?
    
    [1] Internally I do a comparison of strings to sort the items. When using an int I must convert it to a string first including the need to add leading zeros... However the runtime is not critical here, it is more important to have a proper interface.



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

    Assuming I replace this method by another concept: Are there any concerns from your side regarding the non-virtual method 'QHash<KUrl, Nepomuk::Variant> data()? It is BIC too or am I missing something?
    
    We had already a review for an older version of KMetaData widget some months ago (http://reviewboard.kde.org/r/1938) and the non-virtual method had not been considered as issue.
    
    Generally speaking I'm very unhappy about the optional compile dependency of Nepomuk and thought as KDE PIM relies on Nepomuk since 4.4, that it will get non-optional with 4.5. However: Assuming also the non-virtual variant is no solution, then I think we must move the class to kdelibs/nepomuk. Drawback: each application that wants to use this widget must invoke the class inside a nasty HAVE_NEPOMUK path...
    
    I don't want to start a lengthy discussion in this review, but if we want Nepomuk to get used by applications, then this optional compile dependency is really counter productive. The code gets nasty and it is very time consuming for me to do always 2 complete builds for Dolphin before I'm able to commit any Nepomuk related fix... :-(



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

    OK



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

    I'll check this. I initially wanted to stick with QVariant, but this makes it very complicated and time consuming to return Nepomuk resources...



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

    OK.



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

    OK.



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

    Good input, as it makes clear that no information is given in the class documentation how meta data properties can be made visible/invisible. Currently the visibility of each meta data is remembered in a configuration file called kmetadatainformationrc (= global setting).
    
    Tom Albers requested that for Mailody he wants to turn off the visibility of properties that are available for KFileItem on application level. So in this case the application overrules the global settings.
    
    Now as I need to explain it, I recognize that the interface is flawed and a general solution is required. I'd suggest to remove the enum MetaDataTypes completely and work with meta data URIs instead. What do you think from the following interface:
    
    void setVisible(const KUrl& metaDataUri);
    bool isVisible(const KUrl& metaDataUri) const;
    
    also a kind of saveSettings(...) method would be required. By this it would be possible to adjust the visibility on application level only or globally. Any suggestions would be very welcome.



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

    Good input. This is possible and I'll adjust it...
    
    Thanks Christoph for your valuable input, I hope I've time in the evening to cleanup the obvious things, so that we can concentrate on the remaining issues.


- Peter


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

More information about the kde-core-devel mailing list