----------------------------------------------------------- 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
