Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement preview on highlighting quickpick in quick search #202306

Merged
merged 9 commits into from
Jan 18, 2024

Conversation

andreamah
Copy link
Contributor

@andreamah andreamah commented Jan 12, 2024

Fixes #191259

This implements a rough version of the preview on active picker selection.

2024-01-12.10-11-40.mp4

This has:

  • Previewing on active selection
  • Restoring state on escape

TODO:

  • Try to get a 'true preview' editor state where we open a preview editor regardless of user settings.
  • Omit editor navigation from back/forward history.

cc: @TylerLeonhardt @bpasero

@andreamah andreamah self-assigned this Jan 12, 2024
@andreamah andreamah marked this pull request as ready for review January 12, 2024 18:15
@VSCodeTriageBot VSCodeTriageBot added this to the December / January 2024 milestone Jan 12, 2024
@bpasero
Copy link
Member

bpasero commented Jan 16, 2024

Saw this stack when testing:

ERR Cannot read properties of undefined (reading 'match'): TypeError: Cannot read properties of undefined (reading 'match')
    at UniqueContainer.value (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/workbench/contrib/search/browser/quickTextSearch/textSearchQuickAccess.js:69:26)
    at Emitter._deliver (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:889:26)
    at Emitter.fire (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:918:22)
    at UniqueContainer.value (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/platform/quickinput/browser/quickInput.js:650:51)
    at Emitter._deliver (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:889:26)
    at Emitter._deliverQueue (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:900:22)
    at Emitter.fire (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:923:22)
    at UniqueContainer.value (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:98:93)
    at Emitter._deliver (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:889:26)
    at Emitter._deliverQueue (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:900:22)
    at Emitter.fire (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:923:22)
    at vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:98:93
    at vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:1222:52
    at vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:1235:37
    at Array.forEach (<anonymous>)
    at EventBufferer.bufferEvents (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:1235:20)
    at WorkbenchList.splice (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/browser/ui/list/listWidget.js:1187:32)
    at QuickInputList.setElements (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/platform/quickinput/browser/quickInputList.js:511:23)
    at QuickPick.update (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/platform/quickinput/browser/quickInput.js:790:30)
    at set items [as items] (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/platform/quickinput/browser/quickInput.js:402:18)
    at applyPicks (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/platform/quickinput/browser/pickerQuickAccess.js:89:34)
    at vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/platform/quickinput/browser/pickerQuickAccess.js:112:52
    at applyFastAndSlowPicks (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/platform/quickinput/browser/pickerQuickAccess.js:114:27)
    at updatePickerItems (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/platform/quickinput/browser/pickerQuickAccess.js:178:27)
    at UniqueContainer.value (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/platform/quickinput/browser/pickerQuickAccess.js:206:59)
    at Emitter._deliver (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:889:26)
    at Emitter._deliverQueue (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:900:22)
    at Emitter.fire (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:923:22)
    at QuickPick.doSetValue (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/platform/quickinput/browser/quickInput.js:373:46)
    at UniqueContainer.value (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/platform/quickinput/browser/quickInput.js:558:26)
    at Emitter._deliver (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:889:26)
    at Emitter._deliverQueue (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:900:22)
    at Emitter.fire (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/common/event.js:923:22)
    at HistoryInputBox.onValueChange (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/browser/ui/inputbox/inputBox.js:354:31)
    at HTMLInputElement.<anonymous> (vscode-file://vscode-app/Users/bpasero/Development/Microsoft/vscode/out/vs/base/browser/ui/inputbox/inputBox.js:93:49)

@andreamah
Copy link
Contributor Author

Fixed! ^

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am giving +1 on a high level and defer to @TylerLeonhardt for a deeper review. I suggest to tackle follow up things such as preview mode and history blocking on demand based on further feedback.

@andreamah
Copy link
Contributor Author

I can probably add options when opening the editor to omit history and force preview, then figure out how to implement them on the editor side. Does that sound like the correct way to go?

@bpasero
Copy link
Member

bpasero commented Jan 17, 2024

Yeah, given preview is on by default I think we can probably ship this for stable and then gather feedback based on users and then think about the implementation side of things in Feb?

@bpasero
Copy link
Member

bpasero commented Jan 17, 2024

To be fair, I am not even sure if people might expect history to work or not. If you slowly navigate over quick search results, maybe its expected that history allows you to navigate back? Thats why I am suggesting to wait for real user feedback!

@andreamah
Copy link
Contributor Author

Talked to Tyler offline and he's ok with me merging for now and he'll review it after

@andreamah andreamah merged commit 404f9d1 into main Jan 18, 2024
6 checks passed
@andreamah andreamah deleted the andreamah/issue191259 branch January 18, 2024 19:42
This was referenced Jan 23, 2024
Ros-Cos

This comment was marked as spam.

@Ros-Cos Ros-Cos mentioned this pull request Jan 28, 2024
1 task
@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement preview on highlighting quickpick in quick search
5 participants