[Kde-pim] Review Request: add the "Open With ..." button to the attachment handling dialog

[Martin Koller]

> On 2009-08-04 19:44:50, Thomas McGuire wrote:
> > I like having a "Open With" button, was missing that in several cases already.
> > What I don't really like is the strange mix of KMessagebox and KDialog, can't this be made easier, like only a KMessageBox?

I also wanted to use only KMessagebox, but it's missing the possibility for a fourth button.
So I had to go that (ugly) way to use a KDialog/KMessageBox combination.


> On 2009-08-04 19:44:50, Thomas McGuire wrote:
> > trunk/KDE/kdepim/kmail/attachmentdialog.h, line 30
> > <http://reviewboard.kde.org/r/1219/diff/1/?file=9613#file9613line30>
> >
> >     Please enclose the class in the KMail namespace, to avoid symbol clashes.

done


> On 2009-08-04 19:44:50, Thomas McGuire wrote:
> > trunk/KDE/kdepim/kmail/attachmentdialog.cpp, line 1
> > <http://reviewboard.kde.org/r/1219/diff/1/?file=9614#file9614line1>
> >
> >     Missing copyright.

added


> On 2009-08-04 19:44:50, Thomas McGuire wrote:
> > trunk/KDE/kdepim/kmail/attachmentdialog.cpp, line 22
> > <http://reviewboard.kde.org/r/1219/diff/1/?file=9614#file9614line22>
> >
> >     !isEmpty() seems more easy to read

ok


> On 2009-08-04 19:44:50, Thomas McGuire wrote:
> > trunk/KDE/kdepim/kmail/attachmentdialog.h, line 31
> > <http://reviewboard.kde.org/r/1219/diff/1/?file=9613#file9613line31>
> >
> >     why don't you derive from KDialog directly?
> >     Then you don't need your custom exec(), KDialog pointer and such.
> >     Actually, I don't see why a KDialog should be used either, I thought KMessagebox can handle all this?

because I need to call  KMessageBox::createKMessageBox( ) which opens the dialog and enters the local eventloop - 
and I don't want the user of this class to have a need to call it, and exec() is not virtual.

I could do it completely different by deriving from KDialog but then not using KMessageBox at all - which I think
is not a good idea as it handles layouting in a KDE consistent way.


> On 2009-08-04 19:44:50, Thomas McGuire wrote:
> > trunk/KDE/kdepim/kmail/kmreaderwin.cpp, line 2446
> > <http://reviewboard.kde.org/r/1219/diff/1/?file=9615#file9615line2446>
> >
> >     QString() instead of ""

ok


> On 2009-08-04 19:44:50, Thomas McGuire wrote:
> > trunk/KDE/kdepim/kmail/attachmentdialog.h, line 47
> > <http://reviewboard.kde.org/r/1219/diff/1/?file=9613#file9613line47>
> >
> >     coding style: spaces inside of parenthesis

fixed


> On 2009-08-04 19:44:50, Thomas McGuire wrote:
> > trunk/KDE/kdepim/kmail/attachmentdialog.cpp, line 13
> > <http://reviewboard.kde.org/r/1219/diff/1/?file=9614#file9614line13>
> >
> >     read http://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics.
> >     after that, you can use much better markup than '%1'. But that is no requirement, only a suggestion.

I had a look - but ... phew ... I don't know what else I would use here...


> On 2009-08-04 19:44:50, Thomas McGuire wrote:
> > trunk/KDE/kdepim/kmail/kmreaderwin.cpp, line 2469
> > <http://reviewboard.kde.org/r/1219/diff/1/?file=9615#file9615line2469>
> >
> >     coding style: spaces inside of parenthesis, e.g. ( choice == AttachmentDialog::Open ) instead of (choice == AttachmentDialog::Open) (also in other places)

fixed


> On 2009-08-04 19:44:50, Thomas McGuire wrote:
> > trunk/KDE/kdepim/kmail/attachmentdialog.cpp, line 43
> > <http://reviewboard.kde.org/r/1219/diff/1/?file=9614#file9614line43>
> >
> >     I thought the don't ask again stuff is handled implicitly by kmessagebox?

Not when I call createKMessageBox().
BTW: I now explicitely changed the config keyword to _not_ use the previous kmails's settings, as the number of
buttons has changed and I want to avoid that an action is triggered which is now different than the one before.
And I implemented the config read/write now directly as the previous call to the KMessageBox method was not appropriate.


- Martin


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


On 2009-08-05 13:19:04, Martin Koller wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1219/
> -----------------------------------------------------------
> 
> (Updated 2009-08-05 13:19:04)
> 
> 
> Review request for KDE PIM.
> 
> 
> Summary
> -------
> 
> According to https://bugs.kde.org/show_bug.cgi?id=201374
> add the "Open With ..." button to the attachment handling dialog even when KDE found a matching application
> to open the attachment with.
> 
> 
> This addresses bug 201374.
>     https://bugs.kde.org/show_bug.cgi?id=201374
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdepim/kmail/CMakeLists.txt 1005628 
>   /trunk/KDE/kdepim/kmail/attachmentdialog.h PRE-CREATION 
>   /trunk/KDE/kdepim/kmail/attachmentdialog.cpp PRE-CREATION 
>   /trunk/KDE/kdepim/kmail/kmreaderwin.cpp 1005719 
> 
> Diff: http://reviewboard.kde.org/r/1219/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Martin
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/