Preferences - Responsive Scope Changes

[colin-grant-work]

Signed-off-by: Colin Grant [email protected]

What it does

Fixes #7887 by limiting the regeneration of the tree to cases when the visibility of elements is affected (search, preference schema change, or scope change from User/Workspace <---> Folder). If tree is not regenerated, only displayed values are changed. If tree is regenerated, a check is made to see whether the formerly selected element is still visible, and if it is, it is scrolled into view.

In reviewing some of the widget code, I've also added a bit of handling for errors that shouldn't occur, but could conceivably occur.

How to test

1. Open preferences in User scope
2. Select some element in the ToC
3. Make a preference change in the visible field.
4. Change to Workspace scope.
5. Observe that the value is updated, but that the refresh is nearly instantaneous.
6. Open a multi-root workspace, open preferences, and select a folder scope.
7. Do 2-5
8. Observe that after the scope change, a moment passes and then the tree is scrolled to the appropriate item.
9. Do the reverse (Workspace -> folder scope) and observe that, if the item is available in folder scope, it is scrolled to. Otherwise, the editor scrolls to top.

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

[vince-fugnitto]

@colin-grant-work I tested the behavior described from #7887 and it works correctly 👍

One improvement I noticed (possibly in a separate pull-request) is to also handle when switching between an editor and the preferences widget. For example:

1. open the preferences-widget and scroll down to a preference value (ex: output.maxChannelHistory)
2. open an editor (ex: readme.md)
3. navigate back to the preferences-widget
4. notice how the preferences-widget is reset (does not preserve the location) and is re-rendered back to the top

[lmcbout]

Works as expected
Thanks @colin-grant-work

[colin-grant-work]

@vince-fugnitto, I had hoped that this would do better at addressing the concerns you mention and move towards fixing #7746, but it seems the mechanisms are different. As I understand it (which may well be quite wrong 😄), one of two things is happening: (A) when the user leaves the preferences widget and returns, a signal is being sent that triggers a complete rerender. That should be easy to fix, since it would just mean changing how the widget responds to that particular signal. Alternatively, (B) when the user switches back and forth, the widget is temporarily deactivated, and in order to keep its state on its next activation, it would need to implement the logic to turn it into a stateful widget (which is probably desirable anyway). I think window switching is A, and closing and opening the preferences widget would be B - is that correct?

[vince-fugnitto]

...it would need to implement the logic to turn it into a stateful widget...

@colin-grant-work as a reference, I have already tracked an issue on improving the statefulness of the preferences-widget (#7746) which will help in restoring the widget upon reloading the application. Implementing the stateful interface for a widget is generally used for reloading purposes since the state is stored and retrieved from the browser's local-storage. In this particular case I'm thinking that the stateful interface will not help as this seems to be a bug when the application is already reloaded and we are switching active widgets. For this reason I believe the preferences-widget might be re-rendering prematurely or is listening to an incorrect event.

The pull-request is however already an improvement and we could tackle this issue separately if you wish.

[colin-grant-work]

So it sounds like the current problem is in what I described as scenario (A), where the widget's state doesn't need to be saved anywhere, it just needs to respond better to incoming events? If that's the case, I think it's worth taking a stab at as part of this PR, because it shouldn't be too much extra to change a listener - I'll give it a try later today.

[vince-fugnitto]

So it sounds like the current problem is in what I described as scenario (A), where the widget's state doesn't need to be saved anywhere, it just needs to respond better to incoming events? If that's the case, I think it's worth taking a stab at as part of this PR, because it shouldn't be too much extra to change a listener - I'll take a stab at it later today.

Sounds good! :) Please ping me whenever you're ready for a re-review.

[colin-grant-work]

@vince-fugnitto , it took me a couple of days to circle back around to this. I've done some looking around, and it seems that the behavior is inconsistent. In the browser, just changing windows doesn't cause a scroll to top, but in Electron it does.

Here's the browser during a show-hide sequence:

And here's Electron with the same code:

You can see that somewhere between onBeforeShow and onAfterShow, the browser has restored the scroll state, but Electron has not. I'm not really sure why, but I don't think it has to do with the particular methods implemented on these widgets. If you have any suggestions, I'm happy to follow it up, but I think I'm inclined to let it me a matter for another PR.

Apart from the behavior on hide and show, they both show somewhat odd behavior onAfterAttach. In both, doing something that fires onAfterAttach will cause a scroll to top, but without actually triggering a scroll event. I suspect that that behavior has to do with the way the default Widget implementation of onAfterAttach changes scroll behavior:

theia/packages/core/src/browser/widgets/widget.ts

Lines 90 to 109 in 193c85f

	protected onAfterAttach(msg: Message): void {
	super.onAfterAttach(msg);
	if (this.scrollOptions) {
	(async () => {
	const container = await this.getScrollContainer();
	container.style.overflow = 'hidden';
	this.scrollBar = new PerfectScrollbar(container, this.scrollOptions);
	this.disableScrollBarFocus(container);
	this.toDisposeOnDetach.push(addEventListener(container, <any>'ps-y-reach-end', () => { this.onScrollYReachEndEmitter.fire(undefined); }));
	this.toDisposeOnDetach.push(addEventListener(container, <any>'ps-scroll-up', () => { this.onScrollUpEmitter.fire(undefined); }));
	this.toDisposeOnDetach.push(Disposable.create(() => {
	if (this.scrollBar) {
	this.scrollBar.destroy();
	this.scrollBar = undefined;
	}
	container.style.overflow = 'initial';
	}));
	})();
	}
	}

[vince-fugnitto]

@colin-grant-work thank you for taking the time to debug and investigate what is happening. Based on you findings I think the pull-request should be merged and we can fix the issue mentioned in a separate pr.

If possible, can you open an issue so we can track the buggy behaviour?

[tsmaeder]

@vince-fugnitto what are we waiting for to merge this?

[vince-fugnitto]

@vince-fugnitto what are we waiting for to merge this?

Thank you for reminding me, I'll merge shortly :)