Review Request 129643: DesktopFileParser: Honor ServiceTypes field

[Aleix Pol Gonzalez]

> On Dec. 13, 2016, 5:42 a.m., Michael Pyne wrote:
> > src/lib/plugin/desktopfileparser.cpp, line 358
> > <https://git.reviewboard.kde.org/r/129643/diff/1/?file=487269#file487269line358>
> >
> >     If returning false here is really a fatal error it might be worth using a `Q_UNLIKELY` in the condition here.

I can add it, but it's barely a critical path...


> On Dec. 13, 2016, 5:42 a.m., Michael Pyne wrote:
> > src/lib/plugin/desktopfileparser.cpp, line 423
> > <https://git.reviewboard.kde.org/r/129643/diff/1/?file=487269#file487269line423>
> >
> >     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?

Because the services just used to be specified by CLI arguments, it was the only path.

The whole DesktopFileParser should be dropped for KF6 entirely.


- Aleix


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


On Dec. 12, 2016, 4: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, 4: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/4820cf35/attachment.html>