-
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
Improve Preference Filtering #8263
Improve Preference Filtering #8263
Conversation
e06345d
to
afaa91d
Compare
@kenneth-marut-work, would you be willing to take a look at this? |
afaa91d
to
a4858e2
Compare
a4858e2
to
de20117
Compare
In the third set of steps, I can confirm that typing the name of a preference ( |
packages/preferences/src/browser/views/preference-widget-bindings.ts
Outdated
Show resolved
Hide resolved
de20117
to
adea194
Compare
Thanks for the suggestions, @kenneth-marut-work. I have adjusted the editor to scroll to top when the text filter is cleared, as you indicated, and cleaned up the code you commented on. |
adea194
to
c50c9c2
Compare
packages/preferences/src/browser/views/preference-editor-widget.tsx
Outdated
Show resolved
Hide resolved
packages/preferences/src/browser/views/components/single-preference-wrapper.tsx
Show resolved
Hide resolved
b91918d
to
b9a073b
Compare
protected lastSearchedLiteral: string = ''; | ||
protected _currentScope: number = Number(Preference.DEFAULT_SCOPE.scope); | ||
protected _isFiltered: boolean = false; | ||
protected _currentRows: Map<string, PreferenceTreeNodeRow> = new Map(); |
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.
The tree model should not had such representation. Why do you need it here? Flat rows should be computed in the widget based on nodes' attributes like visible
.
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.
Because this tree - and the rules about what should and shouldn't be displayed - are common to the PreferenceTreeWidget
and PreferenceEditorWidget
it's more convenient and more efficient to simply share that data in a location accessible to both widgets. Formerly, a separate service generated a tree that was consumed by both widgets, but that required that the downstream widgets implement logic determining visibility separately, even though the requirements are (nearly) identical.
If your concern is with the checks made elsewhere in the standard TreeModel
implementation that use TreeNode.isVisible
to determine whether to select a node, the function that generates the rows here can be altered to modify the .visible
field on each node appropriately.
b9a073b
to
7f66a9f
Compare
7f66a9f
to
43d81e7
Compare
43d81e7
to
ef73ddc
Compare
@vince-fugnitto, @marechal-p, I've brought this up to date with |
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.
@colin-grant-work I verified the changes with the provided use-cases and it works as described 👍
- preferences are generally much more responsive and seamless
- switching between preference scopes works well (user, workspace and folder)
- searching is more responsive, switching scopes with a search term present is also quick
packages/preferences/src/browser/util/preference-tree-generator.ts
Outdated
Show resolved
Hide resolved
ef73ddc
to
20096d0
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.
LGTM, although I wasn't able to notice a difference with performance. Is there something to specifically do or is my computer just fast enough to make the diff negligible?
Yeah, on fast machines it isn't really noticeable - but the bugfix for workspace-scoped vs. user-scoped preferences still applies. The main difference is that in the old code, the entire tree was reconstructed for any update, and now it's just being filtered in a style similar to the way nodes are hidden or shown on expansion. |
Signed-off-by: Colin Grant <colin.grant@ericsson.com>
20096d0
to
738b72a
Compare
Signed-off-by: Colin Grant colin.grant@ericsson.com
What it does
This PR aims to make the filtering of nodes in the preference widget more performant, and to smooth the way for feature enhancements like #8252.
At the moment, filtering and scope changing actually modify the
root
of the TreeModel, rather than maintaining the same root and conditionally rendering tree nodes based on various criteria. The performance fix in #7936 improves the situation, but at the cost of some flexibility in scoping preferences. This PR moves that functionality into a modifiedTreeModel
implementation and uses an iterative strategy parallel to thedoUpdateRows
function in theTreeWidget
to determine which nodes to display. This will make filtering more performant as well as more flexible in response to changing demands.In addition, this PR refactors away some of the not-well-encapsulated code from #7105 by eliminating the
PreferenceEventService
and locating emitters and state tracking in the classes logically responsible for them.Also fixes #8689
How to test
It should retain the functionality of #7105, #7773, #7764, and #7936.
In addition:
css.lint
is at the top of the visible window.Further:
passesCurrentFilters()
method of thePreferenceTreeModel
. This will mean that implementing enhancements like [Preference]: Add show modified settings menu-item #8252 is straightforward and performant.Review checklist
Reminder for reviewers