> On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/copyjob.h, line 263 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32527#file32527line263> > > > > ... yes? :) > > (end of sentence missing)
fixed > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/copyjob.cpp, line 169 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32528#file32528line169> > > > > Store the UDSEntry here, instead? It is no longer needed at all. > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/copyjob.cpp, line 270 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32528#file32528line270> > > > > 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) fixed > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/copyjob.cpp, line 624 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32528#file32528line624> > > > > No C-style casts please. Use static_cast<time_t>(foo) or > > constructor-syntax like time_t(foo). But it is very common in copyjob.cpp: `grep "(time_t)" copyjob.cpp | wc -l` says 17. Should I replace them all? > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/copyjob.cpp, line 1317 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32528#file32528line1317> > > > > 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) fixed > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/copyjob.cpp, line 1803 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32528#file32528line1803> > > > > It might be more readable to ensure that this case is the last one in > > the method, since it will be executed last, chronologically. fixed > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/copyjob.cpp, line 1813 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32528#file32528line1813> > > > > And what happens if this if() is false, too? Ah then the job is waiting > > for the dialog? Yes. I've changed the order of ifs and added additional commets. I hope it is clear now. > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/interactiondialog/abstractinteractionitem.h, line 20 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32529#file32529line20> > > > > 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). fixed > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/interactiondialog/existinginteractionitem.h, line 42 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32535#file32535line42> > > > > This method should made non-inline (to remove the number of unnecessary > > #includes in this header file, too) fixed > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/interactiondialog/existinginteractionmodel.cpp, line 81 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32537#file32537line81> > > > > No parent object? > > > > (No layout, either?) > > > > I suppose this is done later on, but this makes the code surprising to > > read. fixed > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/interactiondialog/interactiondialog.h, line 45 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32538#file32538line45> > > > > Typo (in all the signals) Emmited -> Emitted. fixed > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/interactiondialog/interactiondialog.cpp, line 51 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32539#file32539line51> > > > > 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). fixed > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/interactiondialog/interactiondialogtab.cpp, line 73 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32541#file32541line73> > > > > 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? This is an example from qt doc. It removes all items from a layout. Anyway we need to iterate over all layout items, because it is required to hide old buttons. So I guess deleting and recreating layout would not reduce the code. > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/jobuidelegate.h, line 107 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32549#file32549line107> > > > > 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 :) I'm not sure this method should be virtual. > On Aug. 22, 2011, 4:06 p.m., David Faure wrote: > > kio/kio/interactiondialog/allinteractionitem.h, line 47 > > <http://git.reviewboard.kde.org/r/102388/diff/2/?file=32532#file32532line47> > > > > 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). Hmm... what is file-static and function-static? - Cyril ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102388/#review5920 ----------------------------------------------------------- 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 > >
