Review Request: Port the kio file conflict dialog to KFileMetaDataWidget and PreviewJob

[David Faure]

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


I like the idea, but I'm concerned about remote files. See details below.


/trunk/KDE/kdelibs/kio/kio/renamedialog.h
<http://reviewboard.kde.org/r/3674/#comment5112>

    No need to include kfileitem.h here, a "class KFileItem" is enough for "const KFileItem&".
    
    Same thing for all the added includes, in fact. Use forward decls instead:
    class QScrollArea;
    class QLabel;
    class QPixmap;
    and include the files in the .cpp (implementation details)



/trunk/KDE/kdelibs/kio/kio/renamedialog.cpp
<http://reviewboard.kde.org/r/3674/#comment5113>

    doesn't seem to be used



/trunk/KDE/kdelibs/kio/kio/renamedialog.cpp
<http://reviewboard.kde.org/r/3674/#comment5114>

    Hmm, for local files the metadata widget will be able to show size and mtime, but what about remote files?
    
    I think this patch leads to a big regression with remote files (e.g. ftp, fish, sftp, smb), where information such as size and modification time won't be displayed anymore. Showstopper :-)
    
    Can you provide a way to see that information for remote files? I don't know much about the metadatawidget so I don't know if this should be done internally or externally...



/trunk/KDE/kdelibs/kio/kio/renamedialog.cpp
<http://reviewboard.kde.org/r/3674/#comment5115>

    s/similar/the same/ ? It's exactly equal...



/trunk/KDE/kdelibs/kio/kio/renamedialog.cpp
<http://reviewboard.kde.org/r/3674/#comment5116>

    Are previews always 128x128? Isn't that configurable? I'm surprised by the hardcoded value -- well, it doesn't look like one, but it is one :-)



/trunk/KDE/kdelibs/kio/kio/renamedialog.cpp
<http://reviewboard.kde.org/r/3674/#comment5117>

    It is much simpler to use containerLayout->addStretch();



/trunk/KDE/kdelibs/kio/kio/renamedialog.cpp
<http://reviewboard.kde.org/r/3674/#comment5118>

    Why 128+32?


- David


On 2010-05-04 18:22:06, Todd wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3674/
> -----------------------------------------------------------
> 
> (Updated 2010-05-04 18:22:06)
> 
> 
> Review request for kdelibs, Peter Penz and David Faure.
> 
> 
> Summary
> -------
> 
> This patch switches the kio file conflict dialog (renamedialog) from using mimetype-specific plugins for previews (sometimes) and file information to instead using KFileMetaDataWidget for information and PreviewJob for previews.  
> 
> This approach has several advantages.  First, it shares code with the rest of KDE, so there is no need to write additional plugins for the renamedialog (which only has two plugins currently).  It also means that the appearance is consistent across all file types, instead of having completely different layouts for different file types.  It also allows for nepomuk information to be displayed (when available).  It allows for more information than would fit comfortably (using scroll bars).  And finally it allows for previews and specialized information of file types for which writing now plugins would be a waste of time (like text files and folders).  
> 
> As far as I can tell it should be backwards-compatible (if it isn't I'll fix it).  Using this approach does mean that renamedialogplugin (which I left alone for compatibility) does not actually do anything anymore.  However, the new widget provides more information and provides it for more file types than the plugins.
> 
> It does not give previews for videos, but the existing rename dialog doesn't either so there is nothing lost there.  PhononWidget handles this in Dolphin, but since it is part of dolphin it is not available in kdelibs
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kio/kio/renamedialog.h 1116651 
>   /trunk/KDE/kdelibs/kio/kio/renamedialog.cpp 1116651 
> 
> Diff: http://reviewboard.kde.org/r/3674/diff
> 
> 
> Testing
> -------
> 
> It gives proper previews for images and text files, and proper icons for folders and unknown binary files.  It also gives the correct icon for video files, but no preview (which is the same as the old widget).
> 
> This is a complete rewrite of a large portion of the dialog, so I would very much appreciate it if other people tested it as well.
> 
> 
> Thanks,
> 
> Todd
> 
>