----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115710/#review51125 -----------------------------------------------------------
Ship it! "Tests pass" == unittests? Did you also try playing with it interactively a bit? I see that both kcomboboxtest and kcompletionuitest instantiate a KHistoryComboBox. Sorry, it's just hard to review such patches for correctness, so a bit of testing wouldn't hurt. But after that (and the two minor issues below), yep, ship it. autotests/kcombobox_unittest.cpp <https://git.reviewboard.kde.org/r/115710/#comment35844> I would add a comment like "TODO: finish", because this is obviously a very limited test ("does not crash", basically). Yes, it's what I asked for, but anyone reading this in the future might wonder what the heck this is really testing :-) src/khistorycombobox.cpp <https://git.reviewboard.kde.org/r/115710/#comment35845> q->connect is a strange way to call a static method. QObject::connect would be usual (if a bit more verbose). - David Faure On Feb. 25, 2014, 10:50 p.m., David Gil Oliva wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/115710/ > ----------------------------------------------------------- > > (Updated Feb. 25, 2014, 10:50 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kcompletion > > > Description > ------- > > Hide private methods and slots behind the d-pointer in KHistoryComboBox > > Also: > -Remove header not used > > > Diffs > ----- > > autotests/kcombobox_unittest.cpp 49ef721 > src/khistorycombobox.h 3667eb4 > src/khistorycombobox.cpp 6f81dda > > Diff: https://git.reviewboard.kde.org/r/115710/diff/ > > > Testing > ------- > > It builds. Tests pass. > > > Thanks, > > David Gil Oliva > >
_______________________________________________ Kde-frameworks-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
