Review Request: Merge request for telepathy-kde-contactlist

[Martin Klapetek]

> On March 11, 2011, 12:46 a.m., David Edmundson wrote:
> > accountfiltermodel.cpp, line 51
> > <http://git.reviewboard.kde.org/r/100838/diff/2/?file=11004#file11004line51>
> >
> >     It seems a bit wrong to reimplement text filtering in something that inherits from QSortFilterProxyModel. For someone using this class there are two methods to set a text filter. One that works, one that doesn't. 
> >     
> >     Your reasoning not to made sense
> >     (default behaviour is recursive so will not show any contacts if the account name doesn't match the string)
> >     
> >     but maybe we can get round that by calling
> >     QSortFilterProxyMode::filterAcceptsRow(source_row, source_parent) here, where we know it's a contact.

>From my understanding of the docs, you can either use the regexp. filtering _or_ to reimplement the filterAcceptsRow by yourself. Since we're already using it for filtering the offline contacts, there's no point in setting the regexp for filtering (which IMHO calls filterAcceptsRow anyway). So it is all implemented in filterAcceptsRow.


> On March 11, 2011, 12:46 a.m., David Edmundson wrote:
> > accountbutton.cpp, line 34
> > <http://git.reviewboard.kde.org/r/100838/diff/2/?file=11002#file11002line34>
> >
> >     This whole class is very weird.
> >     
> >     Never write 'magic numbers' i.e 3 or 7 just unexplained throughout the code.
> >     Storing a C array of every possible enum value seems a wrong approach.
> >     
> >     I would store the presence status() - the string (which you can use as an ID) as the data() field of the QAction.
> >     
> >     onlineAction->setData(Presence::Available().status());
> >     
> >     (you might even be able to just store the Tp::Presence object in there if it's a Q_METATYPE (try it and see). )
> >     
> >     Then you can use this to create a proper presence from.
> >     
> >     Also your presence lists have no profile support - this can be a future thing so I'll explain that later.

Alright, I fixed this by setting the Tp::Presence as the data() of actions. This works surprisingly well. I'll update the diff soon.


> On March 11, 2011, 12:46 a.m., David Edmundson wrote:
> > accountbutton.cpp, line 58
> > <http://git.reviewboard.kde.org/r/100838/diff/2/?file=11002#file11002line58>
> >
> >     This is done for you:
> >     
> >     from the docs:
> >     ----
> >     Tp::Account::iconName();
> >     As a last resort, "im-" + protocolName() will be returned.
> >

Fixed.


> On March 11, 2011, 12:46 a.m., David Edmundson wrote:
> > contactdelegate.cpp, line 79
> > <http://git.reviewboard.kde.org/r/100838/diff/2/?file=11020#file11020line79>
> >
> >     hardcoded values here. At the end of this there should be very few fixed numbers.

Yes, the delegate needs some work regarding the hardcoded values. Both positions and font metrics. Anyone up to it?


- Martin


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


On March 11, 2011, 12:27 a.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100838/
> -----------------------------------------------------------
> 
> (Updated March 11, 2011, 12:27 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This is the work that begun in my clone repo and has matured enough now to be merged back to 'upstream', where the work should continue now.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 2c62ea10556cdba38d1bca0fe63603df97414336 
>   accountbutton.h PRE-CREATION 
>   accountbutton.cpp PRE-CREATION 
>   accountfiltermodel.h PRE-CREATION 
>   accountfiltermodel.cpp PRE-CREATION 
>   accounts-model-item.h PRE-CREATION 
>   accounts-model-item.cpp PRE-CREATION 
>   accounts-model.h PRE-CREATION 
>   accounts-model.cpp PRE-CREATION 
>   avatars/astronaut.jpg PRE-CREATION 
>   avatars/chess.jpg PRE-CREATION 
>   avatars/coffee.jpg PRE-CREATION 
>   avatars/dice.jpg PRE-CREATION 
>   avatars/fish.jpg PRE-CREATION 
>   avatars/lightning.jpg PRE-CREATION 
>   avatars/soccerball.png PRE-CREATION 
>   config.h.cmake PRE-CREATION 
>   contact-model-item.h PRE-CREATION 
>   contact-model-item.cpp PRE-CREATION 
>   contactdelegate.h PRE-CREATION 
>   contactdelegate.cpp PRE-CREATION 
>   contactdelegateoverlay.h PRE-CREATION 
>   contactdelegateoverlay.cpp PRE-CREATION 
>   contactoverlays.h PRE-CREATION 
>   contactoverlays.cpp PRE-CREATION 
>   contactviewhoverbutton.h PRE-CREATION 
>   contactviewhoverbutton.cpp PRE-CREATION 
>   filterbar.h PRE-CREATION 
>   filterbar.cpp PRE-CREATION 
>   main-widget.h aed6f7c8336321bc8a6ffb3b9af6b1d493f85790 
>   main-widget.cpp 6146b62eec17b54be63b594200613931673ac5fe 
>   main-widget.ui 5b0d5aaaf3a4e2f49eb030b98b15fcae3a86170c 
>   main.cpp bba0e4175e8853afe603f26ea4707f4974192d0f 
>   ontologies/CMakeLists.txt 3e0bbe8c634908f689dcd360409aeddcc6fc0d23 
>   tree-node.h PRE-CREATION 
>   tree-node.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/100838/diff
> 
> 
> Testing
> -------
> 
> We did a lot of thourough testing on #kde-telepathy, also some people emailed their reports which all has been fixed.
> 
> 
> Thanks,
> 
> Martin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110311/f6865780/attachment.htm