[Konversation-devel] Review Request: Make dependency on kdepim optional

[Thomas Capricelli]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103551/
-----------------------------------------------------------

(Updated Jan. 7, 2012, 8:54 a.m.)


Review request for Konversation.


Changes
-------


Thanks for the review. I agree with most remarks, especially the one about
'addressee', sorry for that. This new patch fix those issues.
I've installed kdepimlibs, which i hate, just to be able to test 
compilation.

* Concerning the name, I thought ADDRESSBOOK_INTEGRATION was more 
  appropriate as it describes the feature, not he requirements. But you 
  decide, so i've changed it. I dont agree though.

* for the "doesn't belong to the topic of the patch", those were only
  _comments_. It was only a hint for future people working on the file. 
  I've removed them as this concern you so much.

* the else after if (){ return} are _ugly_ and made me loose a lot of time
  before understanding what's going on. I'd like to prevent this kind of 
  frustration to future devs.

I'm surprised by how nitpicking you are when I see the overall quality of
the code. I understand the code has a long history and so much devs worked on 
it, I'm not blaming anyone.
Compilation of konversation here triggers a lot of warnings. I didn't even
notice my own ones.

This patch was tested with and without kdepimlibs installed. I tested cmake
(checking result of kdepim availability), compilation and checked against 
which libs the resulting binary was linked.

Few random notes
* indentation in src/irc/nicksonline.h is ugly, i did not dare touching it.
* in src/irc/server.cpp:Server::setNickOffline, what if nickInfo is the
  null pointer ? wasOnline checks for this, but there's not check here.. ??
* i only have this one warning when compiling without kdepimlibs
  /usr/include/KDE/../kurifilter.h:732:110: warning: ‘ProviderInfoList’ is deprecated [-Wdeprecated-declarations]


Description
-------

For people outthere like me that do not use KDE, the dependency on kdepim is a very huge one. We would very happily make the tradeoff of NOT being able to link nicknames to kde addressbook.

I guess you see the points, depending on kdepim brings two big problems
* installation/compilation : depending on kdepim brings half of kde packages, kdepim package and hence ugliness such as nepomuk/akonadi.
* at runtime, starting konversation, by the magic of dbus and KDE spaghetti runtime dependencies, it starts lot of unrelated KDE stuff daemons that takes both cpu and ram. I know this arg is denied by KDE zealot, but i stand on it.

So I did it. This patch allows compilation without kdepim. I've tried to keep the patch as minimal as possible of course.

I'm always on irc/freenode as 'orzel', and i'll be on #konversation for the next days.
Also, I do have commit rights on KDE (not sure about konversation proper, i did no follow the git migration very closely)


Diffs (updated)
-----

  CMakeLists.txt c8df038 
  config-konversation.h.cmake 4aaf9d1 
  src/CMakeLists.txt a7ba9f4 
  src/application.cpp 038b403 
  src/irc/nick.cpp 298eafb 
  src/irc/nickinfo.h 20b8ef1 
  src/irc/nickinfo.cpp e768d0d 
  src/irc/nicksonline.h fbdd2cd 
  src/irc/nicksonline.cpp 8c9a4b1 
  src/irc/outputfilter.cpp a7701e7 
  src/irc/query.h 3685a53 
  src/irc/query.cpp 1a432d4 
  src/irc/server.h 775cb7c 
  src/irc/server.cpp f2d3168 
  src/irc/serverison.h 4500564 
  src/irc/serverison.cpp f737b51 
  src/konsolepanel.cpp 14b32cc 
  src/main.cpp f2c7a22 
  src/mainwindow.cpp 112dd39 
  src/viewer/irccontextmenus.h 40756d1 
  src/viewer/irccontextmenus.cpp b735843 

Diff: http://git.reviewboard.kde.org/r/103551/diff/diff


Testing
-------

cmake with kdepim installed and with kdepim not installed
compiles tested with kdepim not installed: compiles, link and work fine


Thanks,

Thomas Capricelli

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/konversation-devel/attachments/20120107/637420c7/attachment.html>