GH-7347: Added scroll-lock to the Output view #7570

kittaakos · 2020-04-14T13:07:53Z

What it does

Adds a scroll-lock to the Output view,
Removes the @theia/output dependency from the @theia/languages extension,
Replaces the Output view with a monaco editor,
Adds context menu to the Output view,
Removes the <no output yet> when there is no output on the selected channel,
Fixes a runtime error when creating an in-memory (no URI) monaco.editor.ITextModel,
Can add decoration via append (previously it was supported with appendLine only),
Aligns the OutputChannel API with VS Code (via commands), and
Adds quick pick for showing, hiding, and closing and output channel.

How to test

You can lock the scrolling in the view,
You can use the context menu from the Output view,
You can clear the Output view,
You can show, hide and close output channels from the Command Palette,
Clicking on the tab or in the widget sets the focus properly on the widget. You can see the bluish focus color (with the dark theme) when it's active. Also, there are no warnings due to widget did not activate in the logs ([shell] Widget activation causes a resource leak #7008),
You can alter the max output line count from the preferences, the output scales, (set it to e.g. 20)
Once you are done with the testing, make sure you dispose one of the output channels (use the Output: Close Output Channel... command), refresh the browser, make a memory snapshot, verify that the previously disposed channel is not in the snapshot.

Feel free using the interim commands for populating the output channels with some dummy inputs:

API Sample: Post Date.now() to the 'X' channel.
API Sample: Stop Date.now() on 'X' channel.

Closes #7347.
Closes #7008.

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

paul-marechal · 2020-04-14T20:21:57Z

The locked behavior feels weird: in VS Code, it just disables auto-scrolling, but you can still manually scroll around. This implementation prevents me from manually scrolling elsewhere. Is this intended?

kittaakos

TODOs for me. None of them should affect the functional behavior.

packages/output/src/common/output-channel.ts

packages/output/src/browser/output-widget.tsx

akosyakov · 2020-04-16T08:09:03Z

I see that output content is rendered even if the output widget is not visible. We should not render anything if the output widget is not selected and visible. It will cause performance issues.

EditorConfig is selected, but one and three channels keep rendering:

kittaakos · 2020-04-16T08:10:13Z

I see that output content is rendered even if the output widget is not visible. We should not render anything if the output widget is not selected and visible.

Yes. Otherwise, you cannot keep the scroll-bar.

akosyakov · 2020-04-16T08:11:40Z

Yes. Otherwise, you cannot keep the scroll-bar.

VS Code does not keep it as well and use only one Monaco editor. I also don't like an idea of having many Monaco editors later. I've seen before in profiling session that text.split(/[\n\r]+/) is hot spot sometimes, i.e. tracing chatty language servers.

kittaakos · 2020-04-16T08:15:00Z

I've seen before in profiling session that text.split(/[\n\r]+/) is hot spot

Yes, but I have not changed that within this PR.

akosyakov · 2020-04-16T08:37:39Z

Yes, but I have not changed that within this PR.

On master we never render a channel if it is not selected and visible. With this PR whenever the ouput view is attached all channels get constantly rendered making situation much worse.

Plus if we switch to Monaco editor it forces us to have many instances instead of only one editor which content gets replaced when a channel is switched.

kittaakos · 2020-04-19T18:56:41Z

if we switch to Monaco editor

@akosyakov, I could not pull in the @theia/monaco dependency into @theia/output. Sadly, @theia/languages already depends on @theia/output. There is a cycle. Is it OK to depend on @theia/monaco-editor-core? We can also clean up the dependencies first, then target @theia/monaco instead. Both have their pros and cons. What do you think? Thanks!

akosyakov · 2020-04-20T06:17:57Z

Sadly, @theia/languages already depends on @theia/output.

We are going to drop monaco-languageclient then @theia/languages will be removed.

kittaakos · 2020-04-20T07:10:51Z

We are going to drop monaco-languageclient then @theia/languages will be removed.

What should we do before we drop @theia/languages? Thank you!

paul-marechal · 2020-04-20T14:26:25Z

There is a bug when I open the output view for the first time, width is too small:

It is fixed after switching channels.

Tested on Firefox 75.

kittaakos · 2020-04-20T15:05:59Z

We are going to drop monaco-languageclient then @theia/languages will be removed.

What should we do before we drop @theia/languages? Thank you!

@akosyakov, ping.

kittaakos · 2020-04-20T15:18:55Z

What should we do before we drop @theia/languages? Thank you!

I can create Output commands and invoke them from @theia/languages. It's just a string so no hard dependencies are required. Thoughts?

kittaakos · 2020-04-20T15:42:41Z

create Output commands

We could align the new commands with the current VS Code OutputChannel API.

akosyakov · 2020-04-20T17:25:15Z

What should we do before we drop @theia/languages? Thank you!

#7100 It is in plans to work on it next months. cc @svenefftinge @marcdumais-work

kittaakos · 2020-05-27T13:29:26Z

The PR is ready for the review, please see the updated description.

paul-marechal · 2020-05-27T15:26:32Z

Only concern I have: Is the Find Command item supposed to be in the Output View context menu?

paul-marechal

Code LGTM, tried what you listed under How To Test and everything worked.

packages/output/src/browser/output-widget.tsx

kittaakos · 2020-05-27T16:43:23Z

Is the Find Command item supposed to be in the Output View context menu?

Thanks for asking! It's intentional. Actually, I do not mind if it's there or not, I won't ever use it, but I wanted to align the context-menu with VS Code:

VS Code has it as Comannd Palette..., we call it Find Commands....

I leave the decision to the community.

lmcbout · 2020-05-27T19:00:20Z

I noticed the context menu on MAC seems different than the one in UBUNTU. I have the same as @marechal-p above.

I vote to keep the command palette.

Note: When I add a new plugins ( vscode-coverage-gutters-2.4.3.vsix) , I noticed that the context menu is modified in VSCode, but the context menu in Theia is not modified.

Note2: I think it would be nice also to have in the context menu an entry: "Copy all" to copy all the data from that output channel or "Select all"

kittaakos · 2020-05-27T19:05:51Z

I noticed the context menu on MAC seems different than the one in UBUNTU. I have the same as @marechal-p above.

FYI: My screenshot was from VS Code, and not Theia.

Note: When I add a new plugins ( vscode-coverage-gutters-2.4.3.vsix) , I noticed that the context menu is modified in VSCode, but the context menu in Theia is not modified.

👍 Feel free to open a feature request for it.

Note2: I think it would be nice also to have in the context menu an entry: "Copy all" to copy all the data from that output channel or "Select all"

Sounds great, can you please file a feature request so that we won't forget to implement it. Thanks!

lmcbout

New issues ( #7911 and #7912 ) added according to the features request
Works fine, tested on UBUNTU 18.04 and Chrome
Thanks @kittaakos

akosyakov

I'm uncomfortable with many new contribution points introduced to satisfy a single use case. Usually we need at least 3 to generalize something. I added comments with proposals how to simplify new concepts.

packages/monaco/src/browser/monaco-editor-model.ts

packages/monaco/src/browser/monaco-editor-provider.ts

akosyakov · 2020-06-03T07:37:03Z

packages/monaco/src/browser/monaco-text-model-service.ts

+
+    createModel(
+        resource: Resource,
+        m2p: MonacoToProtocolConverter,


I don't think we need to pass converters. They can be injected by subclasses. Or we can convert interface to abstract class to replace interface + symbol and inject converters for all subclasses.

akosyakov · 2020-06-03T07:49:36Z

packages/output/src/browser/output-contribution.ts

-        return false;
+    open(uri: URI, options?: OpenerOptions): Promise<OutputWidget> {
+        return new Promise<OutputWidget>((resolve, reject) => {
+            if (!OutputUri.is(uri)) {


why not to use async/await? this method will hang if something goes wrong in openView or setInptu

packages/output/src/browser/output-resource.ts

akosyakov

wow, that's a great improvement to the output view!

akosyakov · 2020-06-03T08:05:12Z

@kittaakos does it resolves #2702 and #2385 as well?

Closes #7347. Closes #7008. Signed-off-by: Akos Kitta <kittaakos@typefox.io>

kittaakos · 2020-06-03T10:32:02Z

Thank you all for the review!

colin-grant-work · 2022-06-08T17:53:18Z

packages/output/src/browser/output-widget.tsx

+class NoopDragOverDockPanel extends DockPanel {
+
+    constructor(options?: DockPanel.IOptions) {
+        super(options);
+        NoopDragOverDockPanel.prototype['_evtDragOver'] = (event: IDragEvent) => {
+            event.preventDefault();
+            event.stopPropagation();
+            event.dropAction = 'none';
+        };
+    }
+
+}


@kittaakos, it looks like this code is responsible for #10932. Do you recall the motivation for making the Output view undroppable?

Thanks for taking care of this!

it looks like this code is responsible for #10932.

Do you want to have the same behavior as the Problem view? Split the view like this:

other_widget.mp4

It is not possible with the Output view; it's a bug.

The main reason I used a custom panel implementation was to avoid dropping an editor into the panel. Like this:

output.mp4

I hope this helps. Let me know if there is anything I can assist you with fixing this.

Thanks for the screen casts; they help clarify things quite a bit. I didn't realize the Output widget behaved as a panel with each of the channels as a child widget. I think it would be nice if the Output view could allow drops as other widgets do, to either place the new widget on the bottom tabbar or split the bottom pane. Otherwise, it feels like a bug, even though it's deliberate, since it isn't clear to users why the Output widget would be different from other widgets in this respect.

It looks like something like this might do the trick:

/** * Customized `DockPanel` that does not allow dropping widgets into it. * Intercepts `'p-dragover'` events, and sets the desired drop action to `'none'`. */ class NoopDragOverDockPanel extends DockPanel { } const noop = () => { }; NoopDragOverDockPanel.prototype['_evtDragOver'] = noop; NoopDragOverDockPanel.prototype['_evtDrop'] = noop; NoopDragOverDockPanel.prototype['_evtDragLeave'] = noop;

By preventing the Output widget from handling any of those events, we pass them up to the bottom pane dock panel and allow it to handle them. What do you think of that?

kittaakos force-pushed the output-scroll-lock branch from fdd86b7 to b71c29b Compare April 14, 2020 16:08

kittaakos force-pushed the output-scroll-lock branch 3 times, most recently from 9fd843f to fec6c49 Compare April 15, 2020 15:10

kittaakos requested review from vince-fugnitto, akosyakov and lmcbout April 15, 2020 15:16

kittaakos marked this pull request as ready for review April 15, 2020 15:16

kittaakos force-pushed the output-scroll-lock branch from fec6c49 to 02067fb Compare April 15, 2020 15:50

kittaakos commented Apr 15, 2020

View reviewed changes

akosyakov added the output issues related to the output label Apr 16, 2020

kittaakos force-pushed the output-scroll-lock branch 3 times, most recently from 93e5264 to d3d20bf Compare April 16, 2020 07:57

kittaakos force-pushed the output-scroll-lock branch from d3d20bf to 657b25c Compare April 18, 2020 16:02

kittaakos changed the title ~~GH-7347: Added scroll-lock to the Output view~~ [WIP] GH-7347: Added scroll-lock to the Output view Apr 18, 2020

kittaakos force-pushed the output-scroll-lock branch from a94e465 to 72fd6c1 Compare May 27, 2020 12:31

kittaakos changed the title ~~[WIP] GH-7347: Added scroll-lock to the Output view~~ GH-7347: Added scroll-lock to the Output view May 27, 2020

paul-marechal approved these changes May 27, 2020

View reviewed changes

packages/output/src/browser/output-widget.tsx Show resolved Hide resolved

This was referenced May 27, 2020

Adjust the context menu for the output view according to the added plugins #7911

Open

Add new menu item in the output view context menu #7912

Closed

lmcbout approved these changes May 27, 2020

View reviewed changes

akosyakov reviewed May 28, 2020

View reviewed changes

packages/monaco/src/browser/monaco-editor-model.ts Outdated Show resolved Hide resolved

packages/monaco/src/browser/monaco-editor-provider.ts Outdated Show resolved Hide resolved

kittaakos force-pushed the output-scroll-lock branch from 72fd6c1 to 928adf4 Compare June 2, 2020 12:18

akosyakov reviewed Jun 3, 2020

View reviewed changes

packages/output/src/browser/output-resource.ts Outdated Show resolved Hide resolved

akosyakov approved these changes Jun 3, 2020

View reviewed changes

akosyakov mentioned this pull request Jun 3, 2020

outputChannel.show() no longer reveals output channel #7383

Closed

kittaakos force-pushed the output-scroll-lock branch from 928adf4 to acb7be7 Compare June 3, 2020 09:25

GH-7347: Added scroll-lock to the Output view.

7048b1a

Closes #7347. Closes #7008. Signed-off-by: Akos Kitta <kittaakos@typefox.io>

kittaakos force-pushed the output-scroll-lock branch from acb7be7 to 7048b1a Compare June 3, 2020 09:41

kittaakos merged commit 6043722 into master Jun 3, 2020

kittaakos deleted the output-scroll-lock branch June 3, 2020 10:32

This was referenced Jun 3, 2020

Allow to search in the output view #2702

Closed

[output] As a user I would like to customize the font style in the Output view #2385

Closed

kittaakos mentioned this pull request Mar 1, 2021

Replace the serial monitor view with a monaco editor arduino/arduino-ide#105

Open

colin-grant-work reviewed Jun 8, 2022

View reviewed changes

colin-grant-work mentioned this pull request Jun 8, 2022

Prevent Output widget from handling any drag/drop events #11275

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-7347: Added scroll-lock to the Output view #7570

GH-7347: Added scroll-lock to the Output view #7570

kittaakos commented Apr 14, 2020 •

edited

Loading

paul-marechal commented Apr 14, 2020

kittaakos left a comment

akosyakov commented Apr 16, 2020 •

edited

Loading

kittaakos commented Apr 16, 2020

akosyakov commented Apr 16, 2020 •

edited

Loading

kittaakos commented Apr 16, 2020

akosyakov commented Apr 16, 2020

kittaakos commented Apr 19, 2020

akosyakov commented Apr 20, 2020

kittaakos commented Apr 20, 2020

paul-marechal commented Apr 20, 2020 •

edited

Loading

kittaakos commented Apr 20, 2020

kittaakos commented Apr 20, 2020

kittaakos commented Apr 20, 2020

akosyakov commented Apr 20, 2020

kittaakos commented May 27, 2020

paul-marechal commented May 27, 2020

paul-marechal left a comment

kittaakos commented May 27, 2020

lmcbout commented May 27, 2020

kittaakos commented May 27, 2020

lmcbout left a comment

akosyakov left a comment

akosyakov Jun 3, 2020

akosyakov Jun 3, 2020

akosyakov left a comment

akosyakov commented Jun 3, 2020

kittaakos commented Jun 3, 2020

colin-grant-work Jun 8, 2022

kittaakos Jun 8, 2022

colin-grant-work Jun 8, 2022

colin-grant-work Jun 8, 2022

GH-7347: Added scroll-lock to the Output view #7570

GH-7347: Added scroll-lock to the Output view #7570

Conversation

kittaakos commented Apr 14, 2020 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

paul-marechal commented Apr 14, 2020

kittaakos left a comment

Choose a reason for hiding this comment

akosyakov commented Apr 16, 2020 • edited Loading

kittaakos commented Apr 16, 2020

akosyakov commented Apr 16, 2020 • edited Loading

kittaakos commented Apr 16, 2020

akosyakov commented Apr 16, 2020

kittaakos commented Apr 19, 2020

akosyakov commented Apr 20, 2020

kittaakos commented Apr 20, 2020

paul-marechal commented Apr 20, 2020 • edited Loading

kittaakos commented Apr 20, 2020

kittaakos commented Apr 20, 2020

kittaakos commented Apr 20, 2020

akosyakov commented Apr 20, 2020

kittaakos commented May 27, 2020

paul-marechal commented May 27, 2020

paul-marechal left a comment

Choose a reason for hiding this comment

kittaakos commented May 27, 2020

lmcbout commented May 27, 2020

kittaakos commented May 27, 2020

lmcbout left a comment

Choose a reason for hiding this comment

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov Jun 3, 2020

Choose a reason for hiding this comment

akosyakov Jun 3, 2020

Choose a reason for hiding this comment

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Jun 3, 2020

kittaakos commented Jun 3, 2020

colin-grant-work Jun 8, 2022

Choose a reason for hiding this comment

kittaakos Jun 8, 2022

Choose a reason for hiding this comment

colin-grant-work Jun 8, 2022

Choose a reason for hiding this comment

colin-grant-work Jun 8, 2022

Choose a reason for hiding this comment

kittaakos commented Apr 14, 2020 •

edited

Loading

akosyakov commented Apr 16, 2020 •

edited

Loading

akosyakov commented Apr 16, 2020 •

edited

Loading

paul-marechal commented Apr 20, 2020 •

edited

Loading