[xdg-draft] JobViewServer

[Kevin Krammer]

On Thursday 21 February 2008, Kevin Ottens wrote:
> 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.

Right, good point!

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

The spec could leave it to the implementation how it handles a mismatch, i.e. 
a call to this method when the capability has not been set, but I think it is 
cleaner to specify the reaction to be a specific error.

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

Right, silently catching errors is evil ;-)

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

I agree that this will mostly be interesting for jobs which deal with data 
transfer.
The idea I had in mind was mainly a geek thing, e.g. when you automatically 
rename thousands of MP3 files based on their meta data, each file takes an 
equal amount of time to rename and it could display something like "renaming 
200 files per second" :)

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

Yes, it would mainly be interesting during development or a cross-test.
But since the intention is to have more than one implementation of either side 
this is important IMHO.

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

I don't think it is important either.
The use case I had in mind was to distinguish between making a label empty and 
hiding it. So removeDescription(uint32) might even be more precise.

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

Right, but they could, A map keeps this optional if the specification allows 
key/value pairs to be "missing".
And it keeps things extensible, e.g. a bit like KIO Job meta data.

But it is not so important either.

So I guess my comments boil down to: specify D-Bus errors to be thrown when 
really unexpected things happen, i.e. when it is likely that there is an 
error in the implementation.

Cheers,
Kevin

-- 
Kevin Krammer, KDE developer, xdg-utils developer
KDE user support, developer mentoring
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20080223/6f69dc4e/attachment.sig>