Undoable Polyline segments

[HeCorr]

This PR adds the Remove Last Polyline Segment shortcut, which defaults to the Backspace key and removes individual Polyline points when pressed:

2024-07-20.04-00-22.mp4

Note: if there's only one point left, it is also removed (by cancelling the Polyline as if pressing Escape).

[MrStevns]

I don't think this needs a specific shortcut, using backspace seems fine to me.

When that's said, rather than implementing something for this specifically, why not utilize undo/redo instead?

[HeCorr]

I don't think this needs a specific shortcut, using backspace seems fine to me.

I think it's best to make it customizable, in case someone wants to change it (isn't that the whole point of customizable shortcuts?).

When that's said, rather than implementing something for this specifically, why not utilize undo/redo instead?

This way was much simpler to implement and follows the behavior of other software such as Photoshop (i.e. the Poly Lasso tool).

Also, the whole Polyline gets applied at once when you press Enter instead of the individual points, so I don't even know if the undo system would work here.

I could give it a try but if it doesn't just work I don't think I'd be willing to keep trying. This implementation is fine and works well despite being simple, and since it is such an essential functionally I rather just ship it sooner than trying to make it perfect.

[HeCorr]

PR updated according to the latest changes.

[MrStevns]

My bad, the shortcut does not work without being coupled to some UI element, so for now it might be OK to put it back in the edit menu.

Ideally though I think we could benefit from having a dynamic menu item that can be used to store actions of the current tool, so we don't pollute the other menu items with very specific tool actions.

[HeCorr]

My bad, the shortcut does not work without being coupled to some UI element

Wait, what? It works fine on my end, I've honestly never noticed such behaviour.

Ideally though I think we could benefit from having a dynamic menu item that can be used to store actions of the current tool

Maybe buttons could be added to the tool options since that's already specific to the active tool? Or maybe in a section underneath it.

[MrStevns]

Wait, what? It works fine on my end, I've honestly never noticed such behavior.

And you tested it again after you removed the menu item from the Edit menu?
Regardless, when I press the shortcut button, nothing happens. I can't make it trigger the action at all.

[HeCorr]

And you tested it again after you removed the menu item from the Edit menu?

It has worked before I added it and after I removed it from the menu, but I can test it one more time to be absolutely sure.

Regardless, when I press the shortcut button, nothing happens.

That's weird. Are you on a Linux system? Because I'm on Windows. Can you please double-check that the shortcut is set, and try to change it to something else?

[MrStevns]

I went and made another test and changed shortcut as you asked, though it still didn't work.
I'm on macOS.

The documentation also mention that "Actions are added to widgets using QWidget::addAction() or QGraphicsWidget::addAction(). Note that an action must be added to a widget before it can be used. This is also true when the shortcut should be global (i.e., Qt::ApplicationShortcut as Qt::ShortcutContext)."

It does however work if you add actionRemoveLastPolylineSegment to the MainWindow widget by calling QWidget::addAction(...)

[HeCorr]

Well, that's awkward. It has worked this whole time but doesn't anymore, I guess I really forgot to re-test it after removing the menu item and remembered it working before adding it because it used to be hardcoded (PolylineTool::keyPressEvent) until I migrated it over to the shortcut system and immediately added it to the UI.

Actually, I did test it again afterwards, but on a personal branch which still has it hardcoded. 😅 My sincere apologies for the confusion.

It does however work if you add actionRemoveLastPolylineSegment to the MainWindow widget by calling QWidget::addAction(...)

Does that add it visually as a button/menu item? Also where and how exactly would I call that?

[MrStevns]

Does that add it visually as a button/menu item? Also where and how exactly would I call that?

If it does get added somewhere, I haven't figured out where yet, so I don't think so.
You could start by putting it at the bottom of MainWindow2::setupKeyboardShortcuts, then we can figure out a better place for it when we get more shortcuts that are not connected to UI elements.

[HeCorr]

I'll try that tomorrow. I just don't know which widget to add it to.

[MrStevns]

You can simply add it to the main window, which is also a widget, eg.

MainWindow2::setupKeyboardShortcuts()
{
...
addAction(ui->removeLastPolylineSegment);
...
}

[HeCorr]

Fixed and re-tested, 100% working now! Thanks for the help 🙂

2024-07-25.16-06-38.mp4

[MrStevns]

I can't decide whether it's okay to have an emitter meant for a specific tool in Editor, maybe it should go through ToolManager instead, yet the ToolManager feels more like a bridge to the GUI part like the toolbox, which we do not use with the shortcut specified here, so maybe it's fine?

Anyone else who wants to throw in their 2 cents?

[J5lx]

Anyone else who wants to throw in their 2 cents?

Tbh I don’t think this requires any extra indirection at all. Just connect QAction::triggered to PolylineTool::removeLastSegment directly. MainWindow2 already connects directly to PolylineTool’s isActiveChanged signal, we can just do the same for the segment removing slot.

And unrelated, I think it would be great to add this to the help text in StatusBar::updateToolStatus as well.

[MrStevns]

Tbh I don’t think this requires any extra indirection at all. Just connect QAction::triggered to PolylineTool::removeLastSegment directly. MainWindow2 already connects directly to PolylineTool’s isActiveChanged signal, we can just do the same for the segment removing slot.

Hmm ah good point, I forgot we had that one. It's been addressed now.

And unrelated, I think it would be great to add this to the help text in StatusBar::updateToolStatus as well.

Yes that would be nice but it's already quite long and will truncate with the additional information. I'm not sure expanding the bottom to accommodate for that is a good idea but what other alternatives are there?

Maybe we could randomize the hints 🤔

[J5lx]

Maybe we could randomize the hints 🤔

I don’t really like that either because then the most relevant entries might be missing. GIMP uses a generic “(try Shift, Ctrl, Alt)” for cases like these, but that of course that is kinda vague. Personally, I still have some space left on my Full HD screen which looks like it’d be enough, maybe it’d be good enough to add a tooltip for smaller screens? I don’t think there’s a perfect solution anyway.

That said, I don’t want to block this PR over that. If you are also happy with the state of the PR, let’s merge.

[MrStevns]

Ah you ended up static casting to get access to the tool methods directly, I thought about that too but since we didn't do that anywhere else, I made the function virtual and accessed it through BaseTool.

If you're okay with accessing it directly, then I'm okay it too.
Let's merge it.

[J5lx]

Yeah but I think we overuse virtual methods somewhat and it results in a bunch of interface pollution, hence why I changed it.