> On June 25, 2013, 6: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.
> 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? - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111227/#review35015 ----------------------------------------------------------- On June 24, 2013, 10:49 p.m., Michael Zanetti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/111227/ > ----------------------------------------------------------- > > (Updated June 24, 2013, 10:49 p.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
