> 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 > >
