Review Request 129643: DesktopFileParser: Honor ServiceTypes field

[Michael Pyne]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129643/#review101406
-----------------------------------------------------------


Fix it, then Ship it!




This isn't my area of expertise but the change looks correct (and sane, given the intended purpose). +1 from me.  I have recommendations for changes, but none that would block committing if you think the patch is better the way it currently is.


src/lib/plugin/desktopfileparser.cpp 
<https://git.reviewboard.kde.org/r/129643/#comment67894>

    We can still call `reserve` here right, only now on `m_definitions`?



src/lib/plugin/desktopfileparser.cpp (line 342)
<https://git.reviewboard.kde.org/r/129643/#comment67895>

    If returning false here is really a fatal error it might be worth using a `Q_UNLIKELY` in the condition here.



src/lib/plugin/desktopfileparser.cpp (line 407)
<https://git.reviewboard.kde.org/r/129643/#comment67897>

    I'm not very familiar with this code, but why is this the only codepath in this JSON-related function that modifies the m_definitions list of service type paths?  Is this just to make later fallback lookups at the end of `convertToJson` check the extra file paths to potential services?
    
    If so I think the comment should note that as well as the legacy nod.
    
    Also, is it worth tagging to be removed in KF6 like some of the other compatibility code?



src/lib/plugin/desktopfileparser_p.h (line 48)
<https://git.reviewboard.kde.org/r/129643/#comment67896>

    Although this is a private header a comment about `addFile` purpose still would be appropriate.


- Michael Pyne


On Dec. 12, 2016, 3:26 p.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129643/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2016, 3:26 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> -------
> 
> In case there's a ServiceType field in the desktop file, look them up for definitions.
> 
> Otherwise we just printed a warning whenever the cmake macro was used that nobody resolved (see plasma workspace).
> 
> This patch assumes some heuristic for the naming that is not standardized (`/` to `-`, all lower-case), it won't enforce it though, so worst case scenario it works just as it does now.
> 
> 
> Diffs
> -----
> 
>   KF5CoreAddonsMacros.cmake eff0e3f 
>   autotests/desktoptojsontest.cpp bb539ee 
>   src/lib/plugin/desktopfileparser.cpp 2eb198d 
>   src/lib/plugin/desktopfileparser_p.h c61b297 
> 
> Diff: https://git.reviewboard.kde.org/r/129643/diff/
> 
> 
> Testing
> -------
> 
> Tests still pass, plasma-workspace applets are generated with correct types.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20161213/e203f834/attachment-0001.html>