ContextTracker : Add new UI class for tracking active contexts #5961
Conversation
I added this prematurely, and it turns out I don't need it - what I really need is `previousIteration()`.
This allows the user to get explicit feedback about ContextTracker updates in the unlikely event that they are slow enough to not be done before the menu is accessed. I've deliberately only shown the BusyWidget when the user "opts in" via a refresh as I think most updates are going to be so rapid that it would flicker annoyingly for them.
If we're not showing upstream nodes that aren't active, I don't see any justification for showing downstream nodes that can never be active. This also lets us slightly simplify the menu building logic.
I've removed most of the tests associated with the original state tracking as they are superceded by direct testing of ContextTracker. One test remains to demonstrate that we do correctly update the UI based on changes to the ContextTracker.
This lets us highlight the currently active row.
johnhaddon
force-pushed
the
contextTrackerPR1
branch
from
198369f
to
4ed9d2a
Compare
There was a problem hiding this comment.
I've now gone through the 3 ContextTracker commits pretty thoroughly, and the GraphGadget commit ( since I'm familiar with the old implementation ), and it's all looking pretty good to me.
We discussed the naming of "isActive" offline, and I liked John's suggestion of "isTracked" instead ( active suggests that something is currently be computed, or is not disabled, neither of which is necessarily true when ContextTracker tracks that a plug is a dependency of the specified target ).
The one other thought I had overall is that it's perhaps a little bit weird to assume by default that every output of a node depends on every input - when we have the affects() method that would allow us to be more specific. But there's no reason to be more specific if it wouldn't have any benefits to users. I can imagine a hypothetical node that processes independent streams ( output0 depends only on input0, output1 depends only on input1, etc ), where calling affects could result in a much more accurate tracking ... but I don't think we currently have any nodes like that, and I can't think of any good reason to make any, so the current heuristic is probably fine.
| } | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I had to double check that it correctly matches the behaviour of Loop that indexVariablePlug is evaluated in a different context on every iteration ... but of course, this is correct, because we can't get rid of the index in the current context before we pull the plug to find out what the index variable is named. So, this is correct, and the behaviour of Loop is probably not really a problem unless someone does something very weird - though using a complex expression to drive the name of the index variable could be a really bad performance hazard ( and making the name of the index variable actually depend on the current index would just be incredibly weird ).
| @@ -316,7 +448,8 @@ void ContextTracker::update() | |||
| continue; | |||
| } | |||
|
|
|||
| Context::Scope scopedContext( context.get() ); | |||
| Context::EditableScope scopedContext( context.get() ); | |||
| scopedContext.setCanceller( canceller ); | |||
There was a problem hiding this comment.
It took me a bit of staring to figure out why it was necessary to do this ( and then copy contexts with omitCanceller = true ) - but I think I get it now: we don't want to store this canceller, but we do want the getValue() calls for enabledPlug and selectorPlug to have the proper canceller set. Might be worth a comment?
There was a problem hiding this comment.
Yep, you have it right. Added a comment in ea214b8.
That's an interesting idea - could possibly apply to things like
I'm not sure how much I liked my suggestion! Let's get a tie-breaker from Murray and Eric in our catch up today. |
johnhaddon
force-pushed
the
contextTrackerPR1
branch
from
e848f33
to
ea214b8
Compare
|
Closing in favour of a new PR to bring a limited version of this functionality to 1.4.x. Will open another PR or two with more 1.5-specific functionality once that is merged. |
This adds a new ContextTracker class derived from the GraphGadget's "active state tracking", with the intention of eventually making the entire UI "focus aware". The primary differences to the GraphGadget's tracking are :
In this initial PR I've adapted a few key UI components to use this new functionality :
Hopefully these demonstrate the value in the ContextTracker and validate its API. Before 1.5 is released we'll need to update the rest of the UI to match, so that "focus aware" becomes the norm. I had originally planned to include a PlugValueWidget update as part of this PR but investigation shows it to be closely related to Editor and View, so I'll handle those all together in a separate PR.
I've requested review specifically from Daniel and Murray, but anyone is of course free to weigh in - there's quite a lot going on here, so there's a high likelihood I've messed something up somewhere. @danieldresser-ie, it might make most sense for you to focus your attention on the three ContextTracker commits while @murraystevenson gives a closer look to the UI integrations?