Slowness, no undoability and exception on cancellation for "fix untagged unconnected nodes"

[bilbo]

Load attached file (manufactured test case) and run validator. You will get about 4500 untagged and unconnected nodes. Press "fix" to start fixing them. You'll see progressbar (at my machine going at about 100 nodes deleted per second)

I think 100 nodes/sec are quite slow (45 secs to delete them all), because if I just select all the 4500 nodes and press "d" they disappear in just about 2 seconds.

But what is worse: If I press cancel in middle of deleting the nodes, then some nodes will be deleted, but no "undo" item will be added to the command stack (not good, there should be undoable item with part of the nodes that were deleted) and pressing "Fix" again will cause exception in the plugin (because it is trying to delete already deleted nodes)

If I let the validator finish, it will create about 4500 sequences, each one deleting single node. I think it would be better to create one sequence and put all deleted nodes in it. This way it is impossible to simply undo the fixing, as you'll have to press "undo" 4500 times...

[bilbo]

Test case

[simon04]

Patch attached:

• ignores nodes outside downloaded area
• For each fixable errors (no key, only created_by/watch/source), put all corresponding in a single TestError. This enables fast deletion and only 1 entry in command td (easy undoing).

Any comments?

[stoecker]

No, I d don't thing grouping them is a good idea. The delay is probably caused by permanent updates. Very likely turning off signaling before fixing and enabling after fixing will solve this issue without this grouping.

[simon04]

Replying to stoecker:

Very likely turning off signaling before fixing and enabling after fixing will solve this issue without this grouping.

This wouldn't solve the undo-ability, would it?

[stoecker]

Replying to simon04:

This wouldn't solve the undo-ability, would it?

No. That must be a bug. It should not be possible to have modified data without a Command entry. Maybe validator uses some very old stuff instead of proper calls to DeleteAction? Validator exists for a long time, so it may be there are code parts which no longer match the current design, but still work most of the time.

[simon04]

Replying to stoecker:

Replying to simon04:

This wouldn't solve the undo-ability, would it?

No. That must be a bug. It should not be possible to have modified data without a Command entry.

One command entry is added for each node deleted. When deleting 6k nodes, 6k commands are added which is practically undo-able.

[stoecker]

This could be grouped in a SequenceCommand "Automatic Validation Fixes" or something like that. So it would be only on entry with 6000 sub-entries.

[simon04]

This could solve the problem, but would be more difficult to implement as currently fixError(TestError) is called for each error found. This method can do whatever it wants to do (i.e., does not necessarily need to add a command).

Why don't you like the grouping of unconnected nodes? IMHO one would mostly like to automatically fix these errors.

[stoecker]

When they are grouped I gets much harder to handle each node individual.

Probably it would anyway be a better idea if fixError always returns a Command? This could solve a number of other possible errors as well.

BTW: When each call adds an individual Command directly to the stack, then Undo should be possible and I don't understand the original description.

[simon04]

When they are grouped I gets much harder to handle each node individual.

Is an individual handling required at all?

BTW: When each call adds an individual Command directly to the stack, then Undo should be possible and I don't understand the original description.

It is possible, but impracticable when fixing a huge amount of unconnected nodes (e.g., undoing 6000 times).

[stoecker]

Replying to simon04:

Is an individual handling required at all?

Unless you exactly know what you're doing it is recommended always.

[simon04]

A different approach is needed …