Hey! Newcomer-ish here, getting my feet wet at contributing.

Some questions have crossed my mind while reading the patch:

• If I don't have an active node, the code keeps me from making a group from my selection. Is this intended behaviour? (Regarding line 501)
• If my active node happens to be an "ungrouppable" node, can't I just group the remaining ones? When accessed with ctrl+G, the "make group" operator seems to simply ignore bad nodes, which is quite convenient.
• While we're at it, perhaps it'd be time to consider unifying the operator names "Delete and reconnect" and "Dissolve", for consistency's sake? While used in wildly different contexts, they seem intuitively quite similar.

@Pablo Vazquez (pablovazquez) Since you're talking refactor here... 🙂
The Ctrl + Shift + Click hotkey to toggle the new rBc55d38f00b8c: Geometry Nodes: viewport preview is cool but it's somewhat a hidden hotkey and hard to discover, imo... Would be nice to have a "legal" way of accessing it, and the node context menu seems to be the right place for it, don't you think?

In D16216#441692, @G. Solis (liquidificador) wrote:

Hey! Newcomer-ish here, getting my feet wet at contributing.

Welcome!

Some questions have crossed my mind while reading the patch:

• If I don't have an active node, the code keeps me from making a group from my selection. Is this intended behaviour? (Regarding line 501)

Good catch, this was an oversight. Will fix it.

• If my active node happens to be an "ungrouppable" node, can't I just group the remaining ones? When accessed with ctrl+G, the "make group" operator seems to simply ignore bad nodes, which is quite convenient.

Didn't notice that Group Input/Output nodes had that behaviour, neat. The Render Layers node is not as flexible and simply throws an error, perhaps better to allow grouping regardless of selection, and change how that node behaves instead of adding the logic in the menu. I think I'll do this.

• While we're at it, perhaps it'd be time to consider unifying the operator names "Delete and reconnect" and "Dissolve", for consistency's sake? While used in wildly different contexts, they seem intuitively quite similar.

Nice one! Indeed it's a completely different context and a bit of a strange word to use here but only the first time, and Delete with Reconnect is too long anyway. I think only the menu entry should change, the operator should still be Delete with Reconnect, so it's easier to search as "delete".

This looks like a great improvement!
My only concern is the "Toggle" sub-menu. The naming seems a little awkward, like it's categorizing by button type rather than some more meaningful distinction.
Some properties like "Mute" could be exposed directly in the menu with a checkbox.

In D16216#441996, @Hans Goudey (HooglyBoogly) wrote:

This looks like a great improvement!

Thanks! This menu has been in the to-do since 2.80 days, it was time to tackle it.

Some properties like "Mute" could be exposed directly in the menu with a checkbox.

Ah much nicer indeed! Updated the patch.

My only concern is the "Toggle" sub-menu. The naming seems a little awkward, like it's categorizing by button type rather than some more meaningful distinction.

I wasn't too happy with it either. How about Show/Hide?, we use it in other places already. Moving the Mute toggle outside it kind of makes sense now.

Mockup (not part of the patch yet)

I wasn't too happy with it either. How about Show/Hide?, we use it in other places already. Moving the Mute toggle outside it kind of makes sense now.

That looks better to me!

Generally seems good. Just wanted to mention that the Link to Viewer doesn't quite work right now, because it uses the node/socket under the mouse instead of the active one. Maybe using node.link_viewer works instead of node.select_link_viewer, haven't tried right now.

In D16216#449007, @Jacques Lucke (JacquesLucke) wrote:

Generally seems good. Just wanted to mention that the Link to Viewer doesn't quite work right now, because it uses the node/socket under the mouse instead of the active one. Maybe using node.link_viewer works instead of node.select_link_viewer, haven't tried right now.

Indeed works much better, thanks!