KJob test. Going forward

[Kevin Ottens]

Le Vendredi 2 Février 2007 02:59, Rafael Fernández López a écrit :
> Hi folks,

Hello,

> I've done a small effort writing a test for the KJob class. I pretend to
> take a look of what needs we may have in actual coding, not developing on
> air. This is the full patch that I have ready for kdelibs, including strong
> changes on the observer / kuiserver architecture.
>
> We have the virtual method doKill() at kjob.h. I wonder if it would be nice
> to have another one, like doPause() and doResume() or something.

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

> Since the 
> observer is the class that has to call those methods (because slots for
> that are there, look at the code), and on those slots it receives a KJob*,
> we may need some methods like those on KJob class, so they can be called on
> the observer class.
>
> Waiting for your comments.

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.

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

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


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

Regards.
-- 
Kévin 'ervin' Ottens, http://ervin.ipsquad.net
"Ni le maître sans disciple, Ni le disciple sans maître,
Ne font reculer l'ignorance."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070202/0b1ba277/attachment.sig>