[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-pim
Subject: Re: [Kde-pim] Review Request 119851: Outline for Attachment Controller for Mail API,
From: "Kevin Krammer" <krammer () kde ! org>
Date: 2014-08-21 8:12:41
Message-ID: 20140821081241.26013.31573 () probe ! kde ! org
[Download RAW message or body]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119851/#review64954
-----------------------------------------------------------
mobile/api/mail/attachmentcontroller.h
<https://git.reviewboard.kde.org/r/119851/#comment45383>
just <QObject> and move the Qt includes after the KDE includes
mobile/api/mail/attachmentcontroller.h
<https://git.reviewboard.kde.org/r/119851/#comment45384>
just <KUrl>
mobile/api/mail/attachmentcontroller.h
<https://git.reviewboard.kde.org/r/119851/#comment45386>
it is customary to let QObject sub classes have an optional QObject pointer \
constructor argument.
explicit AttachmentController(QObject *parent = 0)
mobile/api/mail/attachmentcontroller.h
<https://git.reviewboard.kde.org/r/119851/#comment45385>
any reason this is public?
mobile/api/mail/attachmentcontroller.h
<https://git.reviewboard.kde.org/r/119851/#comment45393>
seems unused
mobile/api/mail/attachmentcontroller.h
<https://git.reviewboard.kde.org/r/119851/#comment45394>
probably a good idea to make that into a Q_PROPERTY
mobile/api/mail/attachmentcontroller.cpp
<https://git.reviewboard.kde.org/r/119851/#comment45387>
wParent and signEnabled remain uninitialized. Pointers should at least be \
initialized to 0, the bool probably to false
mobile/api/mail/attachmentcontroller.cpp
<https://git.reviewboard.kde.org/r/119851/#comment45388>
Is it no possible to pass 0 as the parent?
Ultimately we need to find a non-widget way of doing this, but lets go with this \
dialog for now
mobile/api/mail/attachmentcontroller.cpp
<https://git.reviewboard.kde.org/r/119851/#comment45389>
style nitpick: spaces around operators = and <
mobile/api/mail/attachmentcontroller.cpp
<https://git.reviewboard.kde.org/r/119851/#comment45390>
style nitpick: space after keyword "if"
mobile/api/mail/attachmentcontroller.cpp
<https://git.reviewboard.kde.org/r/119851/#comment45392>
space after keyword "foreach"
mobile/api/mail/attachmentcontroller.cpp
<https://git.reviewboard.kde.org/r/119851/#comment45391>
is it possible to have part as a const reference here?
- Kevin Krammer
On Aug. 20, 2014, 1:24 vorm., Abhijeet Nikam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119851/
> -----------------------------------------------------------
>
> (Updated Aug. 20, 2014, 1:24 vorm.)
>
>
> Review request for KDEPIM, Kevin Krammer and Michael Bohlender.
>
>
> Repository: kdepim
>
>
> Description
> -------
>
> I have create an attachment controller for the API, to avoid using KAction, but is \
> this approach ok? And is there a other way around instead of initializing QWidget \
> object from the mail composer funtion and passing it to Attachment Controller? I \
> still have to extend it and complete the remaining parts in it.
>
> Diffs
> -----
>
> mobile/api/mail/CMakeLists.txt d280d62
> mobile/api/mail/attachmentcontroller.h PRE-CREATION
> mobile/api/mail/attachmentcontroller.cpp PRE-CREATION
> mobile/api/mail/composer.h b731287
> mobile/api/mail/composer.cpp dc750e8
>
> Diff: https://git.reviewboard.kde.org/r/119851/diff/
>
>
> Testing
> -------
>
> For saveDraft, sendLater and ShowAttachmentDialog done, others yet to be tested.
>
>
> Thanks,
>
> Abhijeet Nikam
>
>
_______________________________________________
KDE PIM mailing list kde-pim@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic