----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103629/#review9558 -----------------------------------------------------------
Enough comments to keep you busy for a while :-) lib/conversation-target.h <http://git.reviewboard.kde.org/r/103629/#comment7816> This class is full of potential crashes. By default this class is made with a Tp::ContactPtr which doesn't point to any actual contact that means all the code that says d->contact->presence()->something() will (probably) just explode horribly if the contact isn't set. Is there a reason for needing a setContact() method, rather than just forcing you to set a valid one in the constructor? If not - simplify your code, just force a valid Tp::ContactPtr in the constructor. If so - we need to fix the code up in the accessors to be safer. lib/conversation-target.h <http://git.reviewboard.kde.org/r/103629/#comment7817> const Tp::ContactPtr &contact lib/conversation-target.h <http://git.reviewboard.kde.org/r/103629/#comment7818> still put const & stuff in slots. (though don't put them in your "connect(this, SIGNAL( ..." line. lib/conversation-target.cpp <http://git.reviewboard.kde.org/r/103629/#comment7821> QIcon -> KIcon lib/conversation-target.cpp <http://git.reviewboard.kde.org/r/103629/#comment7820> Why would you need the icon name in rather than a KIcon? lib/conversation-target.cpp <http://git.reviewboard.kde.org/r/103629/#comment7819> I'd rather we used Ktp::PResence here (that's new, I'll help with that) lib/conversation.h <http://git.reviewboard.kde.org/r/103629/#comment7824> When could this happen? (or the one beneath) lib/conversations-model.cpp <http://git.reviewboard.kde.org/r/103629/#comment7822> data is a poor name for a variable, it could be anything. lib/conversations-model.cpp <http://git.reviewboard.kde.org/r/103629/#comment7823> IIRC you need to check channel types here too. Check code from main Text-UI. lib/messages-model.h <http://git.reviewboard.kde.org/r/103629/#comment7825> Actually you can. (such as the int that you have in the line above!) But you would never need to deal with textChannel stuff directly from QML, so I would delete this. lib/messages-model.h <http://git.reviewboard.kde.org/r/103629/#comment7826> Whitespace in this class is a bit mental. lib/messages-model.h <http://git.reviewboard.kde.org/r/103629/#comment7827> it's fine to keep debug code in here, but comment it as such. lib/messages-model.h <http://git.reviewboard.kde.org/r/103629/#comment7828> a slot that is non-void is unusual. lib/messages-model.cpp <http://git.reviewboard.kde.org/r/103629/#comment7829> I bet there's a proper method to do this. lib/messages-model.cpp <http://git.reviewboard.kde.org/r/103629/#comment7830> que -> queue - David Edmundson On Jan. 4, 2012, 1:47 p.m., Lasath Fernando wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103629/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2012, 1:47 p.m.) > > > Review request for Telepathy and David Edmundson. > > > Description > ------- > > Right, I decided that I should have reviewed and/or merge what work I've done > so far rather than letting it pile up in its corner into a giant worthless > clump. > > To the people that haven't heard (or wasn't at woshibon), this is a chat > plasmoid that more or less behaves like facebook and google talk, except it > sits in your taskbar :) > > And in terms of feedback, at this stage I think design issues should take > priority over sane code because the main reason I'm doing this is because I > don't want to have to do any massive restructuring later on. > > And if things don't make sense, ask me (I didn't comment/document anything > well and I'm certainly too sleepy now ;) > > PS: All the code is in my scratch repo > http://quickgit.kde.org/?p=clones%2Ftelepathy-text-ui%2Ffernando%2Fqmlplugin.git&a=shortlog&h=refs/heads/qml_plugins2 > > > Diffs > ----- > > CMakeLists.txt d1cc185 > lib/CMakeLists.txt 5d39a62 > lib/chat-window-style.cpp 7b770cf > lib/conversation-que-manager.h PRE-CREATION > lib/conversation-que-manager.cpp PRE-CREATION > lib/conversation-target.h PRE-CREATION > lib/conversation-target.cpp PRE-CREATION > lib/conversation.h PRE-CREATION > lib/conversation.cpp PRE-CREATION > lib/conversations-model.h PRE-CREATION > lib/conversations-model.cpp PRE-CREATION > lib/messages-model.h PRE-CREATION > lib/messages-model.cpp PRE-CREATION > lib/qml-plugins.h PRE-CREATION > lib/qml-plugins.cpp PRE-CREATION > lib/qmldir PRE-CREATION > lib/telepathy-text-observer.h PRE-CREATION > lib/telepathy-text-observer.cpp PRE-CREATION > plasmoid/CMakeLists.txt PRE-CREATION > plasmoid/contents/ui/ChatWidget.qml PRE-CREATION > plasmoid/contents/ui/ConversationDelegate.qml PRE-CREATION > plasmoid/contents/ui/TextDelegate.qml PRE-CREATION > plasmoid/contents/ui/main.qml PRE-CREATION > plasmoid/metadata.desktop PRE-CREATION > > Diff: http://git.reviewboard.kde.org/r/103629/diff/diff > > > Testing > ------- > > Um.. yeah... about that... :/ > > > Thanks, > > Lasath Fernando > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
