----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102388/#review5920 -----------------------------------------------------------
Nice job. Please find my comments below. kio/kio/copyjob.h <http://git.reviewboard.kde.org/r/102388/#comment5199> ... yes? :) (end of sentence missing) kio/kio/copyjob.cpp <http://git.reviewboard.kde.org/r/102388/#comment5202> Store the UDSEntry here, instead? kio/kio/copyjob.cpp <http://git.reviewboard.kde.org/r/102388/#comment5201> deleted where? Also: The dialog should only be created if we're in "interactive" mode. The problem is, the way to disable interactive mode is job->setUiDelegate(0) after all this code has run. So I think the creation of the interaction dialog should be done on demand, when the first thing happens that might need it. At that point, we'll know if the job is interactive (has a ui delegate) or not (no ui delegate -> no way to handle the error, so abort the job, like the code already does in non-interactive mode) kio/kio/copyjob.cpp <http://git.reviewboard.kde.org/r/102388/#comment5203> No C-style casts please. Use static_cast<time_t>(foo) or constructor-syntax like time_t(foo). kio/kio/copyjob.cpp <http://git.reviewboard.kde.org/r/102388/#comment5204> An alternative to the signal mapper would be to just set the interactionId into the job, using a dynamic property (QObject::setProperty). That would be simpler, no? (maybe I'm missing something) kio/kio/copyjob.cpp <http://git.reviewboard.kde.org/r/102388/#comment5206> It might be more readable to ensure that this case is the last one in the method, since it will be executed last, chronologically. kio/kio/copyjob.cpp <http://git.reviewboard.kde.org/r/102388/#comment5205> And what happens if this if() is false, too? Ah then the job is waiting for the dialog? kio/kio/interactiondialog/abstractinteractionitem.h <http://git.reviewboard.kde.org/r/102388/#comment5208> It would be good to name _p.h all private headers in libraries (we don't do this everywhere, but at least we're moving into that direction). kio/kio/interactiondialog/allinteractionitem.h <http://git.reviewboard.kde.org/r/102388/#comment5209> You can't use i18n in a header file, nor in a static object. Instead, fill the list on demand when needed for the first time (if empty then append...). It's either that or I18N_NOOP, but in this case I think on demand is simpler. No static objects in libraries, too, so this should be a function-static (e.g. make a file-static function that has the function-static object, fills it on demand, and returns it). kio/kio/interactiondialog/existinginteractionitem.h <http://git.reviewboard.kde.org/r/102388/#comment5211> This method should made non-inline (to remove the number of unnecessary #includes in this header file, too) kio/kio/interactiondialog/existinginteractionitem.h <http://git.reviewboard.kde.org/r/102388/#comment5210> Same as above. kio/kio/interactiondialog/existinginteractionmodel.cpp <http://git.reviewboard.kde.org/r/102388/#comment5212> No parent object? (No layout, either?) I suppose this is done later on, but this makes the code surprising to read. kio/kio/interactiondialog/interactiondialog.h <http://git.reviewboard.kde.org/r/102388/#comment5213> Typo (in all the signals) Emmited -> Emitted. kio/kio/interactiondialog/interactiondialog.cpp <http://git.reviewboard.kde.org/r/102388/#comment5215> Seems to be a child of this, so I recommend passing "this" as parent widget argument. (I know, the Qt examples don't do that, but there are plenty of good reasons for doing it). kio/kio/interactiondialog/interactiondialogtab.cpp <http://git.reviewboard.kde.org/r/102388/#comment5214> This looks convoluted. In fact I'm not sure what this code does; it de-layouts without deleting the widgets? Then it would be simpler to just delete (and recreate) m_buttonsLayout, no? kio/kio/jobuidelegate.h <http://git.reviewboard.kde.org/r/102388/#comment5207> This is a BIC change (new virtual method in a public class). However, this patch is for kdelibs-frameworks (future 5.0), so in fact it's the right timing for making such a change. So, no objection, I just wanted to point this out for clarity :) - David On Aug. 22, 2011, 1:17 p.m., Cyril Oblikov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102388/ > ----------------------------------------------------------- > > (Updated Aug. 22, 2011, 1:17 p.m.) > > > Review request for kdelibs and David Faure. > > > Summary > ------- > > Modeless dialog to handle interactions and modifications in CopyJob. > > > Diffs > ----- > > kio/CMakeLists.txt b517621 > kio/kio/copyjob.h eb88c7a > kio/kio/copyjob.cpp eff7825 > kio/kio/interactiondialog/abstractinteractionitem.h PRE-CREATION > kio/kio/interactiondialog/abstractinteractionmodel.h PRE-CREATION > kio/kio/interactiondialog/abstractinteractionmodel.cpp PRE-CREATION > kio/kio/interactiondialog/allinteractionitem.h PRE-CREATION > kio/kio/interactiondialog/allinteractionmodel.h PRE-CREATION > kio/kio/interactiondialog/allinteractionmodel.cpp PRE-CREATION > kio/kio/interactiondialog/existinginteractionitem.h PRE-CREATION > kio/kio/interactiondialog/existinginteractionmodel.h PRE-CREATION > kio/kio/interactiondialog/existinginteractionmodel.cpp PRE-CREATION > kio/kio/interactiondialog/interactiondialog.h PRE-CREATION > kio/kio/interactiondialog/interactiondialog.cpp PRE-CREATION > kio/kio/interactiondialog/interactiondialogtab.h PRE-CREATION > kio/kio/interactiondialog/interactiondialogtab.cpp PRE-CREATION > kio/kio/interactiondialog/renameinteractionwidget.h PRE-CREATION > kio/kio/interactiondialog/renameinteractionwidget.cpp PRE-CREATION > kio/kio/interactiondialog/requestitemmodel.h PRE-CREATION > kio/kio/interactiondialog/requestitemmodel.cpp PRE-CREATION > kio/kio/interactiondialog/unreadableinteractionitem.h PRE-CREATION > kio/kio/interactiondialog/unreadableinteractionmodel.h PRE-CREATION > kio/kio/interactiondialog/unreadableinteractionmodel.cpp PRE-CREATION > kio/kio/jobuidelegate.h 25e0728 > kio/kio/jobuidelegate.cpp 85679c2 > > Diff: http://git.reviewboard.kde.org/r/102388/diff > > > Testing > ------- > > > Thanks, > > Cyril > >
