Review Request: Fix "open with" list contents and ordering

[David Faure]

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


It took me a long time to understand what the code was trying to do. In addition to the more specific implementation suggestions below, I would recommend adding a comment about the purpose of the ranking stuff, so that future readers of the code (who won't have this reviewboard request as context) will understand what it's all about.


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

    const QString&



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

    So this goes 0, -1, -2, -3, etc.?
    A bit surprising; why not use positive values (and then sort reverse at the some point, I suppose)?



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

    This code looks slow. After collecting all values, it has to lookup (with a linear search) the key for each value. And then remove from the hash because that's not unique... this is a bit convoluted.
    
    I think a more straightforward way to do this would be to make a vector of (serviceId, int) structs and sort that (using a predicate function) and then just iterate through it. I wouldn't even mind that list being used from the start instead of the hash, even if it means a linear search through the list, to find the serviceId. This can always be optimized with a qhash that points to the index in the vector if necessary, but at least the logic will be much more readable (sort+iterate rather than values+sort+foreach value+lookup+remove).



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

    You could even add the KService::Ptr in the struct, from the start, and then you wouldn't need to look it up again here.


- David


On 2010-02-16 20:13:43, Todd wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2984/
> -----------------------------------------------------------
> 
> (Updated 2010-02-16 20:13:43)
> 
> 
> Review request for Dolphin and kdelibs.
> 
> 
> Summary
> -------
> 
> In the Dolphin and Konqueror menu you get when right-clicking on a group of files, there is a sub-menu that lists possible applications you can use to open the file.  This list, however, does not reliably show all of the valid applications when multiple mimetypes are selected.  This patch fixes that problem.  Further, it sorts the list based on a weighting determined by how highly ranked the application is in all of the selected mimetypes.  So the higher average priority of an application across all the selected mimetypes, the higher its position in the menu.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kio/kio/kfileitemactions.cpp 1091061 
> 
> Diff: http://reviewboard.kde.org/r/2984/diff
> 
> 
> Testing
> -------
> 
> I tested various file types with different combinations of associated applications in different orders.
> 
> 
> Thanks,
> 
> Todd
> 
>