> On Jan. 5, 2012, 1:15 a.m., David Edmundson wrote: > > lib/conversation-target.h, line 45 > > <http://git.reviewboard.kde.org/r/103629/diff/1/?file=45524#file45524line45> > > > > 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. > > Lasath Fernando wrote: > The reason it's there is that any class that's exported to QML has to > have a default constructor. > Does that mean I'll need to check if it's valid before *every* time I use > it?
Ah, I've encountered that before. Though I don't think it applies in this situation, it only needs a default constructor if it's exported as something you can create from within QML, and this isn't. I don't /think/ you need this class in qml-plugins::registerTypes. Only the ConversationsModel which is the entry point for all this . > On Jan. 5, 2012, 1:15 a.m., David Edmundson wrote: > > lib/conversation-target.cpp, line 75 > > <http://git.reviewboard.kde.org/r/103629/diff/1/?file=45525#file45525line75> > > > > Why would you need the icon name in rather than a KIcon? > > Lasath Fernando wrote: > Because the ToolButton in Plasma Components 4.8 didn't give an option to > set an Icon directly. It has a property for Icon name and uses an internal > KIconLoader. Stupid toolbutton. Fair enough. > On Jan. 5, 2012, 1:15 a.m., David Edmundson wrote: > > lib/conversation.h, line 52 > > <http://git.reviewboard.kde.org/r/103629/diff/1/?file=45526#file45526line52> > > > > When could this happen? > > > > (or the one beneath) > > Lasath Fernando wrote: > I wanted this to be reusable. > And also, I thought it would become useful if a channel becomes invalid > and and/or its model needs to be replaced. > > Although, I realise that I'd forgotten to make a setter. > I figure it might be useful later on when the single contact plasmoids > come around, since they'll only have one instance of Conversation. but the model already has a setTextChannel() on it, so there's no need to ever replace the model instance. If it turns out they make sense in the future, add them. Right now it's confusing. > On Jan. 5, 2012, 1:15 a.m., David Edmundson wrote: > > lib/messages-model.h, line 74 > > <http://git.reviewboard.kde.org/r/103629/diff/1/?file=45530#file45530line74> > > > > a slot that is non-void is unusual. > > Lasath Fernando wrote: > I can't really remember the reason I did this. Should I drop it? yes. > On Jan. 5, 2012, 1:15 a.m., David Edmundson wrote: > > lib/messages-model.h, line 36 > > <http://git.reviewboard.kde.org/r/103629/diff/1/?file=45530#file45530line36> > > > > 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. > > Lasath Fernando wrote: > Yes, but I think it needs to be a primitive type or a QObject. I > commented out this line because it gave me compiler errors. It needs to be a registered qmetatype, but as I said - not a useful property in QML. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103629/#review9558 ----------------------------------------------------------- On Jan. 5, 2012, 8:17 a.m., Lasath Fernando wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103629/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2012, 8:17 a.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 > ----- > > lib/chat-window-style.cpp 7b770cf > CMakeLists.txt d1cc185 > lib/CMakeLists.txt 5d39a62 > 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
