A new akonadi resource type implementing the Tomboy REST API
ClosedPublic

Authored by Stefan on Jun 28 2016, 5:46 PM.

Details

Summary

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

Diff Detail

Repository
R44 KDE PIM Runtime
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
Stefan added a comment.Jul 3 2016, 9:29 PM

e-mail and o1 class fix

Stefan marked 2 inline comments as done.Jul 3 2016, 9:30 PM

done

Stefan updated this revision to Diff 4949.Jul 4 2016, 8:16 PM

Found some issues with o1tomboy

mlaurent added inline comments.Jul 5 2016, 5:30 AM
resources/tomboynotes/Messages.sh
4

add "rm -f rc.cpp"

Thanks

Stefan marked an inline comment as done.Jul 5 2016, 8:35 AM

done

Stefan updated this revision to Diff 4954.Jul 5 2016, 8:36 AM

Message.sh

Stefan updated this revision to Diff 4958.Jul 5 2016, 11:00 AM

newline

Hello, whats the actual status of this commit request?

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

Stefan updated this revision to Diff 4986.Jul 6 2016, 5:35 PM
Stefan marked an inline comment as done.

o2 cmake

Stefan marked 4 inline comments as done.Jul 6 2016, 5:37 PM

done

resources/tomboynotes/CMakeLists.txt
42

I have tried it now ;)

Stefan updated this revision to Diff 4987.Jul 6 2016, 5:59 PM

findo2

Stefan updated this revision to Diff 4988.Jul 6 2016, 6:05 PM

missing find_packages

Stefan updated this revision to Diff 4989.Jul 6 2016, 6:43 PM

code style

Stefan updated this revision to Diff 4991.Jul 6 2016, 8:08 PM

Findo2.cmake is now tested and works :)

Stefan updated this revision to Diff 4992.Jul 6 2016, 8:23 PM

unnecessary includes removed

Stefan updated this revision to Diff 4993.Jul 6 2016, 9:27 PM

wrong path

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
11 ↗(On Diff #4993)

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

Stefan updated this revision to Diff 5030.Jul 8 2016, 9:16 PM

review changes

Stefan updated this revision to Diff 5035.Jul 8 2016, 10:15 PM

findo2 cmake

Stefan marked 4 inline comments as done.Jul 8 2016, 10:17 PM

done

Stefan updated this revision to Diff 5036.Jul 8 2016, 10:19 PM

now the correct diff

Stefan updated this revision to Diff 5037.Jul 8 2016, 11:15 PM

o2 included

Stefan updated this revision to Diff 5038.Jul 8 2016, 11:26 PM

forgotten changes regarding cmake

Stefan updated this revision to Diff 5081.Jul 11 2016, 6:09 PM

o2 checked with astyle. Thanks to montel

Stefan updated this revision to Diff 5084.Jul 11 2016, 6:31 PM

astyle on main code

dvratil added inline comments.Jul 12 2016, 10:01 PM
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.

Stefan updated this revision to Diff 5126.Jul 13 2016, 5:04 PM

o2 from link_librarys removed

Stefan marked 3 inline comments as done.Jul 13 2016, 5:07 PM

done

resources/tomboynotes/CMakeLists.txt
2–6

o2 infects the resource code, so the option are also needed for my code

Stefan updated this revision to Diff 5128.Jul 13 2016, 5:19 PM
Stefan marked an inline comment as done.

forgotten to commit ;)

knauss added inline comments.Jul 14 2016, 11:37 AM
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
20

This license is not Debian compatible ( https://www.debian.org/social_contract.html#guidelines), because:

  • Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.

This means you need to write the copyright, if you use the software: "Copyright (c) 2011, Andre Somers"

and the part:

  • Neither the name of the Rathenau Instituut, Andre Somers nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission.

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)

Stefan updated this revision to Diff 5168.Jul 14 2016, 2:48 PM
Stefan marked 3 inline comments as done.

cmake

Stefan marked an inline comment as done.Jul 14 2016, 2:49 PM

old stuff

knauss added inline comments.Jul 14 2016, 3:40 PM
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.
+ document every change to the libriary as a set of patches ( if you made any changes that are not upstream)

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
1 ↗(On Diff #5168)

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
otherwise you need to specifiy which files are under what licsense, because files without a lisence headers exist like the .rc and the CMakeLists.txt file.

knauss added inline comments.Jul 14 2016, 4:51 PM
resources/tomboynotes/o2/o0abstractstore.h
2

nippicking: missing lisence header - this is something that upstream should fix and not you with the copy, but please inform upstream about this.

Stefan updated this revision to Diff 5174.Jul 14 2016, 5:55 PM
Stefan marked 4 inline comments as done.

Hopefully the last diff ;)

resources/tomboynotes/README
15

Yes thats my plan. I don't want to ship o2 forever.

resources/tomboynotes/o2/o0abstractstore.h
2

I'll inform him

Stefan updated this revision to Diff 5176.Jul 14 2016, 6:13 PM

There are more changes regarding o2

knauss accepted this revision.Jul 14 2016, 8:17 PM
knauss added a reviewer: knauss.

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.

Stefan updated this revision to Diff 5185.Jul 14 2016, 9:26 PM
Stefan edited edge metadata.

Some needed options added. Many thanks to Sandro

This revision was automatically updated to reflect the committed changes.