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

List:       kde-core-devel
Subject:    Re: Review Request: Open selected files with default application in
From:       "David Faure" <faure () kde ! org>
Date:       2010-03-29 10:05:11
Message-ID: 20100329100511.4884.25919 () localhost
[Download RAW message or body]


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



/trunk/KDE/kdebase/apps/dolphin/src/dolphincontroller.cpp
<http://reviewboard.kde.org/r/3163/#comment4223>

    Could be a direct member rather than a pointer, then no need for new/delete.



/trunk/KDE/kdelibs/kio/kio/kfileitemactions.cpp
<http://reviewboard.kde.org/r/3163/#comment4225>

    Make the temporary const, as in "const QStringList serviceIdList = ...".



/trunk/KDE/kdelibs/kio/kio/kfileitemactions.cpp
<http://reviewboard.kde.org/r/3163/#comment4224>

    A QString can never be "== 0", since a QString is a string, not a pointer.
    
    Use .isEmpty() instead.



/trunk/KDE/kdelibs/kio/kio/kfileitemactions.cpp
<http://reviewboard.kde.org/r/3163/#comment4229>

    Confusing indentation



/trunk/KDE/kdelibs/kio/kio/kfileitemactions.cpp
<http://reviewboard.kde.org/r/3163/#comment4226>

    KMimeTypeTrader has a preferredService method for this kind of use, no need to \
use query+first.



/trunk/KDE/kdelibs/kio/kio/kfileitemactions.cpp
<http://reviewboard.kde.org/r/3163/#comment4227>

    if (!serviceId.isEmpty() && ...)
    
    But even in the "empty serviceId" case, i.e. "no app associated", you need to \
append to "serviceItems" so that the open-with dialog appears for all these files, \
no? So I think the "service not empty" condition should be removed.  



/trunk/KDE/kdelibs/kio/kio/kfileitemactions.cpp
<http://reviewboard.kde.org/r/3163/#comment4228>

    Here, on the other hand, it would be safer to test that serviceByStorageId didn't \
return NULL (e.g. due to a discrepancy in ksycoca).  
    And if serviceId is empty (no app associated), you need to use \
KRun::displayOpenWithDialog instead of KRun::run.



/trunk/KDE/kdelibs/kio/kio/kfileitemactions.cpp
<http://reviewboard.kde.org/r/3163/#comment4232>

    This could also use KMimeTypeTrader::preferredService to make the code easier to \
read.



/trunk/KDE/kdelibs/kio/kio/kfileitemactions.cpp
<http://reviewboard.kde.org/r/3163/#comment4231>

    use QString() instead of 0.


- David


On 2010-03-06 21:48:09, Todd wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3163/
> -----------------------------------------------------------
> 
> (Updated 2010-03-06 21:48:09)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> When files are selected in the file browser and the right-click menu is opened, \
> adds an option above the "Open with..." sub-menu to open all the files in their \
> respective preferred applications.  If all files have the same preferred \
> application, it says "Open with <application>" and uses that application's icon, \
> otherwise it just uses says "Open" and does not have an icon. 
> This is similar to the behavior in dolphin where selecting multiple files and \
> hitting "enter" opens those files in the preferred application.  In order to \
> maintain consistency, this patch also changes Dolphin to use the same slot as as \
> the file manager right-click menu.  The old Dolphin implementation just iterated \
> through the list of files, opening each one in its default application.  It did not \
> have any way to deal with multiple files having the same default application. 
> 
> Diffs
> -----
> 
> /trunk/KDE/kdebase/apps/dolphin/src/dolphincontroller.h 1080351 
> /trunk/KDE/kdebase/apps/dolphin/src/dolphincontroller.cpp 1080351 
> /trunk/KDE/kdelibs/kio/kio/kfileitemactions.h 1094205 
> /trunk/KDE/kdelibs/kio/kio/kfileitemactions.cpp 1094205 
> /trunk/KDE/kdelibs/kio/kio/kfileitemactions_p.h 1094205 
> 
> Diff: http://reviewboard.kde.org/r/3163/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Todd
> 
> 


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