Review Request: GSoC: Errors handling during file transfer.

Cyril Oblikov munknex at gmail.com
Tue Aug 23 00:07:08 BST 2011 


> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20110822/101d6e4c/attachment.htm>

More information about the kde-core-devel mailing list