Review Request 110995: move jobs from kdeui to kjobswidgets framework

[Kevin Ottens]

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


OK, managed to make a first pass of review nonetheless. For future reference, please make two commits (and then reviews) in situations like this: one for the dependencies related adjustments, one for the actual move. It's easier to review, and it'll be easier for bug hunting and history management later on.


staging/kjobwidgets/src/kjobtrackerformatters_p.h
<http://git.reviewboard.kde.org/r/110995/#comment25229>

    You removed an empty line just after that one. It was a useless change, please reintroduce that empty line.



staging/kjobwidgets/src/kstatusbarjobtracker.cpp
<http://git.reviewboard.kde.org/r/110995/#comment25230>

    Please use just tr() here. Will require using Q_DECLARE_TR_FUNCTIONS in KStatusJobTracker::Private declaration though.



staging/kjobwidgets/src/kstatusbarjobtracker.cpp
<http://git.reviewboard.kde.org/r/110995/#comment25231>

    ditto



staging/kjobwidgets/src/kwidgetjobtracker.cpp
<http://git.reviewboard.kde.org/r/110995/#comment25234>

    I think you should avoid using a variable for the context. Pass this one directly and get rid of trContext (otherwise the context won't get extracted properly IIRC).



staging/kjobwidgets/src/kwidgetjobtracker.cpp
<http://git.reviewboard.kde.org/r/110995/#comment25232>

    Please use just tr() here. Will require using Q_DECLARE_TR_FUNCTIONS in KWidgetJobTracker::Private declaration though.



staging/kjobwidgets/src/kwidgetjobtracker.cpp
<http://git.reviewboard.kde.org/r/110995/#comment25233>

    ditto



staging/kjobwidgets/src/kwidgetjobtracker.cpp
<http://git.reviewboard.kde.org/r/110995/#comment25237>

    You probably could even get rid of KGuiItem here.



staging/kjobwidgets/src/kwidgetjobtracker.cpp
<http://git.reviewboard.kde.org/r/110995/#comment25235>

    Get rid of those comments



staging/kjobwidgets/src/kwidgetjobtracker.cpp
<http://git.reviewboard.kde.org/r/110995/#comment25236>

    Don't call pixmap(32) on the icon. setWindowIcon takes a QIcon as parameter, not a QPixmap.



staging/kjobwidgets/src/kwidgetjobtracker.cpp
<http://git.reviewboard.kde.org/r/110995/#comment25238>

    You probably could even get rid of KGuiItem here.



staging/kjobwidgets/tests/kjobtrackerstest.cpp
<http://git.reviewboard.kde.org/r/110995/#comment25239>

    You inserted plenty of trailing whitespaces in that file, please fix them.


- Kevin Ottens


On June 13, 2013, 10:29 p.m., Wojciech Kapuscinski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110995/
> -----------------------------------------------------------
> 
> (Updated June 13, 2013, 10:29 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Kevin Ottens.
> 
> 
> Description
> -------
> 
> move jobs from kdeui to kjobswidgets framework
> 
> Port away from:
> NET::timestampCompare
> KWindowSystem::setIcon (QWidget.setWindowIcon)
> ki18n (not sure if  KWidgetJobTracker::Private::ProgressWidget::description() is correctly ported)
> KStandardGuiItem
> 
> I'm not sure about plural translations (appears (s) because there is not English translation)
> 
> 
> Diffs
> -----
> 
>   kdeui/CMakeLists.txt 46196b4 
>   kdeui/jobs/kabstractwidgetjobtracker.h 892e787 
>   kdeui/jobs/kabstractwidgetjobtracker.cpp 0b9e829 
>   kdeui/jobs/kabstractwidgetjobtracker_p.h 5f427e3 
>   kdeui/jobs/kdialogjobuidelegate.h 186f160 
>   kdeui/jobs/kdialogjobuidelegate.cpp 5afe6ec 
>   kdeui/jobs/kjobtrackerformatters.cpp 95e8cfb 
>   kdeui/jobs/kjobtrackerformatters_p.h aeeb856 
>   kdeui/jobs/kstatusbarjobtracker.h dbab452 
>   kdeui/jobs/kstatusbarjobtracker.cpp 688c942 
>   kdeui/jobs/kstatusbarjobtracker_p.h 9d93e9c 
>   kdeui/jobs/kwidgetjobtracker.h 761267f 
>   kdeui/jobs/kwidgetjobtracker.cpp 835cad0 
>   kdeui/jobs/kwidgetjobtracker_p.h 2a3f297 
>   kdeui/tests/CMakeLists.txt 8746397 
>   kdeui/tests/kjobtrackerstest.h c6f3195 
>   kdeui/tests/kjobtrackerstest.cpp bbde166 
>   staging/kjobwidgets/src/CMakeLists.txt 84756ce 
>   staging/kjobwidgets/src/config-kjobwidgets.h.cmake PRE-CREATION 
>   staging/kjobwidgets/src/kabstractwidgetjobtracker.h PRE-CREATION 
>   staging/kjobwidgets/src/kabstractwidgetjobtracker.cpp PRE-CREATION 
>   staging/kjobwidgets/src/kabstractwidgetjobtracker_p.h PRE-CREATION 
>   staging/kjobwidgets/src/kdialogjobuidelegate.h PRE-CREATION 
>   staging/kjobwidgets/src/kdialogjobuidelegate.cpp PRE-CREATION 
>   staging/kjobwidgets/src/kjobtrackerformatters.cpp PRE-CREATION 
>   staging/kjobwidgets/src/kjobtrackerformatters_p.h PRE-CREATION 
>   staging/kjobwidgets/src/kstatusbarjobtracker.h PRE-CREATION 
>   staging/kjobwidgets/src/kstatusbarjobtracker.cpp PRE-CREATION 
>   staging/kjobwidgets/src/kstatusbarjobtracker_p.h PRE-CREATION 
>   staging/kjobwidgets/src/kuiserverjobtracker.cpp 13de2f4 
>   staging/kjobwidgets/src/kwidgetjobtracker.h PRE-CREATION 
>   staging/kjobwidgets/src/kwidgetjobtracker.cpp PRE-CREATION 
>   staging/kjobwidgets/src/kwidgetjobtracker_p.h PRE-CREATION 
>   staging/kjobwidgets/tests/CMakeLists.txt d41be07 
>   staging/kjobwidgets/tests/kjobtrackerstest.h PRE-CREATION 
>   staging/kjobwidgets/tests/kjobtrackerstest.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/110995/diff/
> 
> 
> Testing
> -------
> 
> it builds and my tests shows now regressions (except plural translations (s) )
> 
> 
> Thanks,
> 
> Wojciech Kapuscinski
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130614/e55dbc33/attachment-0001.html>