-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
GH-7772: Fixed issue with filtered group options. #8013
Conversation
Signed-off-by: Akos Kitta <[email protected]>
If a `QuickOpenGroupItemOptions` is filtered out, we merge the group properties into the next non-filtered item. - Exposed the `options` from the `QuickOpenItem`. - Made `MonacoQuickOpenService#toOpenModel` extensiable. Closes #7772 Signed-off-by: Akos Kitta <[email protected]>
b15e80f
to
64972ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the sample and it works well.
I however noticed a bug with the quick file open
, it does not open and throws an error:
Uncaught (in promise) TypeError: handler.getOptions is not a function
at PrefixQuickOpenService.<anonymous> (prefix-quick-open-service.ts:185)
at step (prefix-quick-open-service.ts:15)
at Object.next (prefix-quick-open-service.ts:15)
at prefix-quick-open-service.ts:15
at new Promise (<anonymous>)
at ../../packages/core/lib/browser/quick-open/prefix-quick-open-service.js.__awaiter (prefix-quick-open-service.ts:15)
at PrefixQuickOpenService.../../packages/core/lib/browser/quick-open/prefix-quick-open-service.js.PrefixQuickOpenService.setCurrentHandler (prefix-quick-open-service.ts:162)
at PrefixQuickOpenService.../../packages/core/lib/browser/quick-open/prefix-quick-open-service.js.PrefixQuickOpenService.open (prefix-quick-open-service.ts:156)
at QuickFileOpenService.push.../../packages/file-search/lib/browser/quick-file-open.js.QuickFileOpenService.open (quick-file-open.ts:124)
at Object.execute (quick-file-open-contribution.ts:40)
Signed-off-by: Akos Kitta <[email protected]>
I see, thanks for the heads-up. 👍 It is unrelated to the PR itself and the sample will be dropped anyway but I have added a fix in f0e9394; it works now. I'm going to leave this PR open until tomorrow, then merge it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@@ -51,7 +51,7 @@ export interface QuickOpenGroupItemOptions extends QuickOpenItemOptions { | |||
export class QuickOpenItem<T extends QuickOpenItemOptions = QuickOpenItemOptions> { | |||
|
|||
constructor( | |||
protected options: T = {} as T | |||
readonly options: T = {} as T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not sure that exposing this is a good idea. Someone can subclass QuickOpenItem
and override methods to provide other values. We should rather call methods to collect computed values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not sure that exposing this is a good idea.
Why? Please explain.
Someone can subclass
QuickOpenItem
and override methods to provide other values.
Yes, someone can do that, but I needed the original options
do be able to create a "copy".
We should rather call methods to collect computed values.
I do not need computed values here.
Thank you all for the review. As I see the Windows CI has failed with a Git test error; I have opened a follow-up: #8023 I am going to drop the example and start a new build 🤞 |
let groupOptionsToMerge: QuickOpenGroupItemOptions | undefined = undefined; | ||
for (let item of items) { | ||
if (!(item instanceof QuickOpenGroupItem) && groupOptionsToMerge) { | ||
item = new QuickOpenGroupItem({ ...groupOptionsToMerge, ...item.options }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if item
is a subclass of QuickOpenItem
? Maybe we have QuickOpenGroupItem
which is implemented by delegating to QuickOpenItem
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand it. Do you have an example in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look for extends QuickOpenItem
, for instance here:
theia/packages/scm/src/browser/scm-quick-open-service.ts
Lines 73 to 100 in db8c807
class ScmQuickOpenItem<T> extends QuickOpenItem { | |
constructor( | |
public readonly ref: T, | |
protected readonly execute: (item: ScmQuickOpenItem<T>) => void, | |
private readonly toLabel: (item: ScmQuickOpenItem<T>) => string = () => `${ref}`, | |
private readonly toDescription: (item: ScmQuickOpenItem<T>) => string | undefined = () => undefined) { | |
super(); | |
} | |
run(mode: QuickOpenMode): boolean { | |
if (mode !== QuickOpenMode.OPEN) { | |
return false; | |
} | |
this.execute(this); | |
return true; | |
} | |
getLabel(): string { | |
return this.toLabel(this); | |
} | |
getDescription(): string | undefined { | |
return this.toDescription(this); | |
} | |
} |
Copying internal options won't provide proper label and description.
Closing in favor of #8024 |
@akosyakov, please check the other PR instead. Thank you! |
What it does
Fixes an issue with filtered group options. If a
QuickOpenGroupItemOptions
is filtered out, we merge the group properties into the next non-filtered item.options
from theQuickOpenItem
.MonacoQuickOpenService#toOpenModel
extensiable.Closes #7772
How to test
Use the sample, search for
A
(make sureB
is not among the search results), you can still see theRest
label and the border for the group.Review checklist
Reminder for reviewers