Create an editor widget wrapper for geometry fields #51863
Conversation
| mMenu = new QMenu( this ); | ||
|
|
||
| mCopyWktAction = new QAction( mMenu ); | ||
| mCopyWktAction->setText( tr( "Copy as WKT" ) ); |
There was a problem hiding this comment.
Is EWKT supported in copy and paste?
In general, I see that CRS is handled in updateLineEdit but not elsewhere.
I'm not sure about the implications of supporting CRS in this widget.
There was a problem hiding this comment.
Not yet, no -- since ewkt interpretation depends on the database instance, that would require some public data provider method for resolving strings to referenced geometries first.
There was a problem hiding this comment.
Basically what happens with the widget is:
- if no changes are made, then original crs is maintained and the geometry is unchanged. The attribute form never gets notified that the field value is changed.
- if a user changes the geometry by pasting wkt/geojson, then the crs will be reset. The postgres provider then gets a crs less geometry to store. On my tests this means that field's crs is automatically used (by logic somewhere in postgis)
To me that result is ok, and definitely better then the current fragile approach of going via the text edit widget. But we could further improve by :
- adding a crs selector/setter option to this widget to allow users to manually set the crs for changed geometries
- adding the crs (and geometry type) information to the field definition somehow. Right now we have no way of knowing the postgres field's crs and type constraints outside of the provider. Maybe a freeform metadata variant map in QgsField would work for this, or maybe a more formal/strict API is needed ...
|
Thanks @nyalldawson -- I've given the new code a try and I confirm it fixes the data corruption reported in #49380 -- it still doesn't render geometries with no SRID, which I think my PR does, so we could handle that part over there. The "Paste Geometry" action doesn't seem to be allowing me to enter WKT or EWKT manually so I'm not sure how one is supposed to edit those geometric fields. Only after copying WKT you are allowed to paste, but then what you see in the input field is just |
|
Please see #51885 for not discarding geometries with no CRS |
I opted for just showing the geometry type in the display as I didn't want to show potentially massive wkt strings. Maybe we could show a trimmed wkt instead ... |
|
- if a user changes the geometry by pasting wkt/geojson, then the crs
will be reset. The postgres provider then gets a crs less geometry to
store. On my tests this means that field's crs is automatically used
(by logic somewhere in postgis)
This is only true with PostGIS 3.1.0 or higher, and only works
when the field *does* encode a crs (using "typmod") which is still
an optional thing to do.
|
|
On Wed, Feb 15, 2023 at 11:36:42AM -0800, Nyall Dawson wrote:
> what you see in the input field is just Point, with no coordinates at all. Am I doing anything wrong
I opted for just showing the geometry type in the display as I didn't want to show potentially massive wkt strings. Maybe we could show a trimmed wkt instead ...
What surprised me was that the original value was shown in full,
including the CRS, and just turned to just `Point` upon pasting
a WKT. It's the inconsistency that i noticed.
A trimmed wkt always would be better, IMHO.
Even better would be a graphical representation of the geometry,
with editing done via the QGIS editing toolbar, but I understand
this is asking too much for the first step :)
|
Heh, there's LOTS more we could do here. I'm thinking also a summary tooltip eg with part/vertex count, even a preview rendering of line/polygon geometries. And things like "zoom to" and "flash on canvas" in the action menu...
Right. I'm not sure honestly how to handle that though, since wkt/geojson in the clipboard won't have a crs available. I guess for GeoJSON we could assume EPSG:4326, but can't for WKT. We could also just assume that it's in the same crs as the original geometry (assuming the field has an existing value, of course), but that's only going to be correct in a limited set of circumstances and seems to me more likely to cause issues. That's why at the moment any edits drop the CRS and just delegate it to the data provider to decide how to handle this. My gut feeling is that this is the best we can do for now, until we do something like adding the field metadata option described in #51863 (comment) . |
nyalldawson
force-pushed
the
geom_widget3
branch
from
7c4ada4
to
ead1490
Compare
|
How about using the CRS of the original value upon pasting something w/out a CRS ? Would sound like a sensible way to proceed here. I would prefer this over adding the metadata to QgsField as the field might even NOT be constrained and thus not mandate a specific SRID. |
I'm thinking now of showing the crs selector dialog directly after pasting any geometry so that we force users to make the correct choice, and avoid all the potential issues if we just try and guess it ourselves. The selector could default to 4326 for geojson pastes or the current crs for wkt pastes. Even if we can make the fields crs accessible here this would still be a good move as we could then immediately transform the geometry to the required destination crs (respecting the user's transformation settings).
I still think exposing the metadata is a good step forward. Even without crs information if we had access to the geometry type then we can make this a lot more user friendly by eg auto promoting to multi types, segmentizing curves, dropping Z/m etc... |
Is there a way we can enhance that, at least for (E)WKT since GeoJSON is supposed to be 4326 by default ? |
I was thinking.. how about a "Paste CRS WKT" ? |
A new widget which stores a referenced geometry value, and provides handy methods for copying the value as WKT or GeoJSON, pasting a WKT/GeoJSON value from the clipboard, or clearing the geometry
This will show a dedicated widget for geometry fields, allowing for proper lossless use of these fields in the attribute form Refs qgis#49380
Yes. We could move some of the logic from QgsClipboard from app into gui, so that we can access the CRS associated with copied features. But that's non-trivial and I'm not planning on doing it here. |
Since we don't know what CRS is associated with geometries from WKT/JSON strings in the clipboard, force the user to make the correct choice for us.
nyalldawson
force-pushed
the
geom_widget3
branch
from
ead1490
to
810c112
Compare
I've implemented this now. I think it's as good as we'll get without significant effort in refactoring the app library clipboard handling. Let's get this merged now as even if it's not perfect, it's a significant step forward and avoids all risk of data loss. |
|
I've tested the current version of the code: good work ! Maybe "Paste Geometry" label should specify which formats are supported (WKT and GeoJSON seems to be both supported). There doesn't seem to be a way to specify an "unkonwn SRID", not sure if it can be a problem ( @MorriganR would it be a problem for your case ? ). Note that sources with unknown SRID are still rendered as NULL values, but saving changes does not seem to really currupt the underlying dataset. I've a PR that deals with the unkonwn-SRID support ( #51885 ) but didn't try to merge it with the code in your branch to see how it would behave. |
|
The clang-tidy failure might be a real issue: https://github.com/qgis/QGIS/actions/runs/4220049786/jobs/7327818570#step:8:200 |
This will show a dedicated widget for geometry fields, allowing for proper lossless use of these fields in the attribute form
Refs #49380