Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"fill ring" do not trigger the correct computation of the area in the "default value" expression #32377

Closed
gioman opened this issue Oct 24, 2019 · 9 comments
Assignees
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Crash/Data Corruption Editing High Priority

Comments

@gioman
Copy link
Contributor

gioman commented Oct 24, 2019

pick a polygon layer (gpkg in my case).

add a default for a numeric field > $area

check "also apply default value on update"

create a polygon > area is computed

add a ring with the "add ring" tool: the outer polygon area is updated, the inner one is WRONG (it gets the are of the outer polygon)

select the outer polygon and the inner one, then do "merge selected features" > area of new polygon is WRONG as is not re-computed

QGIS 3.4.12

tagging as high priority as this leads to very dangerous errors in the data.

@gioman gioman added Editing High Priority Bug Either a bug report, or a bug fix. Let's hope for the latter! Crash/Data Corruption labels Oct 24, 2019
@gioman
Copy link
Contributor Author

gioman commented Oct 24, 2019

Who has developed the defaults could consider doing an urgent fix? this bad... bad, bad.

@gioman
Copy link
Contributor Author

gioman commented Oct 24, 2019

I tried master and there the "fill ring" tool crashes QGIS.

Anyway this bug report is also about lines:

on master when splitting a line it seems that the $length default is applied correctly to the two resulting lines BUT merging the resulting lines does not trigger the default expression to recompute the length.

@haubourg
Copy link
Member

I think we have several issues here.

The computation issue of only the bigger part of a multi polygon also happens when computing a field. This is unrelated to default value triggering.

In 3.8.2, default calculations are triggered correctly for me when adding parts.

@haubourg
Copy link
Member

Mmm, just rechecked with postgis. In fact the area calculation is also good here. So forget about this.

@gioman
Copy link
Contributor Author

gioman commented Oct 28, 2019

The computation issue of only the bigger part of a multi polygon also happens when computing a field. This is unrelated to default value triggering.

Hi @haubourg I don't follow

In 3.8.2, default calculations are triggered correctly for me when adding parts.

works on 3.4.12, but this ticket is not about the "add part" tool operation, is about the "fill ring" one.

@gioman
Copy link
Contributor Author

gioman commented Oct 29, 2019

On master when using the fill ring the default expression (i.e. to update an area column) is applied.

As far as I understand the code changed since 3.4, but I guess that the LTR should be fixed anyway.

Regarding the "merge" operation:
well... I guess it is arguable if the expression used in an attribute should override the value that is "computed" by the "merge features" tool. Now is the latter, but if I could choose I would say that the default expression should have the priority.

@gioman gioman changed the title "fill ring" (and susequent "merge features") do not trigger the correct computation of the area in the "default value" expression "fill ring" do not trigger the correct computation of the area in the "default value" expression Oct 29, 2019
@troopa81 troopa81 self-assigned this Jan 23, 2020
@troopa81
Copy link
Contributor

@gioman Like you said it was already fixed in master, I can backporting it in LTR if possible

but if I could choose I would say that the default expression should have the priority.

I'm not sure about this. If you choose to make the sum for instance, you don't want the default value to be set, you want the sum. It would be maybe better to have an option "reset default value"

@gioman
Copy link
Contributor Author

gioman commented Jan 23, 2020

@gioman Like you said it was already fixed in master, I can backporting it in LTR if possible

@troopa81 Hi, I'm a bit lost about backports to 3.4. If is ok on 3.10.2 and master I would say that as the next major releases is near we could close this.

I'm not sure about this. If you choose to make the sum for instance, you don't want the default value to be set, you want the sum. It would be maybe better to have an option "reset default value"

I see your point, I agree that the option "reset default value" would be good to have.

@troopa81
Copy link
Contributor

@troopa81 Hi, I'm a bit lost about backports to 3.4. If is ok on 3.10.2 and master I would say that as the next major releases is near we could close this.

That's the case indeed, so I close this ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Crash/Data Corruption Editing High Priority
Projects
None yet
Development

No branches or pull requests

3 participants