'Re: Review Request: Add KMetaDataWidget,'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request: Add KMetaDataWidget,
From:       "Peter Penz" <peter.penz () gmx ! at>
Date:       2010-03-19 21:35:14
Message-ID: 20100319213514.1508.42217 () localhost
[Download RAW message or body]



> On 2010-03-19 10:00:17, Sebastian Trueg wrote:
> > trunk/KDE/kdelibs/kfile/kmetadatamodel.h, line 49
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20853#file20853line49>
> > 
> > IMHO this file should not be made public yet. I doubt anyone will reimplement \
> > this model soon. And the less API is public the more we can tune it afterwards.

It was mainly intended for Gwenview. Aurélien was interested into adding custom meta \
data. But like above I think it is a good idea to make this a private interface in \
the meantime. Aurélien could still use it and we could finetune the interface until \
it gets public.


> On 2010-03-19 10:00:17, Sebastian Trueg wrote:
> > trunk/KDE/kdelibs/kfile/kmetadataconfigurationdialog.h, line 34
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20851#file20851line34>
> > 
> > Like with the model I would not make this public yet.

Hm... I think you are right. The KPropertiesDialog won't use this class and even if \
applications like Gwenview or Okular will use the KMetaDataWidget, it is unclear yet \
whether they need the configuration dialog in this way. OK, so I'll leave the \
KMetaDataConfiguration in Dolphin now. In the worst case, if other applications will \
start to copy this class, we might think again about moving it to kdelibs later after \
getting feedback from the app-developers.


> On 2010-03-19 10:00:17, Sebastian Trueg wrote:
> > trunk/KDE/kdelibs/kfile/kmetadatawidget.h, line 134
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20855#file20855line134>
> > 
> > This could be moved into the model to keep any URI stuff out of the widget. If \
> > you want to keep Nepomuk as an option using URIs as property identifier is \
> > probably not best anyway.

Although Nepomuk is optional, having mixed kind of keys for meta data is worse I \
think (from an interface point of view and implementation point of view). If someone \
adds meta data, where no official URI is defined, it is no problem to define a custom \
URI. But yes: This should be moved to the model (I'll change this - I initially \
thought the translation should belong to the widget, but the labels just belong to \
the meta data of the model).


> On 2010-03-19 10:00:17, Sebastian Trueg wrote:
> > trunk/KDE/kdelibs/kfile/kmetadatawidget.h, line 148
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20855#file20855line148>
> > 
> > Why not move this into the model, too. Then we can optimize it in any way until \
> > we make the model public. Or if you think a widget method should not be in the \
> > model just make it non-public for now to keep the API as clean as possible.  Any \
> > further improvements can be done later. And I don't think any client does \
> > override custom widgets at this point.

I'll move those 2 methods into the model, with minor adjustments (valueWidget() must \
get a factory method...). As you say it gives us more chances to work on improvements \
without thinking about API backward compatibility.


> On 2010-03-19 10:00:17, Sebastian Trueg wrote:
> > trunk/KDE/kdelibs/kfile/kmetadatawidget.h, line 157
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20855#file20855line157>
> > 
> > The simplest solution here would be to use QVariant. After all we can easily put \
> > a Nepomuk::Variant in a QVariant or just convert it internally.

I'll check this. Thanks for your good suggestions!


- Peter


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


On 2010-03-16 20:39:25, Peter Penz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3277/
> -----------------------------------------------------------
> 
> (Updated 2010-03-16 20:39:25)
> 
> 
> 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/CMakeLists.txt 1103259 
> trunk/KDE/kdelibs/config-nepomuk.h.cmake PRE-CREATION 
> trunk/KDE/kdelibs/kfile/CMakeLists.txt 1103259 
> trunk/KDE/kdelibs/kfile/kcommentwidget.cpp PRE-CREATION 
> trunk/KDE/kdelibs/kfile/kcommentwidget_p.h PRE-CREATION 
> 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 1103259 
> trunk/KDE/kdelibs/nepomuk/core/ui/nepomukmassupdatejob.h 1103259 
> trunk/KDE/kdelibs/nepomuk/core/ui/nepomukmassupdatejob.cpp 1103259 
> 
> 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
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic