KIO::Job, JobUiDelegate strangeness (was: Preparing terrain for kio_uiserver)

[Kevin Ottens]

Le Samedi 23 Décembre 2006 13:36, Rafael Fernández López a écrit :
> As usual I forgot to attach the patch. Here it is.

Yeah, as usual four mails before we can reply. :-)

>  void KJob::setUiDelegate( KJobUiDelegate *delegate )
>  {
> -    if ( delegate == 0 || delegate->setJob( this ) )
> +    if ( d->uiDelegate )
>      {
>          delete d->uiDelegate;
> +    }
> +
>          d->uiDelegate = delegate;
> +
> +    if ( d->uiDelegate )
> +    {
> +        d->uiDelegate->setJob( this );
>      }
>  }

I fail to see what this change mean... It's basically different code doing 
almost the same thing. Note that it screws up the indenting. Also note that 
it even introduces a bug since if setJob() failed you act as if it succeeded 
(the job considers this delegate attached while it's wrong).

That said, maybe it could be wise to change the behavior of this method. 
Currently if we do:
KJobUiDelegate *delegate = ...;
KJob *job1 = ...;
KJob *job2 = ...;

job1->setUiDelegate(delegate); // succeeds
job2->setUiDelegate(delegate); // silently fails (the delegate being already 
attached to job1)


I think we should keep the 1:1 multiplicities between jobs and delegates so I 
see two possible changes here:
1) Make setUiDelegate() return a bool indicating success or not (maybe need to 
rename the method then, something like attachUiDelegate()?). It keeps status 
quo, but at least we inform the caller about what's going on.
2) Make setUiDelegate() silently detach delegate from job1 during the second 
call. But I'm not fond of this solution, looks like a good way to shoot 
yourself in the foot.

Now going back to your issue... The problem is that the Observer is poked in 
the JobUiDelegate::connectJob() which is called indirectly (through 
KJobUiDelegate::setJob()) from the KJob::setUiDelegate() 
*before* "d->uiDelegate = delegate;" is done. I commited a fix for this a 
minute ago. Please update and report if it's enough to solve your issue.


> @@ -92,7 +90,7 @@ void KJobUiDelegate::connectJob( KJob *j
>      connect( d->job, SIGNAL( finished( KJob*, int ) ),
>               this, SLOT( slotFinished( KJob*, int ) ) );
>  
> -    connect( job, SIGNAL( warning( KJob*, const QString& ) ),
> +    connect( d->job, SIGNAL( warning( KJob*, const QString& ) ),
>               this, SLOT( slotWarning( KJob*, const QString& ) ) );

This change is wrong don't commit it. OTOH It would be right to 
change "d->job" by "job" on line 92. I also commited this a minute ago with 
the other fix.

Apart from those two bits, the other proposed changes look sane to me.

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