> On June 25, 2013, 8:17 a.m., Pali Rohár wrote: > > Hello, nice work :-) > > > > Some my comments: > > * please make all coding style changes in separate git commit (it will be > > easier to review and read your patch) > > * also if you can move crash during key generation to separate git commit > > * and move also patch for unencrypted messages to separate git commit > > * this patch dropping libotr 3.x support, but lot of linux distributions > > does not have libotr 4.x - can you update patch to support both versions? I > > do not want to see that linux distributions drop otr support in kopete > > because they do not have needed libraries... > > > > For adding new strings, you need to contact kde-i18n-doc mailinglist and > > ask for it. > > Michael Zanetti wrote: > Now read the description again. > > > * please make all coding style changes in separate git commit (it will > be easier to review and read your patch) > > As the code was really badly formatted (no - we didn't have reviewboard > the time I wrote the plugin initially) and QtCreator automatically trims > spaces at the end of lines, I'm not going to manually add whitespaces at the > end of the lines to break that again. > > > * also if you can move crash during key generation to separate git > commit > I don't think it makes sense to split the fix of the crash into a > separate commit. Especially that code changes during the plugin porting > anyways. > > > * and move also patch for unencrypted messages to separate git commit > There is no patch for unecrypted messages. That was a change from libotr > 3.2 to libotr 4.0. > > > * this patch dropping libotr 3.x support, > No it is not. If you would have read at least the description you would > see that it is tested and works with Adium, which does only support OTR > protocol version v2. Heck, it even supports OTR protocol v1. > > > For adding new strings, you need to contact kde-i18n-doc mailinglist > and ask for it. > You mean to ask for an exception for getting it inot 4.11? > > Pali Rohár wrote: > Spliting patch into more smaller (whitespace, crash fix, ...) is just > suggestion, now when we have git (but is it not required). > In your patch, you increased in cmake minimal libotr version to 4.0. So > you dropped support for libotr 3.2 library at compile time. > And yes, for adding new strings you need exception. > > Michael Zanetti wrote: > > In your patch, you increased in cmake minimal libotr version to 4.0. So > you dropped support for libotr 3.2 library at compile time. > > Oh... you meant compile time... Yes. Its true. This requires 4.0.0 to > compile. However, I guess the reason for most distributions not shipping > libotr 4.0.0 yet is exactly because KDE has a hard dependency to 3.2.0 and > does not compile with 4.0.0 yet. I'm really not sure if its worth the effort > of having both versions supported as it would require #ifdef'ing all the > changes from this diff, sometimes in quite complex order. I think by the time > we would release this, libotr 4.0.0 has been out the door for more than half > a year. That should be enough for distributions to update. If they don't > update such a simple lib as libotr in half a year, they are not going to > upgrade their KDE version anyways. Once they realize KDE supports 4.0.0 they > will most likely upgrade their libotr and the 3.2.0 path just adds more > complexity without being used at all. > > Also, there is another problem... If we would support both versions at > compile time, then someone compiles it with 3.2.0, a later upgrade to libotr > 4.0.0 would lead to crashes as the API changes quite a bit. So it would not > only add maintainance efforts on our side, but add more complexeity for > distros to package. > > So I don't think that would be good. Do you think I should do that > nevertheless? > > > And yes, for adding new strings you need exception. > Is that the only exception needed to be able to push this to 4.11 or is > there something else needed? > > Pali Rohár wrote: > > Also, there is another problem... If we would support both versions at > compile time, then someone compiles it with 3.2.0, a later upgrade to libotr > 4.0.0 would lead to crashes as the API changes quite a bit. So it would not > only add maintainance efforts on our side, but add more complexeity for > distros to package. > > This will not happen, because libraries have soname. And kopete already > has #ifdef code to support old/new version of some libaries. If libotr api > was not changed too much, I'd like to see support for 3.2.0... > > > Is that the only exception needed to be able to push this to 4.11 or is > there something else needed? > > Another problem is hard feature freeze and dependences freeze - it is not > possible to update dependences on external libraries (increasing minimal > libotr version). > But because this is review request for bug fixing and specialy allow to > compile kopete with new version of library there is chance (after fixing new > strings) to include it into 4.11. But libotr 3.x cannot be dropped in 4.11. > > Michael Zanetti wrote: > Right... Ok... In that case I'd say the 4.11 train has gone. Lets target > for 4.12 then. > > Regarding the backwards compatibility to 3.2.0: this is the document > describing the API changes between 3.2.0 and 4.0.0: > http://www.cypherpunks.ca/otr/UPGRADING-libotr-4.0.0.txt > IMO too much to #ifdef...
Ok, then after KDE 4.11 branch will be created and kopete master branch will be ready for new features and changes, go ahead and commit this patch. I belive that in next 6 months distributions update libotr package to new version. - Pali ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111227/#review35015 ----------------------------------------------------------- On June 25, 2013, 12:49 a.m., Michael Zanetti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/111227/ > ----------------------------------------------------------- > > (Updated June 25, 2013, 12:49 a.m.) > > > Review request for Kopete. > > > Description > ------- > > This patch ports kopete-otr to libotr 4.0.0 > > Changes: > -> support for translating libotr-generated messagess. > -> incoming unencrypted messages during an ecrypted session can now be > threated as normal messages producing a notification and saved to history > (Bug 204502) > -> Along with the port I worked around the crash with incoming messages > during key generation (Bugs 195328, 218570, 298681, 304105, 306276, 309987, > 318255) > -> Multiple chat session to the same user with different resources can now be > distinguished > > Because the original code was rather badly formatted the change also contains > some formatting changes which I'm not sure what exactly is the difference. I > tried to make as few coding style changes as possible. > > > Oh, and it does add some new strings, but shouldn't change any existing ones. > The ones newly added are the ones that have been inside libotr before - > hardcoded to english. So even if we would ship this patch without updating > translations, translation wise it would look the same as before. So, do you > guys think we could squeeze it into 4.11 still? > > > This addresses bugs 195328, 204502, 218570, 298681, 304105, 306276, 309987, > and 318255. > http://bugs.kde.org/show_bug.cgi?id=195328 > http://bugs.kde.org/show_bug.cgi?id=204502 > http://bugs.kde.org/show_bug.cgi?id=218570 > http://bugs.kde.org/show_bug.cgi?id=298681 > http://bugs.kde.org/show_bug.cgi?id=304105 > http://bugs.kde.org/show_bug.cgi?id=306276 > http://bugs.kde.org/show_bug.cgi?id=309987 > http://bugs.kde.org/show_bug.cgi?id=318255 > > > Diffs > ----- > > CMakeLists.txt 18896be > cmake/modules/FindLibOTR.cmake e9dbf31 > plugins/otr/authenticationwizard.h a4dea40 > plugins/otr/authenticationwizard.cpp e4d5d13 > plugins/otr/kopete_otr.desktop 2429868 > plugins/otr/otrguiclient.h 0347075 > plugins/otr/otrguiclient.cpp 11329db > plugins/otr/otrlchatinterface.h 5bb6e1d > plugins/otr/otrlchatinterface.cpp 56ffd0c > plugins/otr/otrlconfinterface.h 0cfdf29 > plugins/otr/otrlconfinterface.cpp 7d58462 > plugins/otr/otrplugin.h 35ecb7b > plugins/otr/otrplugin.cpp ef973ed > plugins/otr/otrpreferences.h 7aa742c > plugins/otr/otrpreferences.cpp ece403a > plugins/otr/privkeypopup.h 4b271e9 > plugins/otr/privkeypopup.cpp 3fea9e2 > > Diff: http://git.reviewboard.kde.org/r/111227/diff/ > > > Testing > ------- > > Tested with XMPP against Pidgin (OTRv3) and Adium (OTRv2). The overall > handling of messages (escaping, parsing etc) did not change with this patch, > so it shouldn't break any other protocol. Still, some more testing wouldn't > hurt if you have some other protocols in use you can test with. > > > Thanks, > > Michael Zanetti > >
_______________________________________________ kopete-devel mailing list kopete-devel@kde.org https://mail.kde.org/mailman/listinfo/kopete-devel
