<Change Node Type> does not work correctly

It is perhaps worth mentioning that while the mouse is hovering over the control handle, the following text appears in the information box at the bottom of the Inkscape window:

    "Auto node handle: drag to convert to smooth node"

It is unclear what this is supposed to mean, because, of course, dragging the control handle does not result in any such conversion of the node type.

Apparently, the "Auto node handle" message is relevant, and correct, in the case that the node type is actually "auto-smooth", where dragging the control handle really does convert the node type to "smooth".

However, the same message is displayed when the mouse hovers over the control handle, regardless of node type, and for cusp, smooth, and symmetric nodes, it is highly misleading. Even for auto-smooth nodes, it should read "auto-smooth node handle: ..."

In the case of cusp nodes, it might be appropriate to display a message about control-handle locking for conversion to smooth or symmetric, and for smooth nodes, about conversion to symmetric. I was not previously aware of this supposed feature; it does not seem to be documented anywhere except the keyboard command reference, so this would be a useful way to make users aware of it.

But this should only be done when the control-handle locking function works properly, as it currently does not.

It appears that the code responsible for control-handle locking when
converting a cusp node to smooth is in <src/ui/tool/node.cpp>, lines
286-292:

    case GDK_KEY_S:
        if (held_only_shift(event->key) && _parent->_type == NODE_CUSP) {
            // when Shift+S is pressed when hovering over a handle belonging to a cusp node,
            // hold this handle in place; otherwise process normally
            // this handle is guaranteed not to be degenerate
            other()->move(_parent->position() - (position() - _parent->position()));
            _parent->setType(NODE_SMOOTH, false);

https://gitlab.com/inkscape/inkscape/blob/masterFix1777530/src/ui/tool/node.cpp#L286

As discussed above, "make smooth" should rotate the opposite control
handle to be collinear with the locked handle, while preserving its
length. The current code instead adjusts the length of the opposite
control handle to be the same as the locked handle. This seems to
contradict the Inkscape manual:

    <Shift+S>: Make selected nodes smooth. When the keyboard shortcut is
    used, placing the mouse over a handle will preserve the position of
    that handle, rotating the partner handle, if extended, to be
    collinear. If the partner handle is not extended, the partner handle
    will be extended so that it is collinear and of the same length as
    the preserved handle.

http://tavmjong.free.fr/INKSCAPE/MANUAL/html/Paths-Editing.html

Presumably, if there were any code for control-handle locking when
converting a node to symmetric, it would be found in the same place. If,
as it appears, no such code exists, that would explain why this alleged
feature does not work, at all.

The attached patch fixes the behaviour of control-handle locking during
conversion to smooth and symmetric path nodes, to conform to what is
described in the keyboard reference and the manual.

The second patch here attached alters the tip information displayed
during path node editing, to more accurately describe the actions
available for the control that the mouse is currently hovering over, as
discussed above.

Unlike the first patch, it does not alter Inkscape's actual
functionality in any way. I have separated the two, because the
information text will presumably require internationalization.
Hopefully, the first patch can be applied immediately (it seems to work
perfectly), and the second when translations of the new and altered text
are available.

It appears that there is another node-editing bug, related to "undo"
functionality. Since this bug report is still open, and some of the same
code is involved, I will describe it here, rather than opening another
bug report.

in the same function that I patched, above:

    _parent->_pm().update(); // magic triple combo to add undo event
    _parent->_pm().writeXML();
    _parent->_pm()._commit(_("Change node type"));

https://inkscape.gitlab.io/inkscape/doxygen/node_8cpp_source.html#l00293

(observe that the "_commit()" member function includes a call to
"writeXML()", so the explicit call above is at least redundant.)

https://inkscape.gitlab.io/inkscape/doxygen/path-manipulator_8cpp_source.html#l01660

The same test file "node-bug.svg" attached above, will serve again.

With the node editing tool, select the cusp node at the top of the red
quadrilateral. Change the node type to "smooth", with or without
control-handle locking. Verify that the node type really has been
changed. Then "undo" and "redo" this change. Observe that "redo"
restores the new position of the control handles, but it DOES NOT
restore the "smooth" node type. Unlike the original change, with "redo",
the node type remains "cusp".

The same thing happens when changing the node type to "symmetric" or
"auto-smooth", followed by undo/redo. Presumably, it would apply to all
twelve cases of node-type changes.

    ( original-node-type(4) * new-node-type(3) = 12 )

Apparently, the "magic triple combo" above, captures the control-handle
movement, but not the node-type change. Since the code above seems to
apply only to the locked-handle case, presumably it is replicated
somewhere else, for the non-locked case.

Perhaps somebody familiar with the Inkscape codebase knows how to fix
this problem; I do not. Please note that my patch-1, above, duplicated
this code for the previously-unimplemented
"locked-handle-change-to-symmetric-node" case, so any fix for the
"redo-node-type-change" bug will need to be applied in at least six
places (two locked, four unlocked).

The patch attached to this comment fixes the undo/redo bug described above.

This is not a permanent fix, but a temporary fix (see comment in code to that effect) which suggests where the actual source of the problem may be found; it would probably be obvious to someone familiar with the Inkscape codebase.

This patch, and the bug that it addresses, are entirely independent of the two patches previously submitted above.

The attachments to this comment are updated versions of the first two patches given above, to fix control-handle locking and tip information, respectively. However, the redundant calls to PathManipulator::writeXML() have been removed.

second patch, updated version.

further discussion of these issues may be found here:

https://gitlab.com/inkscape/inkscape/merge_requests/303

The node-handle-locking and info-text bugs have been fixed. However, it appears that the undo/redo node-type-change bug probably will not be addressed, for reasons that remain unclear.

If anyone cares to look into it, it's easy to reproduce, using the "node-bug.svg" test case, attached above. This description is copied from the GitLab discussion:

- enter node-edit mode, and select the cusp node at the top of the red quadrilateral. hover the mouse over it, and observe that the info box at the bottom of the window confirms that it is a "cusp node".

- convert the node to smooth, using either the GUI tool or the keyboard shortcut. again, hover the mouse over the node, and observe that the info box now describes it as a "smooth node".

- "undo" this operation. hover the mouse over the node, and observe that it is once again described as a "cusp node".

- "redo" the node-change operation. hover the mouse over the node, and observe that although the positions of the control handles have been "redone", it is STILL identified as a "cusp node".

- move one of the control handles, and observe that the other DOES NOT maintain collinearity with it. which is, of course, the definition of a "cusp node".

Conclusion: undo/redo DOES NOT WORK in the context of node-type changes.