Review Request: [ktp-log-viewer] Global Search

[David Edmundson]

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


Adding global search is awesome \o/. Well done, it's much needed.
Unfortunately I have some gripes about how you're breaking model separation.

I think I would:
 - not touch entity-model 
 - have a QList<Tp::SearchHit> in the LogViewer class.
 - in LogViewer::globalSearchFinished update the LogViewer search hits. This updates the entityFilter just like you wrote it.

 - in LogViewer::onEntitySelected() if m_searchHits.size().. search for the appropriate SearchHit in m_searchHits, then set the datepicker dates here.
To make the API simpler, you could even consider passing the SearchHits directly to the EntityFilterModel and the DatePicker and it's up to these classes to search through them. As a QList is implicitly shared so it's arguably faster than copying into a different list. So it becomes up to the DatePicker::setEntity() to say "do I have any search hits? I'll go look through them and choose the right dates. If not I'll start my pendingDates.". 


logviewer/entity-model-item.cpp
<http://git.reviewboard.kde.org/r/105586/#comment12563>

    Storing the list of dates that match a keyword unrelated to this model in this model makes no sense to me. 
    
    It's completely destroyed any data-view separation.



logviewer/entity-model.h
<http://git.reviewboard.kde.org/r/105586/#comment12562>

    I'd rather we kept EntityModelItem prviate.
    
    We did this sort of thing in one of the other KTp models, and it's led to all sorts of problems.



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

    when you call setEntity() on the datePicker it starts a query for all dates for this contact..destroying what you pass it.


- David Edmundson


On July 15, 2012, 9:50 p.m., Dan Vratil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105586/
> -----------------------------------------------------------
> 
> (Updated July 15, 2012, 9:50 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Added a KLineEdit to the bottom part of the window. Hitting enter starts global search for given term. Only entities with at least one matching log are displayed. Only dates with matching logs are displayed. Matching terms are highlighted in the MessageView. I am cool.
> 
> 
> This addresses bug 294652.
>     http://bugs.kde.org/show_bug.cgi?id=294652
> 
> 
> Diffs
> -----
> 
>   logviewer/conversation-date-picker.h 868eb43 
>   logviewer/conversation-date-picker.cpp 3817083 
>   logviewer/entity-model-item.h e740d25 
>   logviewer/entity-model-item.cpp c7f2249 
>   logviewer/entity-model.h 205c132 
>   logviewer/entity-model.cpp cbee00a 
>   logviewer/entity-proxy-model.h 0548081 
>   logviewer/entity-proxy-model.cpp ac1bbf5 
>   logviewer/log-viewer.h 2b13bc1 
>   logviewer/log-viewer.cpp 04fba29 
>   logviewer/log-viewer.ui 7e4097f 
>   logviewer/message-view.h 9e01260 
>   logviewer/message-view.cpp 1f51a83 
> 
> Diff: http://git.reviewboard.kde.org/r/105586/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dan Vratil
> 
>

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