[Kde-pim] [patch] Printing emails [version 2]

[Jaroslaw Staniek]

Ingo Klöcker said the following, On 2008-07-11 21:37:
> On Friday 11 July 2008, Jaroslaw Staniek wrote:
>> Jaroslaw Staniek said the following, On 2008-07-11 10:39:
>>> Ingo Klöcker said the following, On 2008-07-11 01:04:
>>>> On Wednesday 09 July 2008, Jaroslaw Staniek wrote:
>>>>> Thomas McGuire said the following, On 2008-07-08 18:43:
>>>>>> Please go for a signal/slot solution.
>>>>> Attached for review.
>>>>>
>>>>> - added KHtmlPartHtmlWriter::finished() signal, connected to
>>>>> KMReaderWin::slotPrintMsg().
>>>>> - previous KMReaderWin::printMsg() code splitted into two pieces:
>>>>>    1st - KMReaderWin::printMsg( KMMessage* aMsg ) - connects the
>>>>> finished() signal and calls setMsg(),
>>>>>    2nd - KMReaderWin::slotPrintMsg() - is a response for
>>>>> finished() signal, calls mViewer->view()->print() and
>>>>> deletesLater() the KMReaderWin.
>>>>>
>>>>> - at KMPrintCommand level, we keep QPointer<KMReaderWin>
>>>>> s_printerWin globally until the KMReaderWin object is destroyed;
>>>>> - on KMPrintCommand::execute() we call
>>>>> s_printerWin->htmlWriter()->reset() to stop any previous
>>>>> processing
>>>> Using a static object is a no-go.
>>>>
>>>> +  if ( s_printerWin && s_printerWin->htmlWriter() )
>>>> +    s_printerWin->htmlWriter()->reset(); // stop any previous
>>>> processing
>>>>
>>>> This will cause problems if the user manages to print twice in
>>>> rapid succession because the second print job will kill the first
>>>> one.
>>> KMReaderWin::printMsg() is async now, so if we keep using
>>> KMReaderWin on the stack, it'll be destroyed just a (see the patch
>>> in my previous post).
>> "just after the printMsg() call" I mean
> 
> Exactly. That's why in the part of my message you have snipped away I 
> wrote:
> 
>>>> In fact, I do not see why the static variable is necessary at
>>>> all. It is only used in a single method so keeping the pointer to
>>>> the KMReaderWin in a variable appears to be superfluous. You can
>>>> simply create several KMReaderWin on the heap without keeping the
>                                         ===========
>>>> pointer. The KMReaderWin's will destroy themselves at the end of
>>>> slotPrintMsg().
> 
> I do not see why you try to prevent several instances of KMReaderWin 
> from being created on the heap by storing the pointer to a KMReaderWin 
> used for printing in a static variable. I do not see any problems with 
> creating several instances of KMReaderWin used for printing on the 
> heap. Maybe I am missing something, but I doubt it. The user can create 
> several separate reader windows, so why should it not be possible to 
> create several KMReaderWin's used for printing?
> 
> What I propose is:
>  KMCommand::Result KMPrintCommand::execute()
>  {
> -  KMReaderWin printWin( 0, 0, 0 );
> -  printWin.setPrinting( true );
> [...]
> +  KMReaderWin *printWin = new KMReaderWin( kmkernel->mainWin(), 0, 0 );
> +  printWin->setPrinting( true );
> [...]
> 
> i.e. basically the same as what you proposed except that I do not use a 
> static variable for keeping the KMReaderWin pointer around because it 
> is not necessary to keep the pointer in a variable.

Ingo
OK, thanks for the explanation of basically the same idea :)
Please see the attached patch for review.

-- 
regards / pozdrawiam, Jaroslaw Staniek
  Sponsored by OpenOffice Polska (http://www.openoffice.com.pl/en) to work on
  Kexi & KOffice (http://www.kexi.pl/en, http://www.koffice.org/kexi)
  KDE Libraries for MS Windows (http://windows.kde.org)
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: printing_fix.patch
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20080712/2e8e20b9/attachment.ksh>
-------------- next part --------------
_______________________________________________
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/