Review Request: Add KMetaDataWidget, KMetaDataModel and KMetaDataConfigurationDialog

Peter Penz peter.penz at gmx.at
Sat Mar 20 07:35:22 GMT 2010 


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kcommentwidget.cpp, line 101
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20845#file20845line101>
> >
> >     Qt::Dialog can be removed; QDialog takes care of setting that flag internally.

OK.


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kcommentwidget.cpp, line 126
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20845#file20845line126>
> >
> >     The KConfigBase::Persistent can be removed; the default value is Normal which is equal to Persistent. The explicit value just made the code more confusing to my eyes ("is this trying to remove another default flag, or something?").

OK.


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kedittagsdialog.cpp, line 61
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20847#file20847line61>
> >
> >     The parent widget should be mainWidget, not this. Current code works, but reparents.

OK.


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kedittagsdialog.cpp, line 191
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20847#file20847line191>
> >
> >     In one line with m_deleteButton->setGeometry(x, y, size, size);

OK.


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kloadmetadatathread.cpp, line 68
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20849#file20849line68>
> >
> >     Race condition here; the thread could be not finished at the time of the isFinished() call, but could be finished before you connect to its finished() signal, so you'd miss the signal.
> >     
> >     The only way to do this properly is with a QSemaphore I suppose; hmm, not sure how exactly. Would be better to not need that if basically, and to always connect finished() to a slot in the main thread - which you seem to do anyway, right? So from the main thread you can decide reliably whether you've seen the finished signal already or not. But doing the connect here (at a random point in the lifetime of the thread) is wrong.
> >     
> >     In fact, this method is unused, isn't it? I would just remove it then.

Ah, I forgot to delete this method. As you say it does not work and I used another approach when refactoring the widget. I'll delete it :-)


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kloadmetadatathread_p.h, line 95
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20850#file20850line95>
> >
> >     Should be marked as volatile if it's modified by a thread and read by another (without locking). I'm told it matters on some architectures (not x86).

OK, I was not aware about this.


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kmetadataconfigurationdialog.cpp, line 100
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20852#file20852line100>
> >
> >     Remove 2nd argument

OK.


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kmetadatamodel.h, line 30
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20853#file20853line30>
> >
> >     As you suspected, including config-nepomuk.h and using HAVE_NEPOMUK in a public header is a big no-no. It makes the header unusable in other modules, unless they replicate the creation of a config-nepomuk.h, we usually don't do this.
> >     
> >     We heard the rule "no #ifdef HAVE_FOO in public header files" many many times in kde3 times, I would say it still applies.
> >     
> >     But I'm not sure what to suggest instead. Well, maybe to just use nepomuk unconditionally in all the files and to compile them and use them only if nepomuk support is enabled. At least this doesn't impose on users of the file that they should have a config-nepomuk.h.

After the suggestions of Sebastian the HAVE_NEPOMUK could be removed from the public KMetaData header. I've modified this already in a local patch and will hopefully attach an update tomorrow to the review board.


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kmetadatamodel.cpp, line 80
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20854#file20854line80>
> >
> >     How about passing the thread pointer as parameter, to avoid having to use sender()?

You mean introducing a custom signal finished(QThread* thread) in KLoadMetaDataThread?


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kmetadatamodel.cpp, line 85
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20854#file20854line85>
> >
> >     The thread's slotFinished does deleteLater already, doesn't it?

No, this is also obsolete code that is not used anymore, because the cancelAndDelete() method is obsolete too. I've removed both methods in KLoadMetaDataThread now, so creating and deleting the thread is done in KMetaDataModel.

But what's missing is to handle the case if the model gets destroyed and the threads are still working (-> wait()). I'll fix this.


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kmetadatawidget.cpp, line 575
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20856#file20856line575>
> >
> >     What if the URL is remote? In case setItems(KUrl) was called, the KFileItem will contain absolutely no time/user/permission information. I suppose this needs to test for invalid time etc. in order to show something more user-friendly.
> >     Even isDir() isn't going to work, if a real kfileitem wasn't provided as input. I guess the API docs should warn about this, too.

I'll check this.


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/ktaggingwidget.cpp, line 127
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20859#file20859line127>
> >
> >     Remove Qt::Dialog

OK. Thanks David for your input, I'll try to provide a patch tomorrow.


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kloadmetadatathread.cpp, line 162
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20849#file20849line162>
> >
> >     I am very surprised to see no locking at all in this thread.
> >     This will crash if m_model is deleted while the thread is running, which doesn't seem to be handled at all. The model has to cancel() all threads, then wait() on them, before it can proceed with letting itself be deleted.
> >     
> >     But note that in wait() it won't process events, so the slotFinished of the model won't be called. So the cleanups (and deletion of the thread instance) have to be done in that method, rather than assuming the slotFinished cleanups will happen.
> >     
> >     More generally, since this implements a thread, I strongly suggest running this code in valgrind --tool=helgrind, preferrably from a unittest.

I've just introduced this nasty m_model pointer 10 days ago and it is completely bogus, but I did not notice it :-( Of course you are right. I've now completely removed it in a local patch (I'll provide an update hopefully tomorrow).

Regarding the no locking in the thread: Now as the m_model pointer is (will get) removed again, there is no concurring access to shared data between KMetaDataModel and KLoadMetaDataThread. KMetaDataModel only tells the thread "give me the data for this URLs", waits for the finished() signal and gets the data afterwards -> the access is queued.

Unittesting this class is a good idea.


> On 2010-03-20 00:05:35, David Faure wrote:
> > trunk/KDE/kdelibs/kfile/kmetadatamodel.cpp, line 150
> > <http://reviewboard.kde.org/r/3277/diff/2/?file=20854#file20854line150>
> >
> >     So, this is going to be called from another thread, and currently without any locking at all. That makes me quite suspicious, but then again, there's no actual implementation of loadData() in this patch, right? I'm just curious to see how one would implement this properly.

I've removed this method now, this _is_ suspicious. As said above: I introduced this thing 10 days ago and it seems that was no good day... As you say the implementation is empty currently and that was the reason it worked so well ;-)


- Peter


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


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

More information about the kde-core-devel mailing list