Working paste on virtual node

[tunp]

When selecting virtual nodes and pasting tags to them Merkaartor crashes. This pull request creates normal nodes out of virtual nodes when user pastes tags to them.

[Krakonos]

Thanks, looks good. I'll have a more detailed look and a test a bit later today.

[Krakonos]

First of all, please know I don't mean to be negative or dismissing, and that I do appreciate your contribution. However, I went through the change and the UI this entails and noticed a few things I don't like:

1) (minor issue) Adding two pieces of duplicate code. Not that this is your fault, as the code already is a bit repetitive, but I'd prefer if we could deduplicate, especially if we're adding this large-ish loop.

2) (major issue) I don't think this needs to be implemented. On the contrary, I think we should just block the paste action on virtual nodes. Here is my rationale:

a) The virtual node is a convenience feature that allows us to add points into the middle of path easily. b) Virtual nodes cannot be assigned tags by any standard tool (you have to promote them to a standard node first to make the properties dialog recognize them as taggable objects). c) Virtual nodes are not selectable by any standard tool, or not selectable at all (depending on settings).

By points a) and b), I believe we should either keep the virtual nodes to the original purpose and not allow tagging on them, or promote them to something more universally useful - but that needs to be done at all levels. I personally don't see much benefit in this auto-promotion of virtual nodes, especially if they are currently hard to select (see discussion for c) below). I also don't like benign actions like pasting a tag have a side-effect of creating a node. That seems plain wrong to me.

To comment on c): selecting area does not select virtual nodes on purpose (not needed, not wanted, as many operations cannot be performed on these, and they are volatile). Depending on global settings ("Preferences -> Visual -> Separate move mode"), they are not selectable. They are only selectable when this option is off, and I would consider this a bug.

However, producing a crash in this instance is not an acceptable solution, so I find this PR a valuable input. However, I would prefer another solution in the form of preventing virtual nodes to be selectable. There could even be an assertion that verifies no virtual node is ever inserted into the selection list in case we have more options for selecting these (I didn't find any).

Lastly, I'd like to ask if you find value in the ability to select and paste tags onto virtual nodes? Because I don't see it doesn't mean it's not there and I'm always happy to be proven wrong. In case yes, I would propose some more consistent alternative - for example, double-clicking virtual node would convert it into a full node, that can be used as usual.

If you do agree with my arguments, would you mind looking into fixing the issue by disabling selection of the virtual node in the first place, as suggested above? Also, if you feel like double-clicking a virtual node to convert it into full node brings in some value, feel free to contribute that, but please make it as a separate PR. In any case, feel free to disagree with me, I'm ready to have a civilized discussion and hear arguments contrary to my own.

[tunp]

Thank you for reviewing and you are totally right. I was also thinking if this is right solution but then tough that it should work as like moving virtual node as if user somehow edits node it should become a normal node. However, I don't think that making virtual nodes non-selectable is the right solution as in "Separate move mode" you have to select a node before you can start moving it and it should work same for virtual nodes too. Maybe the best solution would be just to disable the paste option in the menu. But of course it is ok for me if you prefer some other solution.

[Krakonos]

Except that it doesn't work exactly that way. In "Separate move mode", the nodes are not selectable and you are expected to just drag them away. It is when the mode is off they become selectable, as it blends with the "Select mode". You can then select two of the nodes and drag them away.

However, keeping them selectable opens another can of worms: other tools don't really expect that, and some of them fail silently and some of them even crash (Just tested with "detach"). We'd have to verify all the tools and operations in all the menus and shortcuts and fix those as well (Feature -> Force delete hangs Merkaartor).

Seeing how much work this is, I think we need to disable the selection as a stopgap measure and consider adding this as a new feature. However, I don't think it's worth the effort, as selecting the virtual nodes isn't really that useful. I'd rather have the time invested in other things (like more useful tools for editing harder stuff like multipolygons etc.)

I'm just tagging 0.19.0-rc1 and would like to pull this bugfix (and the other one if possible) into the final 0.19.0, so getting a robust fix as soon as possible is a priority. We can tackle the improvements for the next release.

[Krakonos]

Is there any progress regarding the things we discussed? Will you have time to look into it? If not, that's fine, I'll implement a temporary fix along those lines and we can develop if further when you have time.

[tunp]

Unfortunately I have had no time to look into it. If you could make a temporary fix that would be great.

[Krakonos]

Hi! I have implemented a workaround in PR #254. I believe it works around the multiple issues identified. Can you give that a go?

[Krakonos]

I'm closing the PR due to inactivity and low priority of this actually needing the fix, since the crash has been eliminated by disabling selection itself. Virtual nodes can still be moved (and thus "morphed" into full nodes) by using the "Move" command.

Reopen if you ever feel it's worth the time to fix the other issues pointed out in the discussion.