Review Request 125262: Parse service type files when loading from .desktop

[David Faure]

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



KF5CoreAddonsMacros.cmake (line 3)
<https://git.reviewboard.kde.org/r/125262/#comment59097>

    typo: SERIVCE -> SERVICE
    
    Does this break SC, if DEFAULT_SERVICE_TYPE must be specified? It sounds like it should be the default value instead, so that existing cmake code can keep working.



src/lib/plugin/desktopfileparser.h (line 39)
<https://git.reviewboard.kde.org/r/125262/#comment59098>

    typo: Definiton -> Definition



src/lib/plugin/desktopfileparser.h (line 45)
<https://git.reviewboard.kde.org/r/125262/#comment59099>

    defintion? ;)



src/lib/plugin/desktopfileparser.h (line 47)
<https://git.reviewboard.kde.org/r/125262/#comment59100>

    hm? it *is* marked as const...



src/lib/plugin/desktopfileparser.cpp (line 272)
<https://git.reviewboard.kde.org/r/125262/#comment59105>

    should be isEmpty, not just the more specific isNull?



src/lib/plugin/desktopfileparser.cpp (line 318)
<https://git.reviewboard.kde.org/r/125262/#comment59104>

    defs.reserve(paths.count())



src/lib/plugin/kpluginmetadata.h (line 169)
<https://git.reviewboard.kde.org/r/125262/#comment59101>

    give -> given



src/lib/plugin/kpluginmetadata.h (line 173)
<https://git.reviewboard.kde.org/r/125262/#comment59102>

    s/is intended//



src/lib/plugin/kpluginmetadata.cpp (line 106)
<https://git.reviewboard.kde.org/r/125262/#comment59103>

    inconsistent placement of '&', should after the space; best solution is to run astyle-kdelibs over the code.


- David Faure


On Sept. 16, 2015, 4:47 p.m., Alex Richardson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125262/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2015, 4:47 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> -------
> 
> KPluginMetadata("foo.desktop") or kcoreaddons_desktop_to_json() do not always result in the correct JSON for example when a property is
> supposed to be a list. the .desktop file parser in kcoreaddons so far had no way of know this so instead of treating the entry as a 
> comma-separated list it would simply return a JSON string.
> 
> This is a follow up to https://git.reviewboard.kde.org/r/121672/ with a different solution that also handles service types that are not compiled into the kcoreaddons library but instead parses the installed servicetype .desktop file.
> 
> It also contains a bit of code-deduplication and porting to categorized logging which is not directly related to this patch but it would be some effort to split that into a separate RR.
> 
> It is a series of commits with the following messages:
> 
> 
> desktopparser: avoid unnecessary utf8 decoding
> 
> Parse ServiceType files when reading .desktop files
> 
> 
> Remove lots of duplicated code for desktop{tojson,fileparser}.cpp
> 
> The only reason these were copy-pasted are minor differences in the
> output which is now fixed by using qInstallMessageHandler and the
> ability to generate JSON compatible with the first published version
> of desktoptojson (which is hopefully no longer used and can be removed
> soon)
> 
> QCommandLineParser uses -v for --version so just use --verbose
> 
> Otherwise the whole QCommandlineOption is ignored and there is no way
> to enable verbose mode
> 
> desktopparser: Use more categorized logging
> 
> 
> desktopparser: Allow passing relative paths to service type files
> 
> 
> Add KPluginMetaData::fromDesktopFile()
> 
> This function allows specifying a list of service type files to be
> parsed when loading the .desktop file.
> 
> desktopparser: Improve warning messages and add new unit test
> 
> The new test checks how the desktop parser handles service type files
> with invalid property definitions
> 
> desktopparser: Fix parsing of double and bool values
> 
> QString::compare returns 0 on equal and make sure that we don't assign
> the parsed double to an integer local variable
> 
> Add another unit test for desktop parsing with service types
> 
> Test that all supported types are converted correctly
> 
> Allow setting service types in kcoreaddons_desktop_to_json()
> 
> 
> Diffs
> -----
> 
>   KF5CoreAddonsMacros.cmake acfcaa3069991395d83923bcc30cd08f231c30eb 
>   autotests/data/servicetypes/bad-groups-input.desktop PRE-CREATION 
>   autotests/data/servicetypes/bad-groups-servicetype.desktop PRE-CREATION 
>   autotests/data/servicetypes/example-input.desktop PRE-CREATION 
>   autotests/data/servicetypes/example-servicetype.desktop PRE-CREATION 
>   autotests/data/servicetypes/fake-kdevelopplugin.desktop PRE-CREATION 
>   autotests/desktoptojsontest.cpp 64373d5be930426dd8a1f8e455e33c411a4795fd 
>   autotests/kpluginmetadatatest.cpp 3af5e1b842b0bc231a5ac001112e141f751d2ff5 
>   src/desktoptojson/CMakeLists.txt 94a199d8fa44a21b15e24c2e4f42551adada8f72 
>   src/desktoptojson/desktoptojson.h bfa38b0f5ddd0581ad176d854614bc9c80dd139a 
>   src/desktoptojson/desktoptojson.cpp 82626b256df6b3bd106e6d4c6fad84d7d970af37 
>   src/desktoptojson/main.cpp 9bac8ff55d005d1944c04f557aa9601de2b0ca15 
>   src/lib/plugin/desktopfileparser.h 98d47ddf0f877c4a25928026b3d5fe169cfc9e75 
>   src/lib/plugin/desktopfileparser.cpp 0b03eb154deb58840c91c12658780c0d492b593c 
>   src/lib/plugin/kpluginmetadata.h 183b0d0583259f7ed74e97858a68c5c388fd0a9a 
>   src/lib/plugin/kpluginmetadata.cpp b13d6dd52827cc03d9473600aa4d2bab8a95a1d4 
> 
> Diff: https://git.reviewboard.kde.org/r/125262/diff/
> 
> 
> Testing
> -------
> 
> Added some unit test and they pass
> 
> 
> Thanks,
> 
> Alex Richardson
> 
>

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