Implemented filter for debug console output by arhelmus · Pull Request #102704 · microsoft/vscode

arhelmus · 2020-07-16T14:29:45Z

This PR fixes #20838

After migration from Chrome DevTools to VS Code debugger, our users experienced major slowdown in productivity due to missed filtering in debug console. Users used to use filter to cut off irrelevant logs (as those pretty verbose) and with this PR I bringing this possibility in VS Code.

This feature is different from already implemented search, as instead of helping you to find what you searching for, it showing you only what you need.

Video: https://streamable.com/tvatmz

isidorn · 2020-07-28T15:06:05Z

@archDev thanks a lot for this great PR.
Since we are wrapping up our milestone and I was on vacation until today I am assigning this to August so we ideally merge it then and test it out.

In the meantime I just gave this a birds eye review.

Did you checkout how the markers view is doing the filtering here. What needs to be done in the REPL is pretty much the same work unless I am mistaken
Please get latest from master so we test this out with latest Electron version which we pushed to master
Is it possible to write some unit tests
Is it possible to have code re-use between the markers view and what you introduced in the REPL, so we do not have everything duplicated

Next milestone I plan to do a more thorough review.
Thanks a lot for your work!

fyi @joaomoreno and @sandy081 since they worked on the Markers view filter

arhelmus · 2020-07-29T13:49:57Z

Thanks for raising the priority for this @isidorn!

It was the plan initially, but to implement filtering by type (show infos/warnings/errors), this information should present as part of IReplElement. I didnt found easy way to get this information in, so decided to just remove this type of filtering at all. Any suggestions?

2/3. I will try to allocate some time this week to do this

It is definitely possible, I believe the input view itself can definetely by moved to common component. I don't feel confident in making this as I barely understand all the usecases of marker filter (I dropped quite a lot of code from it for repl), would love to get some help here.

isidorn · 2020-07-29T14:09:16Z

No problem, there is no priority, I was just on vacation for the past 3 weeks.

I did not mean filtering by type, but I think you answered my question, you started with what Markers pane is doing and tried to make it simpler
2 / 3 Great.
Let's look into this once we merge this PR, so we can do that later if we think it makes sense

Once you do 2/3 I plan to review it some time start of August. Thanks for working on this.

arhelmus · 2020-07-30T19:10:17Z

@isidorn rebased on master, ensured that it still works and types are correct, added tests on filtering process in repl model

isidorn · 2020-07-31T11:45:58Z

@arhelmus great thanks. I plan to review and test it in detail in 10 days - since next week we are releasing our current stable build. I will provide more feedback then.

isidorn · 2020-08-11T12:56:21Z

Thanks again for this great PR. I have just tested this out and here's my feedback regarding the functionality:

Filter box background seems to not be themable, it should have the same background color as the problems filter. You can for example swithc to Monoaki theme
Even though the width is the same as the problems panel filter width I personaly think it is too wide. @sandy081 would you mind if we decrease the width of the filter box a bit?
Do not store filter state across vscode releoads. Reload should just clear filter. We can add this later if users ask for it

Regarding the code changes I will comment directly in the code.

isidorn · 2020-08-11T12:57:48Z

src/vs/workbench/contrib/debug/browser/media/replFilter.css

@@ -0,0 +1,41 @@
+/*---------------------------------------------------------------------------------------------


Can you please put this under repl.css I do not see a need for having a seperate file,
Or even better @sandy081 what do you think about having shared css rules between problems filter and debug console filter so we do not have this duplication.
@arhelmus did you change anything from the css rules from the problems or they are 100% the same?

works for me

isidorn · 2020-08-11T12:58:18Z

src/vs/workbench/contrib/debug/browser/repl.ts

 const $ = dom.$;

 const HISTORY_STORAGE_KEY = 'debug.repl.history';
+const FILTER_STATE_STORAGE_KEY = 'debug.repl.filter';


As mentioned already we should not store the filter input. It is fine if it gets cleared across reload imho.

isidorn · 2020-08-11T12:58:38Z

src/vs/workbench/contrib/debug/browser/repl.ts

 	private completionItemProvider: IDisposable | undefined;
 	private modelChangeListener: IDisposable = Disposable.None;
+	private readonly panelState: MementoObject;
+	private readonly filterActionId: string = `workbench.actions.treeView.${this.id}.filter`;


This action id should not be a private, but a top level const .

isidorn · 2020-08-11T12:59:38Z

src/vs/workbench/contrib/debug/browser/repl.ts

 		this.history = new HistoryNavigator(JSON.parse(this.storageService.get(HISTORY_STORAGE_KEY, StorageScope.WORKSPACE, '[]')), 50);
+		this.panelState = new Memento(FILTER_STATE_STORAGE_KEY, storageService).getMemento(StorageScope.WORKSPACE);
+
+		const initialFilterText = this.panelState['filter'] || '';


This filtering objects should be created when the tree gets created, in the renderBody, not in the constructor

I was surprised with this comment as that's how filter stuff created in markers view, in constructor. I tried to move it to render body and problem is that repl.getActions is triggered before repl.renderBody as executed, and if filter state will not be created in constructor, we will need to handle undefined invariant in filter view

isidorn · 2020-08-11T13:00:47Z

src/vs/workbench/contrib/debug/browser/repl.ts

 		this.replInputContainer.style.height = `${replInputHeight}px`;

 		this.replInput.layout({ width: width - 30, height: replInputHeight });
+		this.filterState.layout = new dom.Dimension(width, height);


I do not understand why we update teh filterStat layout when in practice this does not seem to do anything ofr me.
Also filterState and filter should idealy not be attributes on the repl but just local variables passed to the tree.

I think it is required for this state change, but I not sure how it is implemented for markers filter. It seems to me like there are some layout change happening which I can't find. Can you help me with this?

isidorn · 2020-08-11T13:02:57Z

src/vs/workbench/contrib/debug/common/replModel.ts


 	getReplElements(): IReplElement[] {
-		return this.replElements;
+		return this.replElements.filter(element =>


Is this really needed? Do the problems do the same?
I thought the tree will do the filtering even if we give it more data? Or I might be mistaken?

unfortunately, this model is literally a model of real data source which has nothing to do with actual implementation (ReplDataSource). I didn't found better way to test this than to add support of primitive filtering to the model.

isidorn · 2020-08-11T13:03:17Z

src/vs/workbench/contrib/debug/test/browser/repl.test.ts

 		assert.equal(repl.getReplElements().length, 3);
 		assert.equal((<SimpleReplElement>repl.getReplElements()[2]).value, 'second global line');
 	});
+


Good job with tests

isidorn · 2020-08-11T13:03:49Z

src/vs/workbench/contrib/debug/browser/replFilterView.ts

+			this.clearFilterText();
+			handled = true;
+		}
+		if (handled) {


I believe this should just be in the previous if statment and we do not need the handled boolean. Feels like a copy paste error.

isidorn · 2020-08-11T13:04:15Z

src/vs/workbench/contrib/debug/browser/replFilterView.ts

+
+	protected updateClass(): void {
+		if (this.element && this.container) {
+			this.element.className = this.class;


This does not seem to work for me. Never does the filter grow, it is always large.

I double checked, now when I change size of window, class is changing.

isidorn · 2020-08-11T13:05:51Z

src/vs/workbench/contrib/debug/browser/replFilterView.ts

@@ -0,0 +1,180 @@
+/*---------------------------------------------------------------------------------------------


As far as I understand nothing in this calss is REPL specific.
Is this a 1:1 copy from the Problems? If yes we should just have a shared class
fyi @sandy081

isidorn · 2020-08-11T13:07:01Z

I have added comments inline. Mostly smaller things, looks good overall.
I would like to understand exactly how much fo the funcionality is shared between problems and debug console so ideally we would just share the same classes and not have code duplication.
Thanks a lot!

arhelmus · 2020-08-11T21:08:18Z

I will allocate some time this week to address your comments, thanks for review

isidorn · 2020-08-12T12:00:07Z

Fantastic. After discussing with my collegues we decided it would be best to introduce a new folder
vs/workbench/contrib/treeFilter under which we should put all the shared things between the Problems view and the DebugConsole. So the .cssfile and the ParsedQuery logic and everything which makes sense to you.

arhelmus · 2020-08-16T12:01:06Z

@isidorn I fixed 1 and 3 from your first comment, moved the filter view in separate component (I believe its only part which can be shared and its actually the part with all the copy-paste I did) and addressed your comments to the code.

Do you think now you can handle this PR to unify treeViewItem between markers and repl?

isidorn · 2020-08-17T11:31:34Z

@arhelmus thanks a lot.
I can try to take it over from here to merge it in and also use it in the markers view.

sandy081 · 2020-08-17T11:56:17Z

@isidorn Just back from vacation. Have not gone through all changes and I trust your decisions. Let me know if you need any input from me?

isidorn · 2020-08-17T13:11:17Z

@sandy081 ok. I will take care of it. Will ping you if something is needed. Thanks

isidorn · 2020-08-21T18:38:18Z

Thanks again for this PR. Merging it in, will polish with some commits on top on master.
☀️ 👏

RobinLbt · 2020-09-01T13:52:26Z

@isidorn is it included in the last release of VS Code ? I can't find filters in the debug console still

isidorn · 2020-09-01T18:31:49Z

It is in vscode insiders. Will be in next vscode stable in 10 days.

vscode-triage-bot assigned isidorn and weinand Jul 16, 2020

arhelmus force-pushed the master branch from ef50df9 to 9bf52fc Compare July 16, 2020 16:51

weinand added the debug Debug viewlet, configurations, breakpoints, adapter issues label Jul 20, 2020

weinand removed their assignment Jul 20, 2020

weinand added this to the On Deck milestone Jul 20, 2020

weinand added the feature-request Request for new features or functionality label Jul 20, 2020

isidorn modified the milestones: On Deck, August 2020 Jul 28, 2020

arhelmus force-pushed the master branch from 9bf52fc to a111f6b Compare July 30, 2020 16:34

DanTup mentioned this pull request Aug 6, 2020

Debug Console window unusable while showing ads on emulator Dart-Code/Dart-Code#1980

Closed

isidorn requested changes Aug 11, 2020

View reviewed changes

arhelmus added 2 commits August 16, 2020 11:55

Implemented filter for debug console output

3be3003

Fixed rebase issues, added test for filtering

166da46

arhelmus force-pushed the master branch from a8dd1fd to 166da46 Compare August 16, 2020 10:56

Moved repl tree filter to the separate common component

bbbc314

isidorn merged commit 4b90413 into microsoft:master Aug 21, 2020

isidorn mentioned this pull request Aug 21, 2020

Word filter for debug console #20838

Closed

github-actions bot locked and limited conversation to collaborators Oct 5, 2020

		@@ -0,0 +1,41 @@
		/*---------------------------------------------------------------------------------------------

		@@ -0,0 +1,180 @@
		/*---------------------------------------------------------------------------------------------

Conversation

arhelmus commented Jul 16, 2020 • edited by isidorn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isidorn commented Jul 28, 2020

Uh oh!

arhelmus commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isidorn commented Jul 29, 2020

Uh oh!

arhelmus commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isidorn commented Jul 31, 2020

Uh oh!

isidorn commented Aug 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arhelmus Aug 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arhelmus Aug 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isidorn commented Aug 11, 2020

Uh oh!

arhelmus commented Aug 11, 2020

Uh oh!

isidorn commented Aug 12, 2020

Uh oh!

arhelmus commented Aug 16, 2020

Uh oh!

isidorn commented Aug 17, 2020

Uh oh!

sandy081 commented Aug 17, 2020

Uh oh!

isidorn commented Aug 17, 2020

Uh oh!

isidorn commented Aug 21, 2020

Uh oh!

RobinLbt commented Sep 1, 2020

Uh oh!

isidorn commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

arhelmus commented Jul 16, 2020 •

edited by isidorn

Loading

arhelmus commented Jul 29, 2020 •

edited

Loading

arhelmus commented Jul 30, 2020 •

edited

Loading

arhelmus Aug 16, 2020 •

edited

Loading

arhelmus Aug 16, 2020 •

edited

Loading