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

List:       kde-core-devel
Subject:    Re: Review Request: Pluggable KFileDialog/KAbstractFileModule
From:       "David Faure" <faure () kde ! org>
Date:       2009-08-30 22:26:53
Message-ID: 20090830222653.14746.65208 () localhost
[Download RAW message or body]


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

Ship it!


Heh. This is funny side effect of the kde-4.0 change were we made kfile \
dlopened. The idea was not at all to allow other implementations of it \
(rather it was just to delay the loading of that code). But ok, why not :)

About using the trader: indeed, not much point when the library name is \
already known (from the config). Using the trader would make sense if you \
wanted the user to be able to configure the preferred "KIO/FileModule" \
without knowing the name of the library (but simply by selecting in the \
list of available ones), or if you wanted the mere installation of the \
alternate module to mean it should be used (InitialPreference field). But \
indeed we don't want that in the case of the file dialog.

The fallback to loadFileModule("kfilemodule") could be done only if \
moduleName != "kfilemodule", btw, to avoid trying the same thing twice.

Anyway. No real objections to the patch, but I wonder if the integration of \
nepomuk features in the filedialog shouldn't be done with an additional \
"mode" in the existing filedialog rather than a hard all-or-nothing config \
item. A user might want to sometimes "query nepomuk" and sometimes "use the \
old-fashioned way"...

- David


On 2009-08-28 13:16:59, Sebastian Trueg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1407/
> -----------------------------------------------------------
> 
> (Updated 2009-08-28 13:16:59)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> KIO already loads the widget used in the KFileDialog dynamically from \
> libkfilemodule. The base class is KAbstractFileModule. However, the \
> loaded lib/plugin is fixed. This small patch changes the situation making \
> it really pluggable by using the standard KService procedure of loading \
> plugins. It allows to easily change the file dialog implementation by \
> creating a plugin of type "KIO/FileModule" and adding its name to the \
> "kfilemodulerc" config file. The reason for this is that in the GSoC \
> Alessandro Sivieri developed a replacement for the file dialog based on \
> Nepomuk. He tried to patch it into kdelibs but it was a mess due to all \
> the dependencies. With this approach implementations can easily be \
> switched. 
> Open issues:
> * the patch uses KService::serviceByDesktopName but also contains \
> commented code using KServiceTypeTrader. I am not sure which is \
>                 preferable.
> * I left in the legacy code of loading the filemodule from \
> libkfilemodule. I suppose this should be removed. 
> 
> Diffs
> -----
> 
> trunk/KDE/kdelibs/kfile/CMakeLists.txt 1013393 
> trunk/KDE/kdelibs/kfile/kfilemodule.desktop PRE-CREATION 
> trunk/KDE/kdelibs/kfile/kfilemodule.h 1013393 
> trunk/KDE/kdelibs/kfile/kfilemodule.cpp 1013393 
> trunk/KDE/kdelibs/kio/CMakeLists.txt 1013393 
> trunk/KDE/kdelibs/kio/kfile/kfiledialog.cpp 1013393 
> trunk/KDE/kdelibs/kio/kfile/kiofilemodule.desktop PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/1407/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sebastian
> 
> 


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