[xdg-draft] JobViewServer

[Kevin Ottens]

Le Thursday 21 February 2008, Rafael Fernández López a écrit :
> > > - void terminate(string errorMessage)
> > > The job has finished (correctly or aborted). errorMessage will be a
> > > null string if the job did finish correctly, and will contain the
> > > string reason of why the job did finish unexpectedly if the job did not
> > > finish correctly.
> >
> > Based on the above assumption:
> > "terminate" sounds like a command, I'd rather call it "terminated".
> > Maybe even split in "finished" (success) and "terminated" (failure)

Well, it's a point of view really, it's also the job telling to the view to go 
away... I wrote this as a command in mind indeed. =)

In particular if you use "terminated" or something similar that wouldn't be 
coherent with the "set*" methods, which are a command.

> > > - void setSuspended(bool suspended)
> > > The job has been suspended or resumed. This method will only be called
> > > if the job had those capabilities (@see requestView() on the
> > > JobViewServer). suspended parameter will determine if the job is
> > > suspended or resumed, depending on its value, true or false,
> > > respectively.
> >
> > IMHO you should specify a D-Bus error which will be "thrown" if the
> > method is called on a view not created with the respective capability.

I think that's a good point to improve error reporting, that said I hardly see 
more than one error type here. It's supposed to fail only if the view was 
created without the support for suspend/resume.

> I don't think the D-Bus error is necessary, since we are talking on
> implementation details. The implementation should not let a job with no
> suspend capability to call this method. Is not about runtime problems that
> should emit a D-Bus error, but an implementation detail.

Well, the implementation of the server can't prevent the client to do bad 
things. Having better error reporting will help client developers, so I'd 
really be in favor of adding such D-Bus exception.

> > > - void setSpeed(uint64 bytesPerSecond)
> > > If applicable, sets the current speed of the job in bytes per second.
> > > Useful when the job is talking about I/O operations.
> >
> > Maybe also make this dependent on some unit?
> > Like "x files per second" e.g. for a tool which renames audio files based
> > their metadata.
>
> Could be. We can revisit this.

Well, AFAIK it's done this way because for speed only the "bytes per second" 
unit is worth it. "files per second" is barely usable since you can have a 1G 
file and then a 4K one...

> > > - bool setDescriptionField(uint32 number, string name, string value)
> > > Sets extra information about the job. number parameter sets the ID for
> > > the description field. name specifies what is the description field
> > > name, and value which value it has. If the description field has been
> > > correctly saved, then true is returned, false otherwise. Depending on
> > > the implementation, it can force number be as much 2, for instance, but
> > > we encourage to freely let the interface user set the number he/she
> > > wants. The
> >
> > If there is more than one reason why the call could fail, it should
> > rather "throw" D-Bus errors so the caller can handle them accordingly.
>
> No. The only reason this could fail is again because of the implementation.
> For instance the implementation X only wants two description fields (and in
> order starting from 0), and all numbers above 1 will be rejected. It will
> return true for 0 and 1, but false for numbers > 1.

Well, could be done with exceptions... That said it's more an "I ignore your 
request, just because", the server implementation has the last word on what 
it wants to support, it's about extended information here.

> > > If you want to remove a description field, you only will need to set
> > > name and value to null strings, and the server should remove the
> > > information.
> >
> > I'd rather suggest adding a clearDescriptionField(uint32) method.
>
> Given the specification, maybe clearDescriptionField is not necessary and
> makes our API bigger with no need. Anyway, I have no objections on adding
> it really.

Me neither, that could be added I think.

> > > > JobViewServer < (org.kde.JobViewServer) -
> > > > (org.freedesktop.JobViewServer)
> > >
> > > - objectPath requestView(string appName, string appIconName, uint32
> > > capabilities)
> > > The job requests to be shown with appName, for instance "Kopete",
> > > appIconName, for example "kopete" and with certain capabilities. This
> > > last parameter supports bit operations. Meaning:
> > >
> > > 0x0001 => user should be able to cancel the job
> > > 0x0002 => user should be able to suspend/resume the job
> >
> > What about a variant map which at least includes the capabilities but
> > which could already include things like total amount and descriptions,
> > saving setup roundtrips?
>
> I don't agree. I think this method should only register a new dbus object
> giving it a new path and returning it. I really think we should follow Qt 4
> mentality on do-not-overkill-constructors-and-let-setters-be.

Well, Kevin is also right that D-Bus calls have a price higher than methods 
calls. :-)

That said, it makes sense to separate the total amount, descriptions etc. 
Because most jobs don't know this information when they start anyway. So 
they'd have to make those calls in any case.

So like Rafael I disagree on this one.

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: signature.asc
Type: application/pgp-signature
Size: 194 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20080221/e1303487/attachment.sig>