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

List:       kde-core-devel
Subject:    Re: Review Request: Add KMetaDataWidget and
From:       "Peter Penz" <peter.penz () gmx ! at>
Date:       2009-10-27 8:06:19
Message-ID: 20091027080619.17795.87855 () localhost
[Download RAW message or body]


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

(Updated 2009-10-27 08:06:19.565297)


Review request for kdelibs, Sebastian Trueg and David Faure.


Changes
-------

Thanks for the review comments. I've uploaded an updated patch with the following \
changes:

* Removed the tags-, comments- and rating-APIs. As Sebastian said it is only an \
arbitrary subset of what Nepomuk can provide. If the application really requires to \
change those properties manually, it can do it directly with Nepomuk anyhow. Nice: \
The HAVE_NEPOMUK hack in the header is now gone.

* Use setShownData()/shownData() instead of setHiddenData()/hiddenData() as Aurélien \
suggests.

* Added methods setCustomDataList()/customDataList(), so that it is possible for \
applications like Gwenview to add a list of custom meta data. The method has not been \
implemented yet, but I mainly ask to check whether the interface and naming is OK. I \
thought about the possibility to add custom widgets instead of QString values, but I \
rejected the idea (this cries for some problems).

In general I'm against splitting KMetaDataWidget into 2 classes (one without Nepomuk \
and one with Nepomuk code). IMO the applications should not be burdened to \
instantiate the "correct" class dependent on whether Nepomuk is running or not. \
Whether Nepomuk is running or not is an implementation detail and reflected in the \
number of shown meta data, but it is not something that should result in the need of \
application code like this:

if (Nepomuk::ResourceManager::instance()->init() != 0) {
    m_metaDataWidget = new KMetaDataWidget(this);
} else {
    m_metaDataWidget = new KNepomukMetaDataWidget(this);
}

just let the application do this:
    m_metaDataWidget = new KMetaDataWidget(this);
and let KMetaDataWidget itself to take care about whether there is Nepomuk available \
or not.

Open issue:

* There is still the cmake warning mentioned above. I tried to use KDE4_NEPOMUK_LIBS \
instead of NEPOMUK_LIBRARIES after a hint from David, but in this case I even get a \
link error. Any help would be appreciated.


Summary
-------

The patch adds KMetaDataWidget and KMetaDataConfigurationDialog as public classes to \
kdelibs/kfile. 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: http://enzosworld.gmxhome.de/temp/metadatawidget_new.png \
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, ...).

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.

Sebastian Trüg, Tom Albers, Oliver Heidbüchel and I did already an internal review \
and the classes have been tested in the context of Mailody and Okular. There are \
still some minor implementation issues, but the main reason for this request is to \
review the public API and the integration into kdelibs/kfile (I'll take care to fix \
the issues until KDE 4.4).

I'd ask to mainly look at the files kfile/kmetadatawidget.h, \
kfile/kmetadataconfigurationdialog.h and kfile/CMakeLists.txt One ugly hack in the \
header file is the HAVE_NEPOMUK part in kmetadatawidget.h. As Nepomuk runs with \
Virtuoso now, the chances are good that we can get rid of this hack until KDE 4.4 \
(see also http://lists.kde.org/?l=kde-core-devel&m=125577498913008&w=2 for the \
discussion on kde-core-devel).

Please let me know whether there are general concerns regarding the location or the \
HAVE_NEPOMUK issue.


Diffs (updated)
-----

  trunk/KDE/kdelibs/kfile/CMakeLists.txt 1038666 
  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/kmetadataconfigurationdialog.h PRE-CREATION 
  trunk/KDE/kdelibs/kfile/kmetadataconfigurationdialog.cpp PRE-CREATION 
  trunk/KDE/kdelibs/kfile/kmetadatawidget.h PRE-CREATION 
  trunk/KDE/kdelibs/kfile/kmetadatawidget.cpp 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 1038666 
  trunk/KDE/kdelibs/nepomuk/core/ui/nepomukmassupdatejob.h 1038666 
  trunk/KDE/kdelibs/nepomuk/core/ui/nepomukmassupdatejob.cpp 1038666 

Diff: http://reviewboard.kde.org/r/1938/diff


Testing
-------

Tested in Dolphin, Mailody and Okular. Some minor implementation issues are open, but \
the interface seems to be sufficient.


Thanks,

Peter


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