Review request changed
Change Summary:
Two little fixes: 1) string represenation of file permissions in ExistingInteractionItem 2) correct representation of date UnreadableInteractionItem
Diff: |
Revision 2 (+2429 -144) |
---|
Nice job. Please find my comments below.
-
kio/kio/copyjob.h (Diff revision 2) -
... yes? :) (end of sentence missing)
-
kio/kio/copyjob.cpp (Diff revision 2) -
Store the UDSEntry here, instead?
-
kio/kio/copyjob.cpp (Diff revision 2) -
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 (Diff revision 2) -
No C-style casts please. Use static_cast<time_t>(foo) or constructor-syntax like time_t(foo).
-
kio/kio/copyjob.cpp (Diff revision 2) -
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 (Diff revision 2) -
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 (Diff revision 2) -
And what happens if this if() is false, too? Ah then the job is waiting for the dialog?
-
kio/kio/interactiondialog/abstractinteractionitem.h (Diff revision 2) -
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 (Diff revision 2) -
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 (Diff revision 2) -
This method should made non-inline (to remove the number of unnecessary #includes in this header file, too)
-
kio/kio/interactiondialog/existinginteractionitem.h (Diff revision 2) -
Same as above.
-
kio/kio/interactiondialog/existinginteractionmodel.cpp (Diff revision 2) -
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 (Diff revision 2) -
Typo (in all the signals) Emmited -> Emitted.
-
kio/kio/interactiondialog/interactiondialog.cpp (Diff revision 2) -
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 (Diff revision 2) -
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 (Diff revision 2) -
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 :)
-
kio/kio/copyjob.cpp (Diff revision 3) -
Remove, unused now.
-
kio/kio/copyjob.cpp (Diff revision 3) -
Maybe the property name should be less generic, to avoid possible clashes (e.g. with Qt or KJob future changes). I would suggest "interactionId" (make sure to change it everywhere ... which is a good reason for using a static const char s_interactionIdProperty[] = "interactionId"; )
Review request changed
I did not review the code, so I cannot comment on it. What I am concerned about is how/when this code should be committed. I certainly believe this is 4.8 material, so it should not be committed to kdelibs/4.7, on the other hand, we have no master repo now, so developers/distributions wanting to test this would have to apply it manually, which is unfortunate for such a big feature.