Skip to content

Improve tab restoration behavior when opening a solution#91

Open
magasser wants to merge 7 commits intoMattParkerDev:mainfrom
magasser:code-edit/avoid-unecessary-tab-changes
Open

Improve tab restoration behavior when opening a solution#91
magasser wants to merge 7 commits intoMattParkerDev:mainfrom
magasser:code-edit/avoid-unecessary-tab-changes

Conversation

@magasser
Copy link
Copy Markdown
Contributor

Description

This PR improves the experience when opening a solution and restoring previously opened tabs.

Overview

Previously, opening a solution would reopen and sequentially select every previously opened tab. With many tabs, this resulted in a poor user experience.

This update changes the behavior so that all tabs are restored simultaneously, and only the previously active tab is selected. This provides a better experience when reopening a solution.

Old Behavior

SharpIDE_OldSolutionOpen

New Behavior

SharpIDE_NewSolutionOpen

I agree to the terms of contributing as stated here

@MattParkerDev
Copy link
Copy Markdown
Owner

I agree the current behaviour is not ideal.
Not the hugest fan of the amount of time where seemingly nothing is happening from the user's perspective, before all of the tabs appear at once. Maybe a loading spinner over the CodeEditorPanel and perhaps SolutionExplorerPanel, worst case?

I'm hoping we can just speed up how long it takes to open all the tabs instead, so that the panel is blank for much less time? My guess is the expensive part is creating all of the SharpIdeCodeEditContainer nodes, to be placed as children of the TabContainer?

@magasser
Copy link
Copy Markdown
Contributor Author

I agree the current behaviour is not ideal. Not the hugest fan of the amount of time where seemingly nothing is happening from the user's perspective, before all of the tabs appear at once. Maybe a loading spinner over the CodeEditorPanel and perhaps SolutionExplorerPanel, worst case?

I'm hoping we can just speed up how long it takes to open all the tabs instead, so that the panel is blank for much less time? My guess is the expensive part is creating all of the SharpIdeCodeEditContainer nodes, to be placed as children of the TabContainer?

The long blank code editor is indeed not ideal. Adding a loading spinner is probably a good idea.

I took a quick look at where the time is spent, and the main bottleneck seems to be, as you predicted, the calls to PackedScene.Instantiate<SharpIdeCodeEditContainer>(). They take up ~80% of the total time.

With ~140 tabs, instantiating all containers currently takes about 7 seconds.

I see two potential approaches to improve this:

  1. Removing some nodes from SharpIdeCodeEditContainer to speed up instantiation (e.g., the two windows which could be created only when needed) which improves the time by ~60–70%
  2. Instantiating containers in parallel by adding files.AsParallel().Select(...) improves the time by ~70–80%, though this depends on the hardware the IDE is running on. But this might be something we only want to do if there are many tabs to create/add.

Do you have any other ideas for speeding this up?

@magasser
Copy link
Copy Markdown
Contributor Author

I added a basic loading spinner for now, let me know if you prefer a different design:

SharpIDE_LoadingSpinner

@MattParkerDev
Copy link
Copy Markdown
Owner

I committed 1ca286c which feels like it speeds up the file tab loading (without actually timing it). If you could merge in main, and lmk whether you agree that it seems improved.

Removing some nodes from SharpIdeCodeEditContainer to speed up instantiation (e.g., the two windows which could be created only when needed) which improves the time by ~60–70%

I like this idea. Doesn't need to be done in this PR, and I might take a look at it myself tomorrow.

Instantiating containers in parallel by adding files.AsParallel().Select(...) improves the time by ~70–80%, though this depends on the hardware the IDE is running on. But this might be something we only want to do if there are many tabs to create/add.

Not a huge fan.

Spinner looks good functionally, not totally sold on its design though 😅 Maybe smaller, and/or less opacity? My other thought is the full width blue loading bar at the top, like what VSCode has, when you e.g. Go To Definition on a Method requiring decompilation. Or even just text in the center, not too white, Loading previously opened files.... I think it is best to not draw too much attention to it, since for most people they will only have like 10 tabs open and will load quickly; IMO it wouldn't look great in that scenario where it quickly appears and disappears.

I also think we should consider a max open tabs limit - Rider's is 100 by default. Would start closing tabs on the left if adding a new one would go over the limit.

@magasser
Copy link
Copy Markdown
Contributor Author

I committed 1ca286c which feels like it speeds up the file tab loading (without actually timing it). If you could merge in main, and lmk whether you agree that it seems improved.

I tried it out but I did not really notice any improvement.

Removing some nodes from SharpIdeCodeEditContainer to speed up instantiation (e.g., the two windows which could be created only when needed) which improves the time by ~60–70%

I like this idea. Doesn't need to be done in this PR, and I might take a look at it myself tomorrow.

Regarding this, I experimented with a complete lazy-loading solution by creating an empty tab scene and only instantiating the SharpIdeCodeEditContainer when it is accessed which allows the tabs to be created and added almost instantly:

public sealed partial class SharpIdeCodeEditTab : Control
{
	public SharpIdeSolutionModel Solution { get; set; } = null!;
	public SharpIdeFile File { get; set; } = null!;

	private static readonly PackedScene SharpIdeCodeEditScene =
		ResourceLoader.Load<PackedScene>("res://Features/CodeEditor/SharpIdeCodeEdit.tscn");

	private SharpIdeCodeEditContainer? _container;

	public SharpIdeCodeEdit CodeEdit
	{
		get
		{
			if (_container is not null) return _container.CodeEdit;

			_container = SharpIdeCodeEditScene.Instantiate<SharpIdeCodeEditContainer>();
			_container.CodeEdit.Solution = Solution;
			Dispatcher.SynchronizationContext.Send(
				static void (state) =>
				{
					var (tab, container) = ((SharpIdeCodeEditTab, SharpIdeCodeEditContainer))state!;
					tab.AddChild(container);
				}, (Tab: this, Container: _container));
			
			_ = _container.CodeEdit.SetSharpIdeFile(File, fileLinePosition: null);

			return _container.CodeEdit;
		}
	}
}

It is by no means a clean solution at this point as I wanted to make as little changes as possible and there still seem to be a few issues but it could be worth investigating further. I have a PoC implementation on a branch if you’re interested: code-edit/lazy-load-tabs.

Spinner looks good functionally, not totally sold on its design though 😅 Maybe smaller, and/or less opacity? My other thought is the full width blue loading bar at the top, like what VSCode has, when you e.g. Go To Definition on a Method requiring decompilation. Or even just text in the center, not too white, Loading previously opened files.... I think it is best to not draw too much attention to it, since for most people they will only have like 10 tabs open and will load quickly; IMO it wouldn't look great in that scenario where it quickly appears and disappears.

I will have a look to implement some other designs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants