Add sash option for widget resize #10441

kaiyue0329 · 2021-11-19T20:23:05Z

What it does

sash-option.mov

Apply focus to a widget handle when you hover over it
sash.hoverDelay controls the hover feedback delay in milliseconds
sash.size controls the feedback area size in pixels for the dragging area
Add preference to set workbench.sash.hoverDelay (value must be in the range of 0 to 2000, default is 0ms)
Add preference to set workbench.sash.size (value must be in the range of 1 to 20, default is 4px)
Move the utilities for creating and manipulating dynamic stylesheets from editor-decoration-style.ts to decoration-style.ts in core so that:
- each namespace now has its indepednent style sheet
- only one rule should exist for a given selector in the provided stylesheet
In the left panel, a sash should only appear when the following two conditions are met:
1. the active horizontal container is expanded
2. the container that is above/below the active horizontal container is also expanded

Scenario	Demo	Sash State
Active container expanded, Container below collapsed		Hidden
Active container expanded, Container below expanded		Shown*
Active container expanded, Container above collapsed		Hidden
Active container expanded, Container above expanded		Shown*
Active container collapsed	see first video below	Hidden
No active container, All containers collapsed	see second video below	Hidden

active-container-collapsed.mov

all-collapsed.mov

How to test

Start the application
Hover the cursor over the horizontal and vertical handles for different view containers in Theia and drag
(please refer to the screencast above for more details)
Search for 'workbench sash' in Preferences (File > Preferences > Open Settings (UI) )
Update the sash.hoverDelay and sash.size preferences to valid values
Repeat Step 2 and verify that the hover delay and feedback area size for the border/sash are updated successfull
Verify the sash behaviors for the horizontal containers in the left panel based on the info provided in the table above

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

Signed-off-by: Kaiyue Pan kaiyue.pan@ericsson.com

vince-fugnitto

I noticed that there is a flicker when resizing with the sash, something which is not present in vscode and can likely be improved upon:

theia:

sash-flicker.mp4

vscode:

sash-flicker-vscode.mp4

Also, I noticed on startup that the sash is quite small (I never updated the preference value), and then it becomes bigger later, perhaps something to try out for yourself.

packages/core/src/browser/common-frontend-contribution.ts

vince-fugnitto · 2021-11-22T13:45:41Z

packages/core/src/browser/core-preferences.ts

+        'workbench.sash.hoverDelay': {
+            type: 'number',
+            default: 1,
+            minimum: 1,
+            maximum: 2000,
+            description: nls.localizeByDefault('Controls the hover feedback delay in milliseconds of the dragging area in between views/editors.')
+        },
+        'workbench.sash.size': {
+            type: 'number',
+            default: 7,
+            minimum: 1,
+            maximum: 20,
+            description: nls.localizeByDefault('Controls the feedback area size in pixels of the dragging area in between views/editors. Set it to a larger value if needed.')
+        },


Any reason we do not want to use the same values and defaults as in vscode?

'workbench.sash.size': { type: 'number', default: isIOS ? 20 : 4, minimum: 1, maximum: 20, description: localize('sashSize', "Controls the feedback area size in pixels of the dragging area in between views/editors. Set it to a larger value if you feel it's hard to resize views using the mouse.") }, 'workbench.sash.hoverDelay': { type: 'number', default: 300, minimum: 0, maximum: 2000, description: localize('sashHoverDelay', "Controls the hover feedback delay in milliseconds of the dragging area in between views/editors.") },

@vince-fugnitto I have updated the values and defaults to align with the behavior present in vscode.

I kept the default value for sash.hoverDelay to be 0 because it would be a more straightforward solution for the resizing latency issue mentioned in #10416.

packages/core/src/browser/view-container.ts

packages/core/src/browser/style/view-container.css

kaiyue0329 · 2021-11-23T23:18:40Z

@vince-fugnitto Thanks for the feedback!

I noticed that there is a flicker when resizing with the sash, something which is not present in vscode and can likely be improved upon

I have pushed up changes that should remove the flicker when resizing the sash.

sash-option.mov

Also, I noticed on startup that the sash is quite small (I never updated the preference value), and then it becomes bigger later, perhaps something to try out for yourself.

After some testings, I noticed a bug with my previous change. When an editor widget is focused, the sash size becomes smaller by a certain ratio (compared to when the left panel is in focused). When the left panel gains focus again, the sash size increases to the value defined in the preference. This bug is fixed in the latest commit.

colin-grant-work

@kaiyue0329, for some reason GitHub isn't letting me reply to our existing conversation, but one way to manage a dynamic style element would be like this. That would give you a central location to update the value, rather than requiring you to iterate through all of the individual elements.

I also notice that in VSCode, the hover shows in between groups if you have multiple editor groups, but that doesn't appear to be the case with this code:

packages/core/src/browser/shell/application-shell.ts

packages/core/src/browser/view-container.ts

kaiyue0329 · 2021-11-24T21:56:35Z

@kaiyue0329, for some reason GitHub isn't letting me reply to our existing conversation, but one way to manage a dynamic style element would be like this. That would give you a central location to update the value, rather than requiring you to iterate through all of the individual elements.

I also notice that in VSCode, the hover shows in between groups if you have multiple editor groups, but that doesn't appear to be the case with this code:

Thank you @colin-grant-work! I have pushed changes that address your comments. I also updated the screencast in the PR description to include the procedure for testing sash for muliple edior groups.

msujew

It seems like the flickering Vince experienced on Chrome is fixed. However, it still exists on Firefox:

2021-11-25.12-44-42.mp4

kenneth-marut-work · 2021-11-29T16:07:45Z

@kaiyue0329 looking really nice so far. I noticed one small functionality issue:

In a VSCode view container, the sash does not appear if the viewcontainer is collapsed (or not-draggable). This is really more of an issue if the sash is set to its maximum size since it can actually occlude the mouse target for the expand/collapse

VSCode	This Branch

This also has the side effect of causing 2 sashes to appear if you attempt to click and drag while the view container is collapsed:

kaiyue0329 · 2021-12-02T16:18:00Z

It seems like the flickering Vince experienced on Chrome is fixed. However, it still exists on Firefox:

@msujew Thank you for the feedback. I have pushed up changes that should remove the flickering in Firefox.

kaiyue0329 · 2021-12-02T16:24:38Z

In a VSCode view container, the sash does not appear if the viewcontainer is collapsed (or not-draggable). This is really more of an issue if the sash is set to its maximum size since it can actually occlude the mouse target for the expand/collapse

This also has the side effect of causing 2 sashes to appear if you attempt to click and drag while the view container is collapsed:

@kenneth-marut-work Thank you for the feedback. I have made changes to the code so that a sash in the left panel only appears under certain conditions (please see the updated PR description for more details).

kaiyue0329 · 2021-12-02T16:27:15Z

@vince-fugnitto @colin-grant-work This PR is ready for another round of review.

colin-grant-work · 2021-12-02T17:35:45Z

@vince-fugnitto points out that there are already some reasonable utilities for creating and manipulating dynamic style sheets here. Would you be interested in taking that code and moving it to core (and perhaps refactoring a bit to make it more generic) where it can be used in the different different places that currently use their own custom solution to this problem?

kaiyue0329 · 2021-12-02T17:54:07Z

@vince-fugnitto points out that there are already some reasonable utilities for creating and manipulating dynamic style sheets here. Would you be interested in taking that code and moving it to core (and perhaps refactoring a bit to make it more generic) where it can be used in the different different places that currently use their own custom solution to this problem?

@colin-grant-work Yes, I will do that!

packages/core/src/browser/core-preferences.ts

packages/core/src/browser/decoration-style.ts

packages/core/src/browser/common-frontend-contribution.ts

packages/core/src/browser/decoration-style.ts

packages/editor/src/browser/decorations/editor-decoration-style.ts

packages/git/src/browser/blame/blame-decorator.ts

packages/core/src/browser/decoration-style.ts

kenneth-marut-work

@kaiyue0329 once you've addressed the remaining comments I'll give this another test to make sure the functionality is in place and there are no issues. I think the EditorDecorationStyle class & DecorationStyle namespace could benefit from some refactoring after this is merged since it's a bit hard to follow. But that's outside the scope of this PR.

packages/core/src/browser/view-container.ts

packages/core/src/browser/decoration-style.ts

packages/git/src/browser/blame/blame-decorator.ts

colin-grant-work · 2021-12-14T00:00:42Z

A couple of visual and functional oddities:
In Dark (Visual Studio) theme, the sash appears as a grey bar between ViewContainer parts when not hovered on:

It may be that the color variable that's being applied to the ::after element is not quite right.

(That's at 20 pixels)

It also appears that the Git Blame annotation styling is not correct:

Master:

Now:

Use the Git: Toggle Blame Annotations command to see these.

colin-grant-work

The sash functionality is working well in the contexts where it's expected to appear, and the problems with the git decorations and odd sash appearance have been resolved.

kenneth-marut-work

I've tested this again and confirm that it's working as expected and confirmed that the blame CSS regressions have been fixed as well as the theming issue

- Apply focus to a widget handle when you hover over it - 'sash.hoverDelay' preference controls the hover feedback delay in milliseconds - 'sash.size' preference controls the feedback area size in pixels for the dragging area - Refactor the code for creating and manipulating dynamic stylesheets Signed-off-by: Kaiyue Pan <kaiyue.pan@ericsson.com>

vince-fugnitto reviewed Nov 22, 2021

View reviewed changes

vince-fugnitto added core issues related to the core of the application shell issues related to the core shell ui/ux issues related to user interface / user experience labels Nov 22, 2021

colin-grant-work reviewed Nov 22, 2021

View reviewed changes

packages/core/src/browser/view-container.ts Outdated Show resolved Hide resolved

kenneth-marut-work reviewed Nov 22, 2021

View reviewed changes

packages/core/src/browser/view-container.ts Outdated Show resolved Hide resolved

packages/core/src/browser/style/view-container.css Outdated Show resolved Hide resolved

packages/core/src/browser/style/view-container.css Outdated Show resolved Hide resolved

colin-grant-work reviewed Nov 24, 2021

View reviewed changes

packages/core/src/browser/shell/application-shell.ts Outdated Show resolved Hide resolved

packages/core/src/browser/view-container.ts Outdated Show resolved Hide resolved

kaiyue0329 requested review from vince-fugnitto, kenneth-marut-work and colin-grant-work November 24, 2021 21:56

msujew reviewed Nov 25, 2021

View reviewed changes

msujew reviewed Dec 6, 2021

View reviewed changes

packages/core/src/browser/core-preferences.ts Outdated Show resolved Hide resolved

colin-grant-work reviewed Dec 9, 2021

View reviewed changes

packages/core/src/browser/decoration-style.ts Outdated Show resolved Hide resolved

packages/core/src/browser/decoration-style.ts Outdated Show resolved Hide resolved

kaiyue0329 marked this pull request as draft December 10, 2021 02:47

kaiyue0329 marked this pull request as ready for review December 10, 2021 15:16

colin-grant-work reviewed Dec 10, 2021

View reviewed changes

kenneth-marut-work reviewed Dec 10, 2021

View reviewed changes

packages/core/src/browser/view-container.ts Outdated Show resolved Hide resolved

colin-grant-work reviewed Dec 13, 2021

View reviewed changes

colin-grant-work approved these changes Dec 15, 2021

View reviewed changes

kenneth-marut-work approved these changes Dec 15, 2021

View reviewed changes

colin-grant-work merged commit 73346de into eclipse-theia:master Dec 15, 2021

vince-fugnitto added this to the 1.21.0 milestone Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sash option for widget resize #10441

Add sash option for widget resize #10441

kaiyue0329 commented Nov 19, 2021 •

edited

Loading

vince-fugnitto left a comment

vince-fugnitto Nov 22, 2021

kaiyue0329 Nov 23, 2021 •

edited

Loading

kaiyue0329 commented Nov 23, 2021 •

edited

Loading

colin-grant-work left a comment

kaiyue0329 commented Nov 24, 2021

msujew left a comment

kenneth-marut-work commented Nov 29, 2021

kaiyue0329 commented Dec 2, 2021 •

edited

Loading

kaiyue0329 commented Dec 2, 2021

kaiyue0329 commented Dec 2, 2021

colin-grant-work commented Dec 2, 2021

kaiyue0329 commented Dec 2, 2021

kenneth-marut-work left a comment

colin-grant-work commented Dec 14, 2021 •

edited

Loading

colin-grant-work left a comment

kenneth-marut-work left a comment

Add sash option for widget resize #10441

Add sash option for widget resize #10441

Conversation

kaiyue0329 commented Nov 19, 2021 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

vince-fugnitto left a comment

Choose a reason for hiding this comment

vince-fugnitto Nov 22, 2021

Choose a reason for hiding this comment

kaiyue0329 Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

kaiyue0329 commented Nov 23, 2021 • edited Loading

colin-grant-work left a comment

Choose a reason for hiding this comment

kaiyue0329 commented Nov 24, 2021

msujew left a comment

Choose a reason for hiding this comment

kenneth-marut-work commented Nov 29, 2021

kaiyue0329 commented Dec 2, 2021 • edited Loading

kaiyue0329 commented Dec 2, 2021

kaiyue0329 commented Dec 2, 2021

colin-grant-work commented Dec 2, 2021

kaiyue0329 commented Dec 2, 2021

kenneth-marut-work left a comment

Choose a reason for hiding this comment

colin-grant-work commented Dec 14, 2021 • edited Loading

colin-grant-work left a comment

Choose a reason for hiding this comment

kenneth-marut-work left a comment

Choose a reason for hiding this comment

kaiyue0329 commented Nov 19, 2021 •

edited

Loading

kaiyue0329 Nov 23, 2021 •

edited

Loading

kaiyue0329 commented Nov 23, 2021 •

edited

Loading

kaiyue0329 commented Dec 2, 2021 •

edited

Loading

colin-grant-work commented Dec 14, 2021 •

edited

Loading