Tidy state view #1086
Tidy state view #1086
Conversation
ben-clayton
force-pushed
the
tidy-state
branch
2 times, most recently
from
dedf4aa to
80071ff
Compare
| @@ -531,7 +531,7 @@ public void setHoveredItem(TreeItem hoveredItem, Follower.Prefetcher<Void> follo | |||
| @Override | |||
| protected boolean isFollowable(Object element) { | |||
| ApiState.Node node = (ApiState.Node)element; | |||
| return node.getData() != null && node.getData().hasValuePath(); | |||
| return node != null && node.getData() != null && node.getData().hasValuePath(); | |||
There was a problem hiding this comment.
I don't like this change. element being null here violates a precondition and represents a bug that should be fixed rather than ignored.
| @@ -62,7 +62,7 @@ public ApiState( | |||
| atoms.addListener(new AtomStream.Listener() { | |||
| @Override | |||
| public void onAtomsSelected(AtomIndex index) { | |||
| load(stateTree(index), false); | |||
There was a problem hiding this comment.
Undo - this is specifically written to handle null selection, which mean deselection.
| @@ -77,15 +78,10 @@ private Paths() { | |||
| return Path.State.newBuilder().setAfter(command).build(); | |||
| } | |||
|
|
|||
| public static Path.Any stateTree(AtomIndex atom) { | |||
There was a problem hiding this comment.
Undo - This is part of handling null selections.
| return null; | ||
| } | ||
|
|
||
| public static Path.Any parentOf(Path.Any path) { |
There was a problem hiding this comment.
This is incomplete and I don't think it's necessary. This is trying to future-proof something without actually making it future-proof. Please undo this and just add the global state case.
There was a problem hiding this comment.
This is trying to future-proof something without actually making it future-proof
I'd say it is more than just future-proofing. You had an ad-hoc collection of find functions that would need to be duplicated for findGlobalState. Yes, what I have changed to is incomplete, but it's as incomplete as your old findState. I'd rather implement the full set than roll back to a bunch of random functions. This approach mirrors the go-side, where parent-traversal is used for numerous cases.
There was a problem hiding this comment.
but it's as incomplete as your old
findState
I disagree with that assertion. findState was incomplete in the sense that the state path was the only path it could find, but it correctly handled every kind of path. parentOf pretends to be able to find the parent of any path, but it does not.
I know this is how it's done in go, and it works OK there due to the implicit interfaces. However, in Java the protos are immutable and the constant toBuilder().build() to wrap it in Path.Any for it to then just be unwrapped is a lot of overhead for something that is not complete and only used to determine if a path has a state root.
I propose, for now, we simply change findState to isStatePath, returning a boolean, since that's all that it's actually used for. This makes the change to handle the new global state root very simple. We then look at how we can do the parent relationship better. I would prefer if it was part of the proto.
|
|
||
| atoms.addListener(new AtomStream.Listener() { | ||
| @Override | ||
| public void onAtomsSelected(AtomIndex index) { | ||
| load(Paths.any(stateTree(index.getCommand())), false); | ||
| Path.StateTree path = stateTree(index.getCommand()); |
There was a problem hiding this comment.
Instead of building the proto twice, simply pass the context into the stateTree method (see the commandTree method)
gapis/service/path/path.proto
Outdated
| @@ -369,6 +369,8 @@ message Slice { | |||
| // State is a path to a subset of the GlobalState at a point in a capture. | |||
| message State { | |||
| Command after = 1; | |||
| // If non-nil, then the state is filtered to the specified context. | |||
| ID context = 2; | |||
There was a problem hiding this comment.
Won't this break links to objects in another context which is shared?
ctx1 = newContext(nil)
ctx1...[1] = newTexture()
ctx2 = newContext(ctx1)
ctx2.bindTexture(1)
If in the UI I am filtering to ctx2, the link will attempt to go to ctx1, which is now no longer part of the tree, right?
There was a problem hiding this comment.
No it won't - because the server will return absolute GlobalState paths for followed links. The state tree nodes will also use GlobalState paths so that everything is absolute.
There was a problem hiding this comment.
State paths are used as relative paths to the most relevant part of the GlobalState (as considered by GAPIS).
ben-clayton
force-pushed
the
tidy-state
branch
from
80071ff to
f816a40
Compare
Create a new interface for an API’s state (State)
Makes it possible to lookup the APIs from a path. Also: Declare ID as a global for each API.
Currently unused, but will be soon.
Also handle slices -> arrays.
The old approach didn’t scale for more complicated paths.
Name() uses fields that can vary over time.
Use the visitor pattern to implement: • toNode() - Takes a path.Any, returns an unboxed Node type. • toAny() - Inverse of toNode. • parentOf() - Returns the parent node of the given node. • setParent() - Returns a copy of the node with the parent changed. • toString() Long and verbose, but is explicit and does the job. Happy to try and replace portions with reflection magic - I just want something that works for now.
ben-clayton
force-pushed
the
tidy-state
branch
from
ef11f7f to
a96d586
Compare
|
Updated with a final CL that fixes the state tree reparenting. PTAL |
Fixes: #1068
Fixes: #24