Path bread crumbs widget by ericmehl · Pull Request #6901 · GafferHQ/gaffer

ericmehl · 2026-04-22T15:02:33Z

This adds a few new navigation widgets to the Graph Editor. It adds a new BreadCrumbsWidget for working with paths and makes use of it in the Graph Editor. There are also forward and back buttons for navigating the history of locations the Graph Editor has been rooted to.

There's a known test failure in GraphEditorTest. This stems from the addition of __rootPath in the Graph Editor. This needs to be kept in sync with the actual node that is the root of the Graph Editor. But we've been switching that root using GraphGadget.setRoot(). This made for somewhat loose coupling between the two and setup two different methods for setting and responding to root changes - GraphGadget.rootChangedSignal() and __rootPath.pathChangedSignal(). The history navigation in particularly was difficult with this setup.

So for the Graph Editor, I've settled on modifying the root via the __rootPath. I didn't fix that test and add additional coverage yet so as to not put too much time into a change that may have a better solution.

I imagine besides the code, there are probably some discussions to be had on the widget styling as well.

Checklist

I have read the contribution guidelines.
I have updated the documentation, if applicable.
I have tested my change(s) in the test suite, and added new test cases where necessary.
My code follows the Gaffer project's prevailing coding style and conventions.

ericmehl · 2026-04-22T22:25:38Z

John and I talked through the __rootPath vs graphGadget.getRoot() methods of setting the root, and the results are in the latest push. I just overwrote the history for simplicity, but I have the original on a backup branch if it's needed.

johnhaddon

Thanks Eric! I think folks are going to find this very handy - they have been asking for a long time so it's nice that we've finally got to it.

Code comments inline as usual. I'd like to spend a bit more time playing with the history navigation, and I also have some thoughts on the styling, but I'll follow up on those separately. @murraystevenson, I don't think we need a second code review, but it might be good to get your thoughts on usability etc.

Cheers...
John

johnhaddon · 2026-04-22T15:36:52Z

+
+		scriptNode = self.ancestor( GafferUI.ScriptWindow ).scriptNode()
+		n = scriptNode.descendant( pathString ) if pathString else scriptNode
+		GafferUI.GraphEditor.acquire( n )


The original plan was for this to be a widget that worked purely on Path objects, and wouldn't know anything about the GraphEditor. We should either keep to that, or move it into GraphEditor.py as a private class.

johnhaddon · 2026-04-23T12:54:57Z

 		if not isinstance( QtWidgets.QApplication.focusWidget(), ( QtWidgets.QLineEdit, QtWidgets.QPlainTextEdit ) ) :
+			# \todo Do we want to clear the `focusItem` here too? If not, the breadcrumbs text
+			# widget will get focus as soon as this GadgetWidget gets focus, which may not be
+			# intuitive?


GadgetWidget is a generic component, so we need to think in general terms rather than just about the breadcrumbs text. We already have QLineEdits in the Viewer toolbars, for example.

One thing that seems strange in our current behaviour is that we're careful not to steal focus from QLineEdit and QPlainTextEdit, but this doesn't apply when those are hosted in a QGraphicsView. Which gives us the following sequence :

Focus a graphics item (breadcrumbs or exposure in Viewer).

Move mouse to another editor - we lose focus (Editor.__enter() is much the same as GadgetWidget.__enter()).

Move mouse back. We gain focus again.

The comment talks about changing the behaviour of 3, but I think changing 2 might be just as important, if not more so. If we never lose focus, then it's not a surprise when we still have it.

johnhaddon · 2026-04-23T13:00:57Z

+			focusWidget = focusWidget.scene().focusItem()
+
+		if not isinstance( focusWidget, QtWidgets.QGraphicsProxyWidget ) :
+			self._qtWidget().clearFocus()


I don't understand how this relates to the commit message : "GadgetWidget : Don't lose item focus on leave". Isn't it doing the opposite? It also doesn't seem to be making any distinction between QGraphicsView in general this QGraphicsView, which makes me think something isn't quite right.

johnhaddon · 2026-04-23T14:02:20Z

 		border: 1px solid $brightColor;
 	}

+	#gafferBreadCrumbs {


Can we use the gafferClass selector rather than #gafferBreadCrumbs. I think the general intention with the stylesheets is that #objectName is intended for special cases for individual instances of a widget, whereas gafferClass is for all instances.

johnhaddon · 2026-04-23T14:07:26Z

+				"<br><kbd>Down</kbd> for contents menu."
+				"<br><kbd>Up</kbd> to change to container path."
+				"<br><kbd>Tab</kbd> for auto-complete."
+				"<br><kbd>Home</kbd> to return to root."


Could we format these as a bullet list? We do that for plug tooltips and I think it makes it a little easier to read.

johnhaddon · 2026-04-23T14:50:44Z

+			self.__rootChangedConnection = self.__graphEditor().graphGadget().rootChangedSignal().connectFront( Gaffer.WeakMethod( self.__rootChanged ), scoped = True )
+			self.__graphEditor().graphGadgetWidget().keyPressSignal().connect( Gaffer.WeakMethod( self.__keyPress ) )
+
+			self.__history = [ ( self.__graphEditor().scriptNode(), None ) ]


This gets clobbered at L97, so I assume can be removed from here.

johnhaddon · 2026-04-23T14:59:21Z

+		# \todo It would be nice to get the `GraphEditor` from the hierarchy.
+		# But the ancestors of this widget are two `ListContainer` objects (including
+		# in `_postConstructor()`).
+		# `Widget.parent()` should be taking care of crossing the QGraphicsProxy


It would be even nicer for HistoryWidget to be self-contained and not need access to the GraphEditor, just using GraphGadget instead (since it's role is to manage the root for the GraphGadget). It seems to be coupled for two reasons :

HistoryWidget wants to handle keypresses on GraphEditor. But GraphEditor could do that itself, and call a method on HistoryWidget to do the work. It's fine for GraphEditor to know about HistoryWidget.

HistoryWidget wants to call __currentFrame(). But that just needs the ViewportGadget, which can be got from the GraphGadget. So I think that could be a free function used by everyone.

johnhaddon · 2026-04-23T15:08:06Z

+						# `GraphGadget` won't emit `rootChangedSignal()` because the roots are the same.
+						# Framing might be different, so we do it ourselves.
+						frame = self.frame( rootNode )
+						if frame is not None :
+							graphEditor.graphGadgetWidget().getViewportGadget().frame(
+								imath.Box3f( imath.V3f( frame.min().x, frame.min().y, 0 ), imath.V3f( frame.max().x, frame.max().y, 0 ) )
+							)
+						else :
+							self.__frame( graphEditor.graphGadget().getRoot().children( Gaffer.Node ) )


We seem to be in two minds about who should do the framing. Sometimes GraphEditor does it, using a frame supplied by us, and sometimes we do it, because we think the GraphEditor won't. Can this be tidied up? I suppose the reason we're doing this at all is so that the same node can have two different framings if it appears twice in the history?

johnhaddon · 2026-04-23T15:15:57Z

+				menuDefinition.append(
+					prefix + str( counter ),
+					{
+						"label" : self.__history[i][0].relativeName( graphEditor.scriptNode() ) if not self.__history[i][0].isSame( graphEditor.scriptNode() ) else "Script Root",


I think perhaps just "Root" rather than "Script Root". Or "Home", to match the icon?

Or maybe /, and we use / separators here to match the separators in the breadcrumbs widget. The thing that's bothering me is that the fact the following three items are nested is not visually obvious from the menu :

Whereas these are :

/A/Long /A /

Or maybe /, and we use / separators here to match the separators in the breadcrumbs widget...

I'd second this approach. I find the words "Root" or "Script Root" here just make the history menu harder to scan.

johnhaddon · 2026-04-23T15:31:38Z

 - ShaderTweaks : Added support for `{shaderType=someShaderType}` qualifiers in parameter names, allowing tweaking of a parameter on all shaders of a given type (#6838).
- Graph Editor : Added location bar for additional control of the Graph Editor's root. Text and button interactions can be used to navigate the node hierarchy.
+- Graph Editor :
+  - Added location bar for additional control of the Graph Editor's root. Text and button interactions can be used to navigate the node hierarchy.


I wonder if "control of the Graph Editor's root" means as much to the average user as "navigation through Boxes and References"?

murraystevenson · 2026-04-23T21:27:44Z

+				self.__backButton = GafferUI.Button( "", "historyBack.png", hasFrame = False, toolTip = "Step back in Graph Editor history. Right-click for past-nodes popup menu.[<kbd>[</kbd>]" )
+				self.__forwardButton = GafferUI.Button( "", "historyForward.png", hasFrame = False, toolTip = "Step forward in Graph Editor history. Right-click for future-nodes popup menu.[<kbd>]</kbd>]" )


We'll want to update the documentation to include these shortcuts n the Graph Editor > Navigation section of ControlsAndShortcuts.

I wonder if the tooltip wording could be simplified along the lines of "Go back. [[]<br/>Right-click for history menu."

It's also unfortunate that there's no overly distinct styling for <kbd> in the tooltips so we wrap shortcuts in [], which results in the not super-readable []] and [[] here. Maybe a <span> with a bit of inline styling could be a better representation?

murraystevenson · 2026-04-23T22:30:41Z

+			with self.__row :
+				self.__backButton = GafferUI.Button( "", "historyBack.png", hasFrame = False, toolTip = "Step back in Graph Editor history. Right-click for past-nodes popup menu.[<kbd>[</kbd>]" )
+				self.__forwardButton = GafferUI.Button( "", "historyForward.png", hasFrame = False, toolTip = "Step forward in Graph Editor history. Right-click for future-nodes popup menu.[<kbd>]</kbd>]" )
+				self.__backButton.buttonPressSignal().connect( Gaffer.WeakMethod( self.__backButtonPressed ) )


I think we'd also want to do the same for the pathButtons and the rest of BreadCrumbsWidget, otherwise double-clicking falls through to whatever is behind, which could acquire a NodeEditor if there's a node positioned behind. Hovering over the '/' labels in the BreadCrumbsWidget can also show the tooltip of whatever is behind, such as the Parent node in the GIF below.

There's some odd interaction with the FocusGadget and the back and forward buttons or BreadCrumbsWidget overlap the top-right corner of a node.

I've been able to trigger a crash in FocusGadget when double-clicking the back or forwards buttons when they overlap it, where the first click changes the graph editor root and then FocusGadget::buttonDoubleClick tries to set focus to a node in the old graph editor root and segfaults in GafferUI::NodeGadget::node(). There are also some ERROR : FocusGadget::nodeMouseEntered : Focus gadget hover timer triggered on unparented FocusGadget messages cropping up when single clicking on the back or forwards buttons while they overlap the FocusGadget, this depends on how quickly you click after hovering over the FocusGadget on the way to the back or forward button.

And there's this funky one where double-clicking in the BreadCrumbsWidget immediately after setting focus somehow toggles focus.

murraystevenson · 2026-04-23T22:33:51Z

+				menuDefinition.append(
+					prefix + str( counter ),
+					{
+						"label" : self.__history[i][0].relativeName( graphEditor.scriptNode() ) if not self.__history[i][0].isSame( graphEditor.scriptNode() ) else "Script Root",


Or maybe /, and we use / separators here to match the separators in the breadcrumbs widget...

I'd second this approach. I find the words "Root" or "Script Root" here just make the history menu harder to scan.

murraystevenson · 2026-04-23T22:39:26Z

+			result.append(
+				pathPrefix + childPath[-1],
+				{
+					"command" : functools.partial( Gaffer.WeakMethod( self.__setPathEntry ), childPath ),
+				}
+			)


From a user's perspective I find it a bit odd seeing "Assets" as a choice in the menu above when it appears to do nothing. Maybe we should disable entries that are ancestors of the current path, or only show their siblings?

murraystevenson · 2026-04-23T22:47:01Z

+			if isinstance( parentWidget, GafferUI.TextWidget ) :
+				xOffset = parentWidget._qtWidget().cursorRect().left()
+
+		self.__popupMenu = GafferUI.Menu( menuDefinition, title = self.__popupMenuTitle )


I find the "Node Path" menu title a bit unnecessary in the Tab completions menu, and distracting when there are only a few completions. Do we need it at all?

GadgetWidget : Don't lose item focus on leave

f05ebb0

ericmehl force-pushed the pathBreadCrumbsWidget branch 2 times, most recently from 01e7b96 to 55a3ecc Compare April 22, 2026 21:13

ericmehl added 3 commits April 22, 2026 18:07

BreadCrumbsWidget : Widget with interactive path

1951338

GadgetWidget : mouseMove to overlay first

b28fe1f

GraphEditor : History forward and back buttons

f383a82

ericmehl force-pushed the pathBreadCrumbsWidget branch from 55a3ecc to f383a82 Compare April 22, 2026 22:23

johnhaddon reviewed Apr 23, 2026

View reviewed changes

murraystevenson reviewed Apr 23, 2026

View reviewed changes

murraystevenson assigned ericmehl Apr 23, 2026

murraystevenson added this to Work in Progress Apr 23, 2026

github-project-automation Bot moved this to Pending Review in Work in Progress Apr 23, 2026

		self.__backButton = GafferUI.Button( "", "historyBack.png", hasFrame = False, toolTip = "Step back in Graph Editor history. Right-click for past-nodes popup menu.[<kbd>[</kbd>]" )
		self.__forwardButton = GafferUI.Button( "", "historyForward.png", hasFrame = False, toolTip = "Step forward in Graph Editor history. Right-click for future-nodes popup menu.[<kbd>]</kbd>]" )

Conversation

ericmehl commented Apr 22, 2026

Checklist

Uh oh!

ericmehl commented Apr 22, 2026

Uh oh!

johnhaddon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants