ui: add API for command filtering for embedders#154
Conversation
ui/src/base/registry.ts
Outdated
| override set filter(filter: ((key: string) => boolean) | undefined) { | ||
| this.parent.filter = filter; | ||
| super.filter = filter; | ||
| } |
There was a problem hiding this comment.
For our use case, this makes sense, but I'm not sure that propagating the setting up to the parent (without the parent then propagating it back down to all children?) would make sense as a general API. In practice, do we need this behavior, i.e. do we end up interacting with a child such that we need to make sure that when we set the filter on that child, it makes its way back up to the root Registry?
There was a problem hiding this comment.
We do need this behaviour. Most of the commands that are registered on the parent (AppImpl-level, not open-Trace-level) are inapplicable and/or dangerous in Sokatoa.
We could require an application to separately set a filter on the parent if you prefer that. We certainly have access to it.
There was a problem hiding this comment.
If we have direct access to the registries where we need set filters, then I think removing child -> parent propagation would assuage any concerns I have. I think it's the inconsistency between child construction time (propagation parent -> child) and post-construction time (propagation child -> parent) that makes me a little queasy.
There was a problem hiding this comment.
Agreed. It'll be cleaner this way.
ui/src/base/registry.ts
Outdated
| set filter(filter: ((key: string) => boolean) | undefined) { | ||
| this.keyFilter = filter ?? (() => true); | ||
|
|
||
| // Run the filter to knock out anything already registered that does not pass it | ||
| [...this.registry.keys()] | ||
| .filter((key) => !this.keyFilter(key)) | ||
| .forEach((key) => this.registry.delete(key)); | ||
| } |
There was a problem hiding this comment.
This also seems a bit odd as API in the following scenario:
- Registrants register a variety of items.
- Someone sets a filter that allows only subset A through.
- Someone sets a filter that allows only subset B through.
- Now we end up with only the intersection of subsets A and B, but the filterer at step (3) might reasonably want to get everything in subset B that was ever registered, since the filter for subset A was never what they were interested in (or knew about, or could control, for whatever reason).
That makes me wonder whether it might make sense (and be possible, of course?) to limit what the registry exposes while retaining a reference to the original registrations? Something like
/** retains everything */
registry
/** target for retrieval: contains the subset of items matching current filter */
filteredRegistry
There was a problem hiding this comment.
The purpose is that the "someone" should be an application that embeds Perfetto and is the only authority for such filters. And so wouldn't be inconsistent about it. Nothing in this PR made that clear, of course.
Perhaps this should be a write-once property. Attempts to change the filter after it has been set should throw an error. And then it should be a setFilter() method and not a set filter prop.
There was a problem hiding this comment.
Set once behavior would make the expectations explicit 👍
ui/src/base/registry_unittest.ts
Outdated
| test('setting filter on child cascades to parent and prunes both registries', () => { | ||
| const parent = Registry.kindRegistry<Registrant>(); | ||
| const a: Registrant = {kind: 'a', n: 1}; | ||
| const b: Registrant = {kind: 'b', n: 2}; | ||
| parent.register(a); | ||
| parent.register(b); | ||
|
|
||
| const child = parent.createChild(); |
There was a problem hiding this comment.
The scenario of interest here would be something like
const parent = Registry.kindRegistry<Registrant>();
const childA = parent.createChild();
const childB = parent.createChild();
childA.filter = (candidate) => isCoolEnough(candidate);
// Should childB be affected, given that the parent is?There was a problem hiding this comment.
It depends on what you mean by "affected."
Assuming that filtering is not inherited, that filters are separately installed at every level of registry delegation, then:
A child should only inherit from a parent the registrations that the parent itself publishes to the world. But the child can override those registrations.
So, if the parent filters out an uncool registration, then the child should not see it. But if the child does not have the cool filter, then the child should be able to add its own registration of an uncool entry.
Assuming that filtering is inherited, then
As always, a child cannot delegate the look-up of an uncool entry to the parent if the parent has a cool filter.
Moreover, a child should not be able to register an uncool entry if the parent has a cool filter.
In some applications that embed the Perfetto UI, there are a number of commands that are not needed. For example, in an application that manages the opening of traces into instances of the Perfetto UI, there is no need for the commands that open and close traces. And other commands may overlap or conflict with other capabilities provided by the host application, which therefore may wish to exclude those commands. Add an API to the Registry class to permit an embedding application to install a filter that quietly prevents registration of unwanted services. Employ this capability in the CommandManager. Signed-off-by: Christian W. Damus <cdamus.ext@eclipsesource.com>
- don't push child filter to parent - dynamically inherit parent filter to child in addition to child filter - make the filter write-once and be explicit about its purpose for embedder use cases Signed-off-by: Christian W. Damus <cdamus.ext@eclipsesource.com>
cdamus
force-pushed
the
feature/ui-command-filtering
branch
from
13c37ce to
848a0a3
Compare
|
Thanks for the review @colin-grant-work . Commit 848a0a3 should address your comments. |
colin-grant-work
left a comment
colin-grant-work
left a comment
There was a problem hiding this comment.
Thanks for the changes. LGTM.
2 participants
In some applications that embed the Perfetto UI, there are a number of commands that are not needed. For example, in an application that manages the opening of traces into instances of the Perfetto UI, there is no need for the commands that open and close traces. And other commands may overlap or conflict with other capabilities provided by the host application, which therefore may wish to exclude those commands.
Add an API to the Registry class to permit an embedding application to install a filter that quietly prevents registration of unwanted services. Employ this capability in the CommandManager.
For android-graphics/sokatoa#3871