This is a new akonadi resource type implementing the Tomboy REST API. It depends on a modified o2[1] that you can find here[2].
[1] https://github.com/pipacs/o2
[2] https://github.com/staeglis/o2
This is a new akonadi resource type implementing the Tomboy REST API. It depends on a modified o2[1] that you can find here[2].
[1] https://github.com/pipacs/o2
[2] https://github.com/staeglis/o2
Lint Skipped |
Unit Tests Skipped |
resources/tomboynotes/Messages.sh | ||
---|---|---|
4 | add "rm -f rc.cpp" Thanks |
You have to add
add_subdirectory(tomboyresource)
to resources/CMakeLists.txt, otherwise the code will never be built. I would argue that you should make the resource optional and only build it if the "o2" dependency is available.
resources/tomboynotes/CMakeLists.txt | ||
---|---|---|
42 | You are linking against the "o2" target, but it's not being located anywhere. You have to add find_package(o2 OPTIONAL) to the root CMakeLists.txt. And since the o2 project does not seem to install any CMake or pkgconfig files, you'll have to write your own Findo2.cmake and add it to cmake/modules. You can look on some of the files there to get an idea how to write the Findo2.cmake file. | |
resources/tomboynotes/configdialog.cpp | ||
69 | No spaces inside "( )" | |
74 | Space after if | |
resources/tomboynotes/tomboycollectionsdownloadjob.cpp | ||
54 | Move { on the same line as the if | |
resources/tomboynotes/tomboyitemuploadjob.cpp | ||
66 | Missed one string :) |
Some minor coding style issues that slipped through, otherwise I think the code is OK.
I'm however worried about the "o2" library. It does not install anything - the binary or the headers and it builds only static library, not a shared one. Packagers will hate us having to package it manually or will simply decide to ignore the dependency and will not ship the resource at all (speaking as a packager, I'd just ignore the dependency).
I see two reasonable options here: you could bundle the o2 library (which is what you did originally), but bundled libraries are hard to keep in sync with upstream and are generally an annoyance for developers as well as packagers. Other option is to borrow only the relevant parts from the o2 library, strip them down to what you actually need and build your own OAuth1 implementation directly into the resource.
CMakeLists.txt | ||
---|---|---|
88 | Normally you would append WebEngineWidgets to the list above, but since it's only needed to build your resource, it should be moved below your find_package(o2) and made conditional: if (o2_FOUND) find_package(Qt5 OPTIONAL_COMPONENTS WebEngineWidgets) endif () | |
140 | Please add set_package_properties(o2 PROPERTIES DESCRIPTION "OAuth1 and OAuth2 library for Qt. Needed to build the Tomboy resource." URL "https://github.com/pipacs/o2" TYPE OPTIONAL) To make it easier for packagers to figure out what they need to package. | |
cmake/modules/Findo2.cmake | ||
12 | Are you sure this works? The library does not install any pkgconfig files (does not install anything for that matter), so I doubt this finds anything... | |
resources/tomboynotes/tomboyitemuploadjob.cpp | ||
61 | QLatin1String | |
resources/tomboynotes/tomboynotesresource.cpp | ||
244 | } else { | |
resources/tomboynotes/tomboyserverauthenticatejob.cpp | ||
116 | Move to the same line as the if |
resources/tomboynotes/CMakeLists.txt | ||
---|---|---|
2–6 | This will unfortunately affect your resource code too, even though it's only needed because of o2 :-( You could add a CMakeLists.txt to the o2 subdirectory, where you remove these definitions and build the o2 library as a static library. Then you just put add_subdirectory(o2) here and you are done. | |
20–32 | If you go for the approach described above, remove those files from here. | |
61 | If you don't go for the approach described above (i.e. you stick to what you have now), then remove this, because in that case there won't be any "o2" library to link against. |
done
resources/tomboynotes/CMakeLists.txt | ||
---|---|---|
2–6 | o2 infects the resource code, so the option are also needed for my code |
CMakeLists.txt | ||
---|---|---|
137 | please no unrelated added/deleted newlines, they make it quite harder to use git blame f.ex. | |
resources/tomboynotes/o2/acknowledgements.md | ||
19 ↗ | (On Diff #5128) | This license is not Debian compatible ( https://www.debian.org/social_contract.html#guidelines), because:
This means you need to write the copyright, if you use the software: "Copyright (c) 2011, Andre Somers" and the part:
So you need to ask Andre Somers before redistrubution, becuase his name is paort of the copyright and this is against the DFSG #1, because you have a "royality" how can deside if you can redistrubute or not. And do not hold in the "Desert Island" test (https://people.debian.org/~bap/dfsg-faq.html) |
CMakeLists.txt | ||
---|---|---|
140 | typo? forgotten ) ? or is this only a error in the view? | |
resources/tomboynotes/README | ||
15 | Add a note that you have a copy of the o2 lib inside the sudir o2 and the lisnce of that lib + the git checkout. Btw distributions are mostly not happy with copies of libraries, so they wanna strip this out again. If the library ( I know that Dan suggested you to ship a copy). So if we can delete the copy with the nexit iteration, it would be great. | |
resources/tomboynotes/acknowledgements.md | ||
2 | wrongy copied file? This should only exist in the o2 sudir? Is there is any file thatis under this lisence in that folder? If not: delete |
resources/tomboynotes/o2/o0abstractstore.h | ||
---|---|---|
1 ↗ | (On Diff #5168) | nippicking: missing lisence header - this is something that upstream should fix and not you with the copy, but please inform upstream about this. |
looks fine for me.
resources/tomboynotes/README | ||
---|---|---|
24 | argh - never ever make whitespace changes (codestlye change) to a (temp) copy of a library. Because this makes the diff much bigger and harder to see, what you changed that matters. |