Review Request: Baby steps for Chat Plasmoid...

David Edmundson kde at davidedmundson.co.uk
Thu Jan 5 12:16:50 UTC 2012 


> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120105/5232c027/attachment-0001.html>

More information about the KDE-Telepathy mailing list