Review Request 113516: Implement queueing directly in KDialogJobUiDelegate

[Alex Merry]

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

Ship it!


Generally looks good, but see the comments below.  Also, the apidocs should state that messageboxes for any given job will be queued up, but that queueing will not happen across jobs (or globally); I believe this is a change in behaviour from kdelibs4, where the queueing was application-global.


tier2/kjobwidgets/src/kdialogjobuidelegate.cpp
<http://git.reviewboard.kde.org/r/113516/#comment30981>

    Would it not be marginally more efficient (and still correct) to put this test at the end of the method, with the invokeMethod in an else clause?



tier2/kjobwidgets/src/kdialogjobuidelegate.cpp
<http://git.reviewboard.kde.org/r/113516/#comment30983>

    This will be application model, I believe.  Might be worth mentioning the class apidocs (and/or providing a way to make it window-modal instead).



tier2/kjobwidgets/src/kdialogjobuidelegate.cpp
<http://git.reviewboard.kde.org/r/113516/#comment30980>

    For the sake of being "obviously correct", this should probably be enqueue (since you use dequeue to remove).


- Alex Merry


On Oct. 31, 2013, 9:41 a.m., Àlex Fiestas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113516/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2013, 9:41 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> Implement queueing directly in KDialogJobUiDelegate by queueing calls to KDialogJobUiDelegate::Private::next which shows 1 KMessageBox at the time.
> 
> 
> Diffs
> -----
> 
>   tier2/kjobwidgets/src/kdialogjobuidelegate.cpp 29c2bae 
>   tier2/kjobwidgets/tests/kjobtrackerstest.cpp 7a61407 
> 
> Diff: http://git.reviewboard.kde.org/r/113516/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20131102/5ce8a89b/attachment.html>