----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128412/#review97252 -----------------------------------------------------------
The port seems fine, you can commit it as is if you want, it's not worse than before. My suggestions would improve the code but are all about issues that were already there. src/manager/allyourbase.cpp (line 318) <https://git.reviewboard.kde.org/r/128412/#comment65657> Running an event loop in a dropEvent is a dangerous recipe. The dropevent data gets deleted by Qt at some point in that duration, this has bitten me in the past (for KIO::paste, where the event loop can of course last for much longer than here, since there we wait for the user to reply to a modal dialog). I suppose this is probably fine (apart from the mouseButtons() test happening after the event loop), but it would be much cleaner/safer to move the decodeFolder() code and following to a slot. src/manager/allyourbase.cpp (line 337) <https://git.reviewboard.kde.org/r/128412/#comment65658> I wonder why this doesn't use the proposedAction from the dropEvent instead of checking for ControlModifier directly (which is going to be wrong when there's an event loop in between)... src/manager/allyourbase.cpp (line 438) <https://git.reviewboard.kde.org/r/128412/#comment65659> Same issue here, exec() followed by mouseButtons(). At least the call to mouseButtons() could be done at the beginning of the method.... - David Faure On July 9, 2016, 7:14 p.m., Martin Tobias Holmedahl Sandsmark wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128412/ > ----------------------------------------------------------- > > (Updated July 9, 2016, 7:14 p.m.) > > > Review request for KDE Frameworks, KDE Utils, David Faure, and Valentin Rusu. > > > Repository: kwalletmanager > > > Description > ------- > > Port away from NetAccess. > > > Diffs > ----- > > src/manager/CMakeLists.txt 503298b > src/manager/allyourbase.cpp 05590f5 > src/manager/kwalleteditor.cpp 7c4229b > src/manager/kwalletmanagerwidget.cpp c318491 > > Diff: https://git.reviewboard.kde.org/r/128412/diff/ > > > Testing > ------- > > > Thanks, > > Martin Tobias Holmedahl Sandsmark > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
