Review Request: GSoC: Errors handling during file transfer.

David Faure faure at kde.org
Tue Aug 23 15:15:45 BST 2011 


> 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).
> 
> Cyril Oblikov wrote:
>     But it is very common in copyjob.cpp:
>     `grep "(time_t)" copyjob.cpp | wc -l` says 17.
>     Should I replace them all?

OK, old code shows bad example :-)
Leave it as is for now, and fix them all in a separate commit later.


> 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).
> 
> Cyril Oblikov wrote:
>     Hmm... what is file-static and function-static?

File-static is a C-style function in a .cpp file. Marked "static" so that it's only visible inside the .cpp file.

A function-static object is "static Foo bar;" inside of a function/method. "Safe" because it's only created when entering the function/method for the first time.

In clear:

static QStringList allInteractionItemHeaders() {
    static QStringList s_headers;
    if (s_headers.isEmpty()) {
        s_headers << i18n("...") << ...;
    }
    return s_headers;
}

In a CPP file, not in a header file :)


> 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?
> 
> Cyril Oblikov wrote:
>     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.

Well, a simple loop could be used to hide all buttons, but OK. I drop the charges :)


> 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 :)
> 
> Cyril Oblikov wrote:
>     I'm not sure this method should be virtual.

Actually it could be useful. E.g. for unittests, which could test that the interactiondialog is called, without actually showing a dialog.

If you had more time for your SOC I would strongly recommend to add unit tests as the next step, BTW...

But did you at least run jobtest (in kdelibs/kio/tests) to make sure that the new code doesn't break it? E.g. the creation of a dialog for sure broke it, but now you've fixed that bit.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102388/#review5920
-----------------------------------------------------------


On Aug. 22, 2011, 11:12 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, 11:12 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_p.h PRE-CREATION 
>   kio/kio/interactiondialog/abstractinteractionmodel.cpp PRE-CREATION 
>   kio/kio/interactiondialog/abstractinteractionmodel_p.h PRE-CREATION 
>   kio/kio/interactiondialog/allinteractionitem.cpp PRE-CREATION 
>   kio/kio/interactiondialog/allinteractionitem_p.h PRE-CREATION 
>   kio/kio/interactiondialog/allinteractionmodel.cpp PRE-CREATION 
>   kio/kio/interactiondialog/allinteractionmodel_p.h PRE-CREATION 
>   kio/kio/interactiondialog/existinginteractionitem.cpp PRE-CREATION 
>   kio/kio/interactiondialog/existinginteractionitem_p.h PRE-CREATION 
>   kio/kio/interactiondialog/existinginteractionmodel.cpp PRE-CREATION 
>   kio/kio/interactiondialog/existinginteractionmodel_p.h PRE-CREATION 
>   kio/kio/interactiondialog/interactiondialog.h PRE-CREATION 
>   kio/kio/interactiondialog/interactiondialog.cpp PRE-CREATION 
>   kio/kio/interactiondialog/interactiondialogtab.cpp PRE-CREATION 
>   kio/kio/interactiondialog/interactiondialogtab_p.h PRE-CREATION 
>   kio/kio/interactiondialog/renameinteractionwidget.h PRE-CREATION 
>   kio/kio/interactiondialog/renameinteractionwidget.cpp PRE-CREATION 
>   kio/kio/interactiondialog/requestitemmodel.cpp PRE-CREATION 
>   kio/kio/interactiondialog/requestitemmodel_p.h PRE-CREATION 
>   kio/kio/interactiondialog/unreadableinteractionitem.cpp PRE-CREATION 
>   kio/kio/interactiondialog/unreadableinteractionitem_p.h PRE-CREATION 
>   kio/kio/interactiondialog/unreadableinteractionmodel.cpp PRE-CREATION 
>   kio/kio/interactiondialog/unreadableinteractionmodel_p.h 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/20110823/7b099c45/attachment.htm>

More information about the kde-core-devel mailing list