Review Request: Baby steps for Chat Plasmoid...

Sat Jan 14 13:25:35 UTC 2012

> On Jan. 11, 2012, 9:35 p.m., David Edmundson wrote:
> > lib/CMakeLists.txt, line 27
> > <http://git.reviewboard.kde.org/r/103629/diff/5/?file=46307#file46307line27>
> >
> >     Try not to commit commented out code.
> >     
> >     (also if this is what I think it's for, I'm not sure it's a great name)

Yeah I'll get around to building those eventually. And hell, half my classes could use better names - I just can't think up of any.

> On Jan. 11, 2012, 9:35 p.m., David Edmundson wrote:
> > lib/qml-plugins.cpp, line 29
> > <http://git.reviewboard.kde.org/r/103629/diff/5/?file=46335#file46335line29>
> >
> >     You really don't need to register all these types.
> >     
> >     Only register the stuff that you should be able to create in QML.
> >     
> >     Especially when actually using it would result in immediately throwing a kError. (which applies to at least one of these classes)
> >     
> >     I think you only need ConversationsModel. Though you will then need to fix a few things just like we did with conversationtarget.
> >     Even for your single person chat, you're only going to have one entry point, you'd never construct a messagesModel (etc) from inside QML.
> >     
> >     Plus you'll be able to tidy up some stuff because of it.

// this can be used in QML because it spits out Conversations which
// can be given to ChatWidget.qml
qmlRegisterType<TelepathyTextObserver> (uri, 0, 1, "TelepathyTextObserver");
// this is currently instanitized and used by the multi-user plasmoid
qmlRegisterType<ConversationsModel>(uri, 0, 1, "ConversationsModel");
// this isn't instanitized directly, but need to be registered,
// because ChatWidget has a property of type Conversation
qmlRegisterType<Conversation>(uri, 0, 1, "Conversation");

// these two are used, but not instanitized within QML
// and it turns out calling qmlRegisterType with no arguments
// does exactly that
qmlRegisterType<ConversationTarget>();
qmlRegisterType<MessagesModel> ();

> On Jan. 11, 2012, 9:35 p.m., David Edmundson wrote:
> > plasmoid/contents/ui/main.qml, line 95
> > <http://git.reviewboard.kde.org/r/103629/diff/5/?file=46343#file46343line95>
> >
> >     No.

Alright, it outlived its usefulness a while ago. Will remove.

> On Jan. 11, 2012, 9:35 p.m., David Edmundson wrote:
> > plasmoid/contents/ui/main.qml, line 74
> > <http://git.reviewboard.kde.org/r/103629/diff/5/?file=46343#file46343line74>
> >
> >     try to avoid committing large chunks of commented out code.

Yeah, I'm going to go through all the QML code and clean it up soon.

Okay, what I actually ended up doing was renaming model to messages and removing all commented code. If I try to refractor everything at once, I will loose my sanity. 

- Lasath

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103629/#review9755
-----------------------------------------------------------

On Jan. 10, 2012, 10:01 a.m., Lasath Fernando wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103629/
> -----------------------------------------------------------
> 
> (Updated Jan. 10, 2012, 10:01 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
> -----
> 
>   CMakeLists.txt d1cc185 
>   lib/CMakeLists.txt 5d39a62 
>   lib/adium-theme-content-info.h e293732 
>   lib/adium-theme-header-info.h 952dc48 
>   lib/adium-theme-message-info.h 10c00a2 
>   lib/adium-theme-status-info.h 926586a 
>   lib/adium-theme-view.h c2fae74 
>   lib/adium-theme-view.cpp c834c94 
>   lib/chat-search-bar.h 315fd2b 
>   lib/chat-search-bar.cpp 15aa5da 
>   lib/chat-text-edit.cpp b0476f5 
>   lib/chat-widget.h 30afa31 
>   lib/chat-widget.cpp 9313c03 
>   lib/chat-window-style-manager.h c876ba8 
>   lib/chat-window-style-manager.cpp 8604c97 
>   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/logmanager.h 4e11086 
>   lib/logmanager.cpp 61484af 
>   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/20120114/67cccf91/attachment.html>