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

[Andreas Hartmetz]

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


I know that David doesn't work on weekends, so I'll take care of the nitpicky comments :)
I'll mention *every little thing I can think of*, this does not mean that I think the code is bad.


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

    static bool serviceRankLess(const serviceRank& id1, const serviceRank& id2)
    (see below)



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

    While the usefulness here is small, I encourage using const as much as possible. (this is not a criticism ;)



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

    rankings[idPos].score += 1;



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

    spacing



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

    const & will probably save a copy here



/trunk/KDE/kdelibs/kio/kio/kfileitemactions_p.h
<http://reviewboard.kde.org/r/2984/#comment3746>

    1) const serviceRank &
    2) There is no need to forward-declare that function here. File-static functions generate no symbols in release binaries and can save a few bytes and maybe CPU cycles - the compiler can mangle file static functions any way it wants because they are guaranteed to be internal to the currently compiled file. Inline (the keyword as well as just defining the function here, see http://www.parashift.com/c++-faq-lite/inline-functions.html) has a similar effect.
    Feel free to pick the file static approach, inline, or make no change at all.
    Strictly speaking you don't need to declare the struct here as well, it would suffice to do that in the .cpp file.


- Andreas


On 2010-02-20 08:59:04, Todd wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2984/
> -----------------------------------------------------------
> 
> (Updated 2010-02-20 08:59:04)
> 
> 
> Review request for 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_p.h 1091061 
>   /trunk/KDE/kdelibs/kio/kio/kfileitemactions.cpp 1091061 
>   /trunk/KDE/kdelibs/kio/kio/kfileitemactions.h 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
> 
>