Review Request: Fix Bug 304299 - Dolphin launches multiple instances of a program when multiple files are selected

[Frank Reininghaus]

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


Thanks Emmanuel for the new patch! Looks good, I'm just a bit scared that we might run into problems by removing items from a list inside the foreach (...) loop.

But I think that DolphinView::slotItemsActivated(const QSet<int>& indexes) is more complicated than it needs to be even in the KDE/4.9 branch, and that we can rewrite it in a simpler way not that we are looking at this function in detail. How about this:

1. Move the "items.count() > 5" check further up and replace 'items' by 'indexes'.
2. In the loop that iterates over the set, determine the KFileItem that belongs to the index.
3. Still inside the loop, check if the item is a directory and open it in a new tab if that is the case, and append it to the list 'items' otherwise.

I think that the function becomes simpler that way, and there's no need anymore to first add directories to the list and then remove them again.



dolphin/src/views/dolphinview.cpp
<http://git.reviewboard.kde.org/r/107305/#comment17999>

    I'm just wondering if it's actually safe to remove items from the list that we are currently iterating over. Reminds me a bit of bug 311246.
    


- Frank Reininghaus


On Dec. 12, 2012, 6:29 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107305/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2012, 6:29 p.m.)
> 
> 
> Review request for Dolphin, David Faure and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> Fix Bug 304299 - Dolphin launches multiple instances of a program when multiple files are selected
> 
> 
> This addresses bug 304299.
>     http://bugs.kde.org/show_bug.cgi?id=304299
> 
> 
> Diffs
> -----
> 
>   dolphin/src/dolphinpart.h e5693b3 
>   dolphin/src/dolphinpart.cpp ccc91fd 
>   dolphin/src/dolphinviewcontainer.h 0300273 
>   dolphin/src/dolphinviewcontainer.cpp 6e99437 
>   dolphin/src/views/dolphinview.h 6d15ebf 
>   dolphin/src/views/dolphinview.cpp ea64a04 
> 
> Diff: http://git.reviewboard.kde.org/r/107305/diff/
> 
> 
> Testing
> -------
> 
> Tested with Gwenview, works fine.
> 
> # Dolphin 2.1: Start one Gwenview instance for each selected image
> # Patched Dolphin: Open all selected images in one Gwenview instance
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20121213/eaf96b9f/attachment.htm>