Ensure correct node expansion with short trailing sections #15005

colin-grant-work · 2025-02-21T21:47:34Z

What it does

Fixes a bug where clicking on a preference at the end of the list with few items would result in the previous section appearing expanded in the list.

I also switched the default section for preferences to features. The older default extensions was mainly a workaround to avoid implementing a new, plugin-aware preference service in the plugin system. Since a method has been added specifically for registering preferences from plugins, it no longer makes sense as the default. I'm happy to discuss the implications of the change, because it could affect downstream applications and where users expect to find preferences.

NB: Only the second commit is the fix to the scroll bug. The first commit is optional, and motivated by the considerations above.

How to test

Start the application without many extensions (e.g. no user extensions, and move the development plugins folder out of the repo temporarily)
Open the preferences view and click on the bottom 'Extensions' category.
That section should remain expanded and highlighted, even though the first item appearing in the UI may not belong to it.

If you apply only the first commit from this PR and try the same thing, the previous section, AI Features, will be expanded (unless you're on a very, very short monitor).

Follow-ups

Breaking changes

This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

tsmaeder · 2025-02-25T08:41:35Z

packages/preferences/src/browser/views/preference-editor-widget.ts

+        if (!node) { return; }
+        this.shouldUpdateModelSelection = false;
+        node.scrollIntoView();
+        requestAnimationFrame(() => this.shouldUpdateModelSelection = true);


Two comments here: Why are we delaying this here? If it's for the code to execute after some promises we create in the meantime, that's not reliable because execution order of promises vs requestAnimationFrame() is not guarantueed.
Second: are we sure this code is not invoked before the Window is visible? Because otherwise, this might delay loading, see #13798

Re: delay, we do want to postpone at least a tick, because we want the false setting to be visible to an event handler that won't be running in the same task as this code, but no promises are involved, just scrollIntoView -> onScroll.

Re: the concerns raised in Theia app is not loading while minimized at startup #13798, this is actual on-screen behavior in that it relates to what's at the top of the viewport at a given time, but it's probable that setTimeout would do the job - I'll try that.

It appears that setTimeout does not reliably push the reset past the running of the handler. This particular requestAnimationFrame currently doesn't run at startup, as we don't restore model selection independently.

Do I understand that correctly that if we were to run this at startup, we would run into a problem or is it just delaying the visibility of the window?

martin-fleck-at

Overall the changes look good to me and seem to work well, thank you very much, Colin! I do have some open questions but they may turn out to be moot.

martin-fleck-at · 2025-02-26T16:16:36Z

packages/preferences/src/browser/util/preference-tree-generator.ts

@@ -148,7 +148,7 @@ export class PreferenceTreeGenerator {
        if (!property.owner) {
            return;
        }
-        const groupID = this.defaultTopLevelCategory;
+        const groupID = 'extensions';


I'm not really sure what this change entails and if someone would ever change the defaultTopLevelCategory but if someone this, this change makes it a bit harder for them to get their previous behavior back. Would it at least make sense to add this as an optional argument with default value to the method signature, so that it can be more easily overwritten?

martin-fleck-at · 2025-02-26T16:21:26Z

packages/preferences/src/browser/util/preference-tree-generator.ts

@@ -43,7 +43,7 @@ export class PreferenceTreeGenerator {

    protected readonly onSchemaChangedEmitter = new Emitter<CompositeTreeNode>();
    readonly onSchemaChanged = this.onSchemaChangedEmitter.event;
-    protected readonly defaultTopLevelCategory = 'extensions';
+    protected readonly defaultTopLevelCategory = 'features';


I'm not really sure what impact this change has, could you elaborate just a bit more. As far as I can tell, it really is just a visual change that moves some of the preferences from 'Extension' to 'Features'. I'm not sure if this is more in line with what VS Code does but to me it makes sense. However, I want to make sure I'm not missing anything.

martin-fleck-at · 2025-02-26T16:22:29Z

packages/preferences/src/browser/views/preference-editor-widget.ts

+        if (!node) { return; }
+        this.shouldUpdateModelSelection = false;
+        node.scrollIntoView();
+        requestAnimationFrame(() => this.shouldUpdateModelSelection = true);


Do I understand that correctly that if we were to run this at startup, we would run into a problem or is it just delaying the visibility of the window?

colin-grant-work · 2025-02-26T17:27:37Z

@martin-fleck-at, thanks for your review.

Do I understand that correctly that if we were to run this at startup, we would run into a problem or is it just delaying the visibility of the window?

Just delaying the startup behavior, in this case, I think only of this widget, as nothing depends on the widget having scrolled (or not) anywhere in particular.

Re: your other comments, they have to do with the changes in the preference tree generator, which are part of the first commit and can be removed. I believe they're 'correct,' but they can be taken up separately from the fix for the scrolling issue.

martin-fleck-at

Alright, thank you very much, Colin! I'm happy with the changes then but would like to give @tsmaeder a chance to chime in as he raised the timing issues in the first place.

tsmaeder · 2025-02-27T09:59:12Z

@martin-fleck-at I just randomly came across this PR, saw the "requestAnimationFrame()" and given recent discussions wanted to makes sure we're not calling the code from our startup sequence (for aforementioned reasons).
As for the change in general: in the preferences widget, we're updating the selection upon changing scroll state and we're updating the scroll state based on the selection. That is tough to get right.
At the root. the problem is this: we cannot distinguish between changes due to user interaction and changes due to programmatic changes. The PR tries to "cut the loop" with a state variable. The asynchronous nature of DOM updates requires us to reset the variable "later". I believe the PR improves the behaviour and have no objections to merging it. However, similar problems with scroll tracking also plague our tree implementation and I firmly believe we'll have to find a clean solution for it.

JonasHelming requested a review from martin-fleck-at February 24, 2025 08:18

tsmaeder reviewed Feb 25, 2025

View reviewed changes

martin-fleck-at reviewed Feb 26, 2025

View reviewed changes

No model updates while handling model update

c5e3d7a

colin-grant-work force-pushed the bugfix/expand-penultimate-preference branch from 317461b to c5e3d7a Compare February 26, 2025 17:30

martin-fleck-at approved these changes Feb 27, 2025

View reviewed changes

colin-grant-work merged commit b950571 into master Feb 28, 2025
11 checks passed

github-actions bot added this to the 1.60.0 milestone Feb 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure correct node expansion with short trailing sections #15005

Ensure correct node expansion with short trailing sections #15005

colin-grant-work commented Feb 21, 2025

tsmaeder Feb 25, 2025

colin-grant-work Feb 25, 2025 •

edited

Loading

colin-grant-work Feb 25, 2025

martin-fleck-at Feb 26, 2025

martin-fleck-at left a comment

martin-fleck-at Feb 26, 2025

martin-fleck-at Feb 26, 2025

martin-fleck-at Feb 26, 2025

colin-grant-work commented Feb 26, 2025 •

edited

Loading

martin-fleck-at left a comment

tsmaeder commented Feb 27, 2025

Ensure correct node expansion with short trailing sections #15005

Ensure correct node expansion with short trailing sections #15005

Conversation

colin-grant-work commented Feb 21, 2025

What it does

How to test

Follow-ups

Breaking changes

Attribution

Review checklist

Reminder for reviewers

tsmaeder Feb 25, 2025

Choose a reason for hiding this comment

colin-grant-work Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

colin-grant-work Feb 25, 2025

Choose a reason for hiding this comment

martin-fleck-at Feb 26, 2025

Choose a reason for hiding this comment

martin-fleck-at left a comment

Choose a reason for hiding this comment

martin-fleck-at Feb 26, 2025

Choose a reason for hiding this comment

martin-fleck-at Feb 26, 2025

Choose a reason for hiding this comment

martin-fleck-at Feb 26, 2025

Choose a reason for hiding this comment

colin-grant-work commented Feb 26, 2025 • edited Loading

martin-fleck-at left a comment

Choose a reason for hiding this comment

tsmaeder commented Feb 27, 2025

colin-grant-work Feb 25, 2025 •

edited

Loading

colin-grant-work commented Feb 26, 2025 •

edited

Loading