Remove Identity Provider in Quick Pick Tree #211399
Conversation
When QP moved from a list to a tree the identity provider concept behaved differently. Post-move, things were getting/staying selected that shouldn't have been. See the below issues. This is because the identity provider was, perhaps, working how you expect... treating certain items in the list as other items and thus causing them to be selected. But as I think about this more, quick pick always operates under the model that the references of those items are really the "id"... so this PR removes the identity provider. Still fixes #209842 Fixes #209848
TylerLeonhardt
force-pushed
the
tyler/relieved-wildebeest
branch
from
b690b7c
to
8da7e81
Compare
src/vs/base/common/event.ts
Outdated
| data.buffers.push(() => listener.call(thisArgs, data.items.reduce(reduce, initial))); | ||
| } else { | ||
| data.buffers.push(() => listener.call(thisArgs, data.items.reduce(reduce as (last: T | undefined, event: T) => T))); |
There was a problem hiding this comment.
The reduce operation will run for each listener when events get flushed. Any way we can just run it once and give the result to each listener instead?
| identityProvider: { | ||
| getId: element => { | ||
| // always prefer item over separator because if item is defined, it must be the main item type | ||
| const mainItem = element.item || element.separator; | ||
| if (mainItem === undefined) { | ||
| return ''; | ||
| } | ||
| // always prefer a defined id if one was specified and use "label + description + detail" as a fallback | ||
| if (mainItem.id !== undefined) { | ||
| return mainItem.id; | ||
| } | ||
| let id = `label:${mainItem.label}`; | ||
| id += `$$description:${mainItem.description}`; | ||
| if (mainItem.type !== 'separator') { | ||
| id += `$$detail:${mainItem.detail}`; | ||
| } | ||
| return id; | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I still find it strange that we remove the identity provider here. Is there no way to get proper ids for each element?
There was a problem hiding this comment.
I don't think this can be done with an identity provider. When identity is equal, the tree keeps the 'old' object. This is not the behavior we want for quick pick.
For example, let's say Element A has a label of foo, no description, no detail, but a QP item button of trash and we're gonna setChildren with an Element of B that has the same label, description, detail, but a button of plus. This identity provider above would keep the trash button, but we want the plus button.
In all cases, we want the tree to have the 'new' object, and it looks like that's not the behavior of the tree when using an Identity Provider, so I don't think we want to use an Identity Provider.
There was a problem hiding this comment.
This is only because you're defining identity to be a function of appearance. I understand why, since QP API is like that. But if the QP items were, say, files, then their identity would instead be a function of their URI. And their actions visibility would be a function of context.
All of this is OK, it will make QP not be as performance at it could (since all elements will always be trashed across setChildren calls instead of being reused) but I doubt it will make a perceptual difference.
🚢 Ship it!
When QP moved from a list to a tree the identity provider concept behaved differently. Post-move, things were getting/staying selected that shouldn't have been. See the below issues.
This is because the identity provider was, perhaps, working how you expect... treating certain items in the list as other items and thus causing them to be selected.
But as I think about this more, quick pick always operates under the model that the references of those items are really the "id"... so this PR removes the identity provider.
Still fixes #209842
Fixes #209848