Review Request 119722: Added Emails Plugin

Tue Aug 12 08:54:07 UTC 2014

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119722/#review64352
-----------------------------------------------------------

This is some fine code, thanks! Couple comments below

src/widgets/personemailview.h
<https://git.reviewboard.kde.org/r/119722/#comment44946>

    This is a library, so LGPL ;)

src/widgets/personemailview.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44947>

    The "m_" prefix stands for "class member variable" and it's used in simple classes to clearly denote what's class var and what's local.

    When using d-pointers, we usually leave the m_ part out, because you'll be using it as "d->", it's easier to read without the m_ in this case.

    Also the m_emails is actually of type Emails, so change that too

src/widgets/personemailview.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44949>

    Is it really needed to delete and recreate the m_mainWidget? I think it would be easier (and faster) to just deleteLater() the widget you get from the plugin

    Alternatively, as the widget is basically a view with a model, even the listview could stay and just the model could be recreated, this would have to be done in the emails.cpp method tho...this is an optimization that can be done later however

src/widgets/personemailview.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44950>

    Don't use FormLayout if you don't have a form, rather reuse the layout() directly

src/widgets/plugins/emaillistmodel.h
<https://git.reviewboard.kde.org/r/119722/#comment44951>

    Struct names, just like classes, should start with capital letter --> Email

    I'd also suggest to name it EmailItem, so it's clear it's an item in the model (it's sort of a convention)

src/widgets/plugins/emaillistmodel.h
<https://git.reviewboard.kde.org/r/119722/#comment44958>

    What is "desc" actually? is it the truncated text? let's name it properly then as "desc" is really nondescriptive.

    Suggest to use "body"

src/widgets/plugins/emaillistmodel.h
<https://git.reviewboard.kde.org/r/119722/#comment44952>

    Enum name should also be capitalized, same goes for its values eg

    enum DataRole {
        MailSubjectRole = ...,
        ...
    }

    Also, in fact, remove the Mail prefix in the enum values, you'll be using it as EmailsListModel:: and it will be clear it belongs to emails, the Mail is a bit redundant in the name then

src/widgets/plugins/emaillistmodel.h
<https://git.reviewboard.kde.org/r/119722/#comment44953>

    the "struct email mail" should become "const EmailItem &mail" ;)

src/widgets/plugins/emaillistmodel.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44959>

    Check if parent is valid and return 0 if not

src/widgets/plugins/emaillistmodel.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44955>

    Missing {}s

src/widgets/plugins/emaillistmodel.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44956>

    You don't need to put "struct" in front of every usage of that struct

src/widgets/plugins/emaillistmodel.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44957>

    Don't do this, each time the view asks for a certain role, you'll be processing always everything --> slow

    Instead right after you obtain the EmailItem from the list, use 

    switch (role) {
        case MailSubjectRole:
            ...subject processing...
            return subject;
        case MailDescRole:
            return mail.desc;
    }

src/widgets/plugins/emaillistmodel.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44960>

    No seconds, that's quite useless in our preview :)

    (and more data in a view means less clean interface)

src/widgets/plugins/emaillistmodel.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44954>

    This is missing the {}s

    also put "No subject" in i18n(..)

src/widgets/plugins/emaillistmodel.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44961>

    This should actually be handled by the delegate, more below

src/widgets/plugins/emaillistmodel.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44962>

    If you're appending just one item, the last parameter must be the same as the second, because the "first position" of the inserted items will be the same as the "last position" (the third arg) because you're inserting just one item ;)

    tl;dr - remove the +1

src/widgets/plugins/emaillistmodel.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44963>

    Each new file must also have an empty new line at the end

src/widgets/plugins/emaillistviewdelegate.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44964>

    Use fm.elidedText(desc, Qt::ElideRight, descRect.width()) instead of the "desc" argument

    This will make sure the text will always scale with the view

    (also please rename the desc to body here too)

src/widgets/plugins/emails.h
<https://git.reviewboard.kde.org/r/119722/#comment44948>

    Can you fill the variable names here please (to all the methods)? It makes it easier to understand when opening the header files

src/widgets/plugins/emails.h
<https://git.reviewboard.kde.org/r/119722/#comment44965>

    Private vars should always go last, even after private Q_SLOTS

    also, this is the place where m_ prefixes should be used ;)

    finally, rename *me to *emailsModel

src/widgets/plugins/emails.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44966>

    You're instantiating it with an empty list, that's no use

    I'd either suggest to remove this constructor overload altogether and use only the default one (also pass "this" as parent otherwise it leaks) and then add the data to the model using the addEmail() method OR construct the model like this only when you actually have the list of models (inside jobFinished())

src/widgets/plugins/emails.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44967>

    You can also bulk-fetch the items with one single job, it would actually be better in terms of efficiency, you can either create a list of Items or list of Item ids and pass it to the ItemFetchJob, then you'll receive a list of items

src/widgets/plugins/emails.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44968>

    Since you'll have to first build a list of items in the while loop above, you can get rid of the hasMsg and replace it with list's isEmpty()

src/widgets/plugins/emails.cpp
<https://git.reviewboard.kde.org/r/119722/#comment44969>

    i18n

- Martin Klapetek

On Aug. 11, 2014, 11:29 p.m., Nilesh Suthar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119722/
> -----------------------------------------------------------
> 
> (Updated Aug. 11, 2014, 11:29 p.m.)
> 
> 
> Review request for Telepathy and Martin Klapetek.
> 
> 
> Repository: libkpeople
> 
> 
> Description
> -------
> 
> Email are listed in QListview and fetch with the help of baloo
> To be tested with http://quickgit.kde.org/?p=scratch%2Fnileshsuthar%2Fkpeople.git
> 
> 
> Diffs
> -----
> 
>   src/widgets/plugins/emaillistmodel.cpp PRE-CREATION 
>   src/widgets/plugins/emaillistviewdelegate.h PRE-CREATION 
>   src/widgets/plugins/emaillistviewdelegate.cpp PRE-CREATION 
>   src/widgets/plugins/emails.h PRE-CREATION 
>   src/widgets/plugins/emails.cpp PRE-CREATION 
>   src/widgets/plugins/emaillistmodel.h PRE-CREATION 
>   src/widgets/personemailview.cpp PRE-CREATION 
>   src/widgets/personemailview.h PRE-CREATION 
>   CMakeLists.txt b599284 
>   src/widgets/CMakeLists.txt f637838 
> 
> Diff: https://git.reviewboard.kde.org/r/119722/diff/
> 
> 
> Testing
> -------
> 
> Working as needed
> 
> 
> Thanks,
> 
> Nilesh Suthar
> 
>

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