[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-kimageshop
Subject:    Re: Usage of setCurrentNodeLocked()
From:       Boudewijn Rempt <boud () valdyas ! org>
Date:       2010-09-21 6:43:11
Message-ID: 201009210843.11797.boud () valdyas ! org
[Download RAW message or body]

On Tuesday 21 September 2010, Dmitry Kazakov wrote:
> Hi, All!
> 
> I found that some of the calls to setCurrentNodeLocked() are done in a wrong
> way. I mean, at least in KisToolFill and KisToolGradient there is a
> possibility that the codepath will return from the event function and leave
> the node locked forever.
> 
> I do really think we should remove all these workaround "locks" and handle
> our tools in a proper way. Because these subtle locking bugs are really
> difficult to see and catch. I think it's better to delay the release if
> needed, than to release a product with random bugs.

Okay, let's go back a step or two and consider the requirements:

* if a user executes some long running action on a layer we want him to be able to \
                continue working on another image in another window
* if a user executes some long running action on a layer we want him to be able to \
                continue working on another layer in the same image
* long running actions should be cancelable.
* long running actions should show progress
* if an action is running on a layer, no other action should be executed on that \
layer

For now, I think the systemlocked mechanism is a working option, but as you say, we \
are doing it in the wrong way. We could have a KisNodeLocker object that's created at \
the start of the option on the stack and on deletion (when the action method ends) \
should unlock the node, instead of manually locking and unlocking. That is a \
relatively small change that will help a lot.

I know you would prefer a queuing mechanism, and I still sort of agree desktop Sven's \
objections, but it needs really careful designing since then every non-interactive \
action (like fill, filter, gradient, scale) needs to be encapsulated in a job and the \
job put on a queue.

And there would still be a clash with the paint tool, since I don't think that its \
actions should be mixed in the queue with the long running actions. And it might \
still be better usability to disallow starting a new gradient while the old one is \
running, for instance.

-- 
Boudewijn Rempt | http://www.valdyas.org
Ceterum censeo lapsum particulorum probae delendum esse
_______________________________________________
kimageshop mailing list
kimageshop@kde.org
https://mail.kde.org/mailman/listinfo/kimageshop


[prev in list] [next in list] [prev in thread] [next in thread]