Review Request 112437: Port LogViewer to KPeople

[David Edmundson]

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



logviewer/log-viewer.cpp
<http://git.reviewboard.kde.org/r/112437/#comment28996>

    Why are you doing this? 
    This will make parsing the original model a lot lot harder than it needs to be.
    
    You have your own custom grouping of items anyway (see below comment), we're grouping items - then merging contacts with entity model - then grouping it again.
    



logviewer/person-entity-merge-model.cpp
<http://git.reviewboard.kde.org/r/112437/#comment28995>

    I should have spotted this on the first review.
    
    Why are you doing your own custom group grouping?
    
    We have a working proxy model for doing this without reinventing the wheel. That one supports multiple groups properly too. This one doesn't.
    
    It's very hard to do properly which is why I only want to do it once.
    
    
    
    



logviewer/person-entity-merge-model.cpp
<http://git.reviewboard.kde.org/r/112437/#comment28994>

    Why has this been added in the latest revision.
    
    Is this due to a lack of a sourceModelInitialised signal? We could have just added that in. I think I even hinted at that.
    A 5 line change vs a 300 line one.


I know you're seriously pro at programming, but you don't need to go deliberately out of your way to make life difficult for yourself.

The only change I was expecting was PeronsModel model to ContactsModel (without grouping) and a few role enums. 

What I want changed:
 - PersonsEntityMergeModel uses the ungrouped ContactsModel
 - Then use AbstractGroupingModel on the PersonsEntityMergeModel

I reckon this will halve the size of the code.. and the grouping will work better.



- David Edmundson


On Sept. 4, 2013, 7:01 a.m., Dan Vrátil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112437/
> -----------------------------------------------------------
> 
> (Updated Sept. 4, 2013, 7:01 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> I have finished porting our beloved Log Viewer to KPeople and would like to merge it for 0.7.
> 
> The idea is that we take EntityModel (populated by entities from logger) and KPeople model and merge them into one model. This is done by an ugly beast called PersonEntityMergeModel. There is still some space for improvements (mainly performance), but generally it works and it is reasonably fast with cca 100 entities/contacts. I have also stolen code for tree view delegates from contact list, so that we are more consistent across KTp components.
> 
> Contacts are grouped by their group membership (retrieved from KPeople). When you select a Persona, dates from all subcontacts are loaded into the dates view and the Persona is expanded so that you can selected individual subcontacts.
> 
> The date picker has been replaced by a tree view with list of dates grouped by months. If there are logs from multiple subcontacts for one date, the date can be further expanded and you can pick specific subcontact.
> 
> When KPeople is disabled at build time or Nepomuk is not running, we fallback to current logviewer behavior, i.e. cntacts are grouped by accounts and display names are fetched from Telepathy (so it works only for accounts that are online).
> 
> As a new feature, the logger now has settings page which contains two tabs: one for theme configuration and one specifically for logviewer. So far it only has one option - you can pick whether you want to sort messages in logs from newest to oldest or vice versa. It's not related to KPeople, but I started working on it in the kpeople branch (don't ask me why), so there it is.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 8083bc2 
>   logviewer/CMakeLists.txt 300ba8f 
>   logviewer/config/CMakeLists.txt PRE-CREATION 
>   logviewer/config/behavior-config.h PRE-CREATION 
>   logviewer/config/behavior-config.cpp PRE-CREATION 
>   logviewer/config/behavior-config.ui PRE-CREATION 
>   logviewer/config/kcm_ktp_logviewer_behavior.desktop PRE-CREATION 
>   logviewer/conversation-date-picker.h 6e1bd60 
>   logviewer/conversation-date-picker.cpp b8db972 
>   logviewer/dates-model.h PRE-CREATION 
>   logviewer/dates-model.cpp PRE-CREATION 
>   logviewer/dates-view-delegate.h PRE-CREATION 
>   logviewer/dates-view-delegate.cpp PRE-CREATION 
>   logviewer/entity-filter-model.h PRE-CREATION 
>   logviewer/entity-filter-model.cpp PRE-CREATION 
>   logviewer/entity-model-item.h d7cb8ed 
>   logviewer/entity-model-item.cpp 4e2189d 
>   logviewer/entity-model.h f9bf293 
>   logviewer/entity-model.cpp 164f263 
>   logviewer/entity-proxy-model.h 837f4af 
>   logviewer/entity-proxy-model.cpp b57296b 
>   logviewer/entity-view-delegate.h PRE-CREATION 
>   logviewer/entity-view-delegate.cpp PRE-CREATION 
>   logviewer/entity-view.cpp 77fefc0 
>   logviewer/log-viewer.h 5820a88 
>   logviewer/log-viewer.cpp 909777d 
>   logviewer/log-viewer.rc 82df425 
>   logviewer/log-viewer.ui a76b34a 
>   logviewer/message-view.h 4e2e2bb 
>   logviewer/message-view.cpp b037900 
>   logviewer/person-entity-merge-model.h PRE-CREATION 
>   logviewer/person-entity-merge-model.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112437/diff/
> 
> 
> Testing
> -------
> 
> Browsed some logs, seems to work :)
> 
> 
> Thanks,
> 
> Dan Vrátil
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130904/fd81fa40/attachment.html>