Review Request: Add direct support for remote URLs to previewjob

[David Faure]

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



kio/kio/previewjob.cpp
<http://git.reviewboard.kde.org/r/102929/#comment6510>

    const QStringList protocols = ...



kio/kio/previewjob.cpp
<http://git.reviewboard.kde.org/r/102929/#comment6508>

    Good point, there should be a KService::mimeTypes(). Feel free to add a KDE5 TODO to that effect in kservice.h.



kio/kio/previewjob.cpp
<http://git.reviewboard.kde.org/r/102929/#comment6501>

    ThumbCreator is so that the above servicetype trader query works ;-) The services are associated with the "ThumbCreator" servicetype, that's how they get selected.



kio/kio/previewjob.cpp
<http://git.reviewboard.kde.org/r/102929/#comment6502>

    Remove this if( contains()) line, value() will return an empty list if it's not in the hash anyway.



kio/kio/previewjob.cpp
<http://git.reviewboard.kde.org/r/102929/#comment6509>

    value + modification + insert can be done much simpler.
    You want "find or create", that's what operator[] is for.
    
    QStringList &ms = m_remoteProtocolPlugins[protocol]; // find or create
    
    foreach(...) { ... }
    
    // No need to .insert() afterwards, you have modified the stringlist "in place" already.



kio/kio/previewjob.cpp
<http://git.reviewboard.kde.org/r/102929/#comment6503>

    Where is 'u' used?



kio/kio/previewjob.cpp
<http://git.reviewboard.kde.org/r/102929/#comment6511>

    Move inside the else().



kio/kio/previewjob.cpp
<http://git.reviewboard.kde.org/r/102929/#comment6504>

    Ouch, why is there a QUrl here? Make that a KUrl -- and then you don't need 'u' indeed.
    
    And move it inside the else, you don't need it in the local-path case.



kio/kio/previewjob.cpp
<http://git.reviewboard.kde.org/r/102929/#comment6507>

    Here again, no reason for a double lookup, contains+value.
    value is enough. If not found, empty stringlist, so contains(mimetype) will say false as expected.
    => remove the if().



kio/kio/previewjob.cpp
<http://git.reviewboard.kde.org/r/102929/#comment6506>

    You have a local var "mimeType", why not use it? ;)



kio/kio/previewjob.cpp
<http://git.reviewboard.kde.org/r/102929/#comment6505>

    And this is why you shouldn't use QUrl. As documented in KUrl, QUrl::toString() is terribly wrong (gives wrong results when a filename contains a '#' for instance).
    
    Make localUrl a KUrl, and use .url() here.


- David Faure


On Oct. 21, 2011, 11:20 a.m., Sebastian Kügler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102929/
> -----------------------------------------------------------
> 
> (Updated Oct. 21, 2011, 11:20 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> This patch allows previewjobs to take a URL in case they understand how to deal with that. preview plugis can specify a list of protocols which they support, this is then matched with the scheme of the URL and if supported the URL is passed directly into the plugin. Otherwise, the file is downloaded and passed as local file (this part hasn't changed).
> 
> A suitable preview plugin that does the job will be submitted separately to kde-runtime.
> 
> Is this definitively Frameworks 5 material, or would you be OK with adding it to the 4.7 branch? (The latter would mean, we could use vanilla kdelibs for Plasma Active 2 in december.)
> 
> 
> Diffs
> -----
> 
>   kio/kio/previewjob.cpp ff6d340 
> 
> Diff: http://git.reviewboard.kde.org/r/102929/diff/diff
> 
> 
> Testing
> -------
> 
> Generating preview for HTTP URLs, for local files and for remote files without a suitable plugin.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20111021/8b00f883/attachment.htm>