Hi,<br><br><div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Yes, we probably want pause()+doPause() and resume()+doResume() just like we<br>

have kill()+doKill() right now.</blockquote><div><br>I will write them when have time, and as usual, asking before committing.<br></div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

Comments on the patch itself now:  - There's already a kjobtest in kdecore/tests so at first I thought about moving your there. That said, it's really testing observer+kuiserver not really kjob itself, so I'd vote for renaming it kioobservertest.

</blockquote><div><br>Renamed to kioobservertest. You meant this one or kjobobservertest ? I called it as you suggested, but maybe the second is better in this case.<br></div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

 - In the CMakeLists.txt I wouldn't put the add_test(...), there's no point in<br>running this one automatically since it's really not an automated test IMO<br>(meaning you have to be in front of the computer, you can't go away and get

<br>FAIL or PASS when you're back).</blockquote><div><br>Agree, changed. <br></div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> - Seeing Branan's comment about the other patch, maybe it's better to

<br>keep "Pausable" ane "Killable" since it's what about other stuff can do to<br>the job. We're still free to change it later if that proves to be a problem.</blockquote><div><br>Since I'm not native speaker, I believe what Branan pointed, so I've changed it :) 

<br></div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> - I see some code duplication at the end of Observer::newJob():<br>+    if (job->capabilities() & KJob::CanPause)

<br>+    {<br>+        m_uiserver->newAction(progressId, KJob::CanPause, i18n("Pause"));<br>+        newSlotCall.slotName = SLOT(jobPaused(KJob*,int));<br>+        m_hashActions[progressId].insert(KJob::CanPause, newSlotCall);

<br>+    }<br>+<br>+    if (job->capabilities() & KJob::CanKill)<br>+    {<br>+        m_uiserver->newAction(progressId, KJob::CanKill, i18n("Cancel"));<br>+        newSlotCall.slotName = SLOT(jobCanceled(KJob*,int));

<br>+        m_hashActions[progressId].insert(KJob::CanKill, newSlotCall);<br>     }<br><br>And actually, it made me wonder if it would be better to provide the full<br>capabilities flag to the m_uiserver->newJob() call. Then it would be the

<br>responsibility of the kuiserver to show the actions it wants to deal with<br>those capabilities. It would have more control for the layout etc. While with<br>a bunch of m_uiserver->newAction() calls apart from adding the buttons next

<br>to each other it can't really take decision on how to layout the item<br>buttons. For instance, it's probably better to have a button with an icon<br>next to the progress bar for the "cancel" action (like the ones with have in

<br>kmail).<br><br>But well, for now we probably want to keep m_uiserver->newAction() getting<br>ride of it in favor of providing the capabilities to the uiserver directly<br>could be done later in another refactoring. This last comment was more food

for thought. ;-)</blockquote><div> Well, the code duplication in that case will be moved to the kuiserver, since in some place flags needs to be checked, and because none of them (right now) are exclusive we have to check one by one, adding support.

What I find better with moving this logic to the kuiserver is that the addAction wouldn't fly through dbus, and wouldn't be in the interface, so external applications wouldn't be able to call this function from an extern place. So addAction could live in the kuiserver (private for example, but doesn't matter since it is a self-running app), and because of that, it is able to decide what to do. More for keeping safe all the actions stuff than drawing.

Indeed, drawing the button on the right of the progressbar is not trivial (right now), it can always be changed. Right now, the progressbar is drawn through style, not a "real" widget at all. But the buttons that are below, is an editor with some buttons. For that reason is hard to draw (right now) a button perfectly fitted with the progressbar and in another place that has nothing to do with the below bar of buttons. The easy solution is to make the editor bigger and to remove the progressbar painted with style, being replaced with a real QProgressBar widget in the delegate (inside the editor). I'm not very sure at this point if a click on that progress bar (the real widget) would make the delegate being selected, or the focus is going to the editor itself, instead of making the row selected (see that in buttons it doesn't matter, they're clicked and row is no selected, that is the right behavior).

Anyway as you say, this can be done in another refactoring, but I want to say I strongly support this decision for the reasons I explained before: keeping safety.<br clear="all"></div></div> --  Bye, Rafael Fernández López.