QGIS » QGIS Application

Discarding features always implies data loss, doesn't it!?

Split a feature is composed of two phases in this order:
1) change geometry of the original geom
2) add new features => error

the rollback remove added feature but not reset old geom because it's change was successfull in the edit buffer => more than a data loss is a not reversible data change.

Luigi Pirelli wrote:

more than a data loss is a not reversible data change.

user-side is data loss.

Jürgen Fischer wrote:

Discarding features always implies data loss, doesn't it!?

is not a new added geometry that is discarded, but a chunk of an existing geometry (split with the split features tool) that is lost.

Giovanni Manghi wrote:

Jürgen Fischer wrote:

Discarding features always implies data loss, doesn't it!?

is not a new added geometry that is discarded, but a chunk of an existing geometry (split with the split features tool) that is lost.

It's lost because the user decided to discard it. The user should fix the feature by assigning a valid key value and proceed with the commit.

It's lost because the user decided to discarded it. The user should fix the feature by assigning a valid key value and proceed with the commit.

you have faith in users that they will click in the message bar and read the log ;) they usually don't, and just run away from the error as fast as they can (by undoing changes).

A fix for #17186 would make this issue much less important.

Giovanni Manghi wrote:

If this was already reported as #7550 and closed as fixed this means that is a regression (I "only" tested down to 2.8).

I don't consider it a bug. It's the expected behaviour: You have to specify a key value if you don't use a sequence. And there might by arbitrary other constraints that you have to adjust your attributes (or geometries) to to be able to commit.

Jürgen Fischer wrote:

Giovanni Manghi wrote:

If this was already reported as #7550 and closed as fixed this means that is a regression (I "only" tested down to 2.8).

I don't consider it a bug. It's the expected behaviour: You have to specify a key value if you don't use a sequence. And there might by arbitrary other constraints that you have to adjust your attributes (or geometries) to to be able to commit.

I think we agree we don't agree :)

my point of view is the following:

I do a whatever editing operation, then before any save I discard it. Is not correct that after discarding any unsaved change I have lost data.

Jürgen Fischer wrote:

Giovanni Manghi wrote:

If this was already reported as #7550 and closed as fixed this means that is a regression (I "only" tested down to 2.8).

I don't consider it a bug. It's the expected behaviour: You have to specify a key value if you don't use a sequence. And there might by arbitrary other constraints that you have to adjust your attributes (or geometries) to to be able to commit.

may we have another API option to setup if to use default nextval or leave it to the user ?

Luigi Pirelli wrote:

may we have another API option to setup if to use default nextval or leave it to the user ?

The point is that there is no nextval in this case (see #17186) - otherwise it would be used (merge already doesn't copy attributes with default values; see #5475)

Giovanni Manghi wrote:

my point of view is the following:

I do a whatever editing operation, then before any save I discard it. Is not correct that after discarding any unsaved change I have lost data.

If you would just "discard", it would work like that. But you "commit" - which only partly succeeds and then "discard" the parts that didn't succeed. You wouldn't loose data if you would just update the failed parts and commit them. I agree that partly commits are not ideal and that deducing what actually needs to be done to fix the remaining parts might not be easy. But we still handle adding and deleting, changing of attributes and geometries as separate operations in the provider interface (and hence they run in separate transactions and cannot rolled back cleanly; see QgsVectorLayer::commitChanges).

Jürgen Fischer wrote:

Giovanni Manghi wrote:

my point of view is the following:

I do a whatever editing operation, then before any save I discard it. Is not correct that after discarding any unsaved change I have lost data.

If you would just "discard", it would work like that. But you "commit" - which only partly succeeds and then "discard" the parts that didn't succeed. You wouldn't loose data if you would just update the failed parts and commit them. I agree that partly commits are not ideal and that deducing what actually needs to be done to fix the remaining parts might not be easy. But we still handle adding and deleting, changing of attributes and geometries as separate operations in the provider interface (and hence they run in separate transactions and cannot rolled back cleanly; see QgsVectorLayer::commitChanges).

ok, thanks I understand better the technicality of what is going on here. But I think that we all agree that from a user point of view this is an unexpected thing. Also is not what we tell people about how editing works (if I'm not wrong in docs we tell people that changes are saved only when hitting the "save" button, regardless of the tool they use).

Giovanni Manghi wrote:

(if I'm not wrong in docs we tell people that changes are saved only when hitting the "save" button, regardless of the tool they use).

as said that is what's happening. just that the save partly fails and the failing parts are kept and the successful parts can't be undone.

Jürgen Fischer wrote:

Giovanni Manghi wrote:

(if I'm not wrong in docs we tell people that changes are saved only when hitting the "save" button, regardless of the tool they use).

as said that is what's happening. just that the save partly fails and the failing parts are kept and the successful parts can't be undone.

so this would be won't fix because impossible to fix? If yes I would add a ticket to remember me to document this behavior.

Giovanni Manghi wrote:

so this would be won't fix because impossible to fix? If yes I would add a ticket to remember me to document this behavior.

Of course it's not impossible - but the current behavior is a result of the current provider design (see #17185-17 and #5475-17), so it wouldn't be a small change.

Of course it's not impossible - but the current behavior is a result of the current provider design (see #17185-17 and #5475-17), so it wouldn't be a small change.

We should leave this open then. I will add a note to docs to document this. As said, fixing #17186 would makes this slightly less relevant.