Nice job. Please find my comments below.

kio/kio/copyjob.h (Diff revision 2)

... yes? :)
 (end of sentence missing)

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   fixed

kio/kio/copyjob.cpp (Diff revision 2)

Store the UDSEntry here, instead?

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   It is no longer needed at all.

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)

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   fixed

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

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   But it is very common in copyjob.cpp:
   `grep "(time_t)" copyjob.cpp | wc -l` says 17.
   Should I replace them all?

2. 

   David Faure Aug. 23, 2011, 3:50 p.m.

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

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)

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   fixed

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.

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   fixed

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?

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   Yes. I've changed the order of ifs and added additional commets. I hope it is clear now.

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

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   fixed

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

1. 

   Cyril Oblikov Aug. 23, 2011, 12:39 a.m.

   Hmm... what is file-static and function-static?

2. 

   David Faure Aug. 23, 2011, 3:57 p.m.

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

3. 

   Cyril Oblikov Aug. 24, 2011, 6:21 p.m.

   done

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)

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   fixed

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.

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   fixed

kio/kio/interactiondialog/interactiondialog.h (Diff revision 2)

Typo (in all the signals) Emmited -> Emitted.

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   fixed

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

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   fixed

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?

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   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.

2. 

   David Faure Aug. 23, 2011, 3:58 p.m.

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

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

1. 

   Cyril Oblikov Aug. 23, 2011, 12:37 a.m.

   I'm not sure this method should be virtual.

2. 

   David Faure Aug. 23, 2011, 3:59 p.m.

   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.

3. 

   Cyril Oblikov Aug. 24, 2011, 6:23 p.m.

   No, I didn't use tests. Are there any docs about kdelibs testing?

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";
)

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.