Well, this approach has two problems, one is fixable and the other not

1. Use barrierLock() instead of lock() to not break strokes that are running in the background.
2. Using QTimer for unlocking is *really* dangerous, because we can end up with deadlocks and/or unbalances locking. E.g. in this patch calling slotLowerClicked() with a delay less than 200ms will lead to the lock held twice and unlocked only once, which will make Krita freeze for any user actions.

There are several safe solutions to this problem:

1. Use KisProcessingsApplicator with 'node' set to null and handle updates manually with a scpecifically crafted update command that uses code from KisImageLayerAddCommand::redo/undo(). This will solve the problem of multiple updates while moving multiple layers. But it will *not* solve the problem of moving a single layer multiple times.

2. Create some class inside KisImage that is used by all node-related commands that accepts updates form them and compresses them using KisSignalCompressor or KisSignalCompressorWithParam. This will solve both of the problems of too-many updates.

This very approach with locking by QTimer is really unsafe :(

Well, I suspected that. What it tells me is that our architecture/api is dangerous, because both your solutions are way too complicated for what should be a really simple thing, delaying the recomposition of the image until the user stops doing things in the ui.

The first solution doesn't solve the problem, which is specifically, people trying to move a layer up several places.

The second solution is adds yet another special class, of which we already have way too many :-(

Hm, looking through our KisUpdateSchedular api, shouldn't

blockUpdates()

be exactly what we need here? barrierLock's apidox sound like it blocks processing, which isn't what we want, we only want to block updating the projection and resuming the projection after some time.

In D263#4706, @rempt wrote:

Well, I suspected that. What it tells me is that our architecture/api is dangerous, because both your solutions are way too complicated for what should be a really simple thing, delaying the recomposition of the image until the user stops doing things in the ui.

Well, any api that involves multithreading is dangerous. We have no way to delay updates only. If you lock() the image all the processing, including updates and strokes is stopped. If you unlock the image after you played with the layers and the background stroke continues, it will be a bit surprized with the new layout of layers. The result of such continuation is not defined, most probably will cause a crash. So before explicitly executing any command the image should be barrier-locked, so all the strokes are finished. This locking is usually performed by legacy KisUndoAdapter.

The second solution is adds yet another special class, of which we already have way too many :-(

Well, classes are not that bad, especially when you ask for a non-standard behaviour.

In D263#4708, @rempt wrote:

Hm, looking through our KisUpdateSchedular api, shouldn't

blockUpdates()

be exactly what we need here? barrierLock's apidox sound like it blocks processing, which isn't what we want, we only want to block updating the projection and resuming the projection after some time.

Anyway you need to wait until all the background strokes are finished before adding/removing/moving any layer, because they can still access it. And as far as I can tell, calling KisLegacyUndoAdapter::addCommand() after blockUpdates() will lead to a deadlock, because the latter one is supposed to be called from the stroke's context only. And the problem of unbalanced unlocking is also actual here.

It looks like it would be simpler to queue the layer movements and when none is forthcoming anymore, calculate the move, skipping all in-between moves...

In D263#4710, @rempt wrote:

It looks like it would be simpler to queue the layer movements and when none is forthcoming anymore, calculate the move, skipping all in-between moves...

Yes, it seems it is the best approach :)

Okay, and that idea won't work either because the contents of the layerbox are directly coupled to the model that reflects the layer order in the image. We really dropped the ball here, design-wise. It must be possible to rearrange the image while blocking the recalculation of the projection. All the other actions done to layer contents or whatever are irrelevant here. We must have a way to push all kinds of actions to the image without having to wait for the projection to be recalculated, just like we do on loading images. And there we use lock(), don't we?