KJob test. Going forward

[Rafael Fernández López]

Hi,

Yes, we probably want pause()+doPause() and resume()+doResume() just like we
> have kill()+doKill() right now.


I will write them when have time, and as usual, asking before committing.

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.


Renamed to kioobservertest. You meant this one or kjobobservertest ? I
called it as you suggested, but maybe the second is better in this case.

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


Agree, changed.

- Seeing Branan's comment about the other patch, maybe it's better to
> keep "Pausable" ane "Killable" since it's what about other stuff can do to
> the job. We're still free to change it later if that proves to be a
> problem.


Since I'm not native speaker, I believe what Branan pointed, so I've changed
it :)

- I see some code duplication at the end of Observer::newJob():
> +    if (job->capabilities() & KJob::CanPause)
> +    {
> +        m_uiserver->newAction(progressId, KJob::CanPause, i18n("Pause"));
> +        newSlotCall.slotName = SLOT(jobPaused(KJob*,int));
> +        m_hashActions[progressId].insert(KJob::CanPause, newSlotCall);
> +    }
> +
> +    if (job->capabilities() & KJob::CanKill)
> +    {
> +        m_uiserver->newAction(progressId, KJob::CanKill, i18n("Cancel"));
> +        newSlotCall.slotName = SLOT(jobCanceled(KJob*,int));
> +        m_hashActions[progressId].insert(KJob::CanKill, newSlotCall);
>      }
>
> And actually, it made me wonder if it would be better to provide the full
> capabilities flag to the m_uiserver->newJob() call. Then it would be the
> responsibility of the kuiserver to show the actions it wants to deal with
> those capabilities. It would have more control for the layout etc. While
> with
> a bunch of m_uiserver->newAction() calls apart from adding the buttons
> next
> to each other it can't really take decision on how to layout the item
> buttons. For instance, it's probably better to have a button with an icon
> next to the progress bar for the "cancel" action (like the ones with have
> in
> kmail).
>
> But well, for now we probably want to keep m_uiserver->newAction() getting
> ride of it in favor of providing the capabilities to the uiserver directly
> could be done later in another refactoring. This last comment was more
> food
> for thought. ;-)


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.

-- 
Bye,
Rafael Fernández López.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070203/3f12b730/attachment.htm>