[markers] enable single-click and keyboard arrow selection to navigate markers by vince-fugnitto · Pull Request #5646 · eclipse-theia/theia

vince-fugnitto · 2019-07-04T15:02:10Z

the current implementation of the problems-widget requires double-clicking
nodes in order to navigate to the problems. This is an annoying behavior and
inconsistent with vscode where if you single-click a marker the corresponding
editor is revealed. The feature works both with single clicking markers, as well
as selecting them with the keyboard arrow keys.

Signed-off-by: Vincent Fugnitto vincent.fugnitto@ericsson.com

lmcbout · 2019-07-04T19:35:06Z

Question: while testing, I noticed the selected file opens, but the visible position of the line is not selected, could it be possible to highlight or select the line where the marker is pointing to, not just opening the file ?

fangnx

Single-clicking works smoothly. As Jacques suggested, it would be better to set the focus to the specific line of the problem, but I guess that can be a different PR.

vince-fugnitto · 2019-07-04T22:50:24Z

@fangnx @lmcbout thanks guys, I updated the code, it should work a lot better now :)

When a marker node is selected in the tree (through single-click or with the keyboard arrows) the corresponding marker in the editor should be revealed and highlighted without the cursor (so that the problems-widget keeps focus)

lmcbout · 2019-07-05T12:23:50Z

While testing, if the file is not expanded in the problem view, you don't expand it and open the first item. When the file is expanded in the problem view, then it navigates well.
Can we expand the file to navigate to the next problem item ?

vince-fugnitto · 2019-07-05T12:40:12Z

While testing, if the file is not expanded in the problem view, you don't expand it and open the first item. When the file is expanded in the problem view, then it navigates well.
Can we expand the file to navigate to the next problem item ?

@lmcbout

Files nodes are expanded using Enter, is this not sufficient?
The same is true for other trees (ex: explorer, search-in-workspace)

I actually prefer it this way, it means I can easily navigate to the file I want, press Enter and get the errors I care about. If I were to expand at each selection it'd take me much longer.

lmcbout

Explanation about the "enter" key is fine, also for performance
Code looks OK
Testing: LGTM
Thanks @vince-fugnitto

vince-fugnitto · 2019-07-05T13:27:03Z

@lmcbout thank you! I'll wait for others to give a review as well.
@akosyakov if you ever get the chance, can you review too?

akosyakov · 2019-07-05T13:29:57Z

packages/markers/src/browser/problem/problem-widget.tsx


        this.addClipboardListener(this.node, 'copy', e => this.handleCopy(e));
+        this.toDispose.push(this.model.onSelectionChanged(selection => {
+            const node = selection[0];


this event is fired even if someone programmatically select a node, you don't want to focus in this case

I was following other designs from other tree-widgets which used the same event but I'll try using the suggestion you made.

Could you point me to this code?

Sure, there's a few other widgets that did it similarly to my initial implementation:

https://github.com/theia-ide/theia/blob/d4adca224ff8e0314101ca792bf98967e4e33184/packages/callhierarchy/src/browser/callhierarchy-tree/callhierarchy-tree-widget.tsx#L53-L57

https://github.com/theia-ide/theia/blob/d4adca224ff8e0314101ca792bf98967e4e33184/packages/typehierarchy/src/browser/tree/typehierarchy-tree-widget.tsx#L48-L53

It will bite there if someone trigers programmatic selection. I am not sure that a user then wants to have some editors pop up.

It will bite there if someone trigers programmatic selection. I am not sure that a user then wants to have some editors pop up.

Should we file issues to track and fix them?

Let's wait for someone to open. It is probably very rare case.

akosyakov · 2019-07-05T13:34:38Z

You should implement it explicitly by overriding handleClickEvent.

packages/markers/src/browser/marker-tree-model.ts

vince-fugnitto · 2019-07-05T13:42:15Z

You should implement it explicitly by overriding handleClickEvent.

Will this work when using the keyboard to select tree nodes and navigating?

akosyakov · 2019-07-05T13:43:16Z

@vince-fugnitto No, when you should override other widget methods which are responsible for keyboard navigation. You cannot rely on generic selection event.

vince-fugnitto · 2019-07-05T15:10:18Z

@akosyakov I overwrote the handleClickEvent, handleDown and handleUp methods to implement the feature, is this the correct way forward?

packages/markers/src/browser/problem/problem-widget.tsx

akosyakov · 2019-07-05T15:18:38Z

@vince-fugnitto yes, that's what i meant

vince-fugnitto · 2019-07-05T15:25:47Z

@vince-fugnitto yes, that's what i meant

Thank you for the feedback and your help implementing it better!
@lmcbout do you mind re-testing to see if functionally it's still correct?

lmcbout

Works fine

vince-fugnitto · 2019-07-08T15:18:20Z

@akosyakov are you fine with me merging the PR?

akosyakov

I've only reviewed the code.

…markers Fixes #5003 - the current implementation of the `problems-widget` requires double-clicking nodes in order to navigate to the problems. This is an annoying behavior and inconsistent with vscode where if you single-click a marker the corresponding editor is revealed. The feature works both with single clicking markers, as well as selecting them with the keyboard arrow keys. Signed-off-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>

vince-fugnitto added enhancement issues that are enhancements to current functionality - nice to haves markers issues related to problem markers problems issues related to the problems widget labels Jul 4, 2019

vince-fugnitto requested review from akosyakov, fangnx and lmcbout July 4, 2019 15:02

vince-fugnitto self-assigned this Jul 4, 2019

fangnx reviewed Jul 4, 2019

View reviewed changes

vince-fugnitto force-pushed the vf/GH-5003 branch from 6036fa2 to 575edb2 Compare July 4, 2019 22:47

lmcbout approved these changes Jul 5, 2019

View reviewed changes

akosyakov reviewed Jul 5, 2019

View reviewed changes

packages/markers/src/browser/marker-tree-model.ts Outdated Show resolved Hide resolved

vince-fugnitto force-pushed the vf/GH-5003 branch 2 times, most recently from d76c038 to 905c907 Compare July 5, 2019 15:01

akosyakov reviewed Jul 5, 2019

View reviewed changes

packages/markers/src/browser/problem/problem-widget.tsx Outdated Show resolved Hide resolved

vince-fugnitto force-pushed the vf/GH-5003 branch from 905c907 to 1c25288 Compare July 5, 2019 15:24

lmcbout approved these changes Jul 5, 2019

View reviewed changes

akosyakov approved these changes Jul 8, 2019

View reviewed changes

vince-fugnitto force-pushed the vf/GH-5003 branch from 1c25288 to d36a918 Compare July 9, 2019 01:39

vince-fugnitto merged commit 4f58723 into master Jul 9, 2019

vince-fugnitto deleted the vf/GH-5003 branch July 9, 2019 02:19

Conversation

vince-fugnitto commented Jul 4, 2019

Uh oh!

lmcbout commented Jul 4, 2019

Uh oh!

fangnx left a comment

Choose a reason for hiding this comment

Uh oh!

vince-fugnitto commented Jul 4, 2019

Uh oh!

lmcbout commented Jul 5, 2019

Uh oh!

vince-fugnitto commented Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmcbout left a comment

Choose a reason for hiding this comment

Uh oh!

vince-fugnitto commented Jul 5, 2019

Uh oh!

akosyakov Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

vince-fugnitto Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

akosyakov Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

vince-fugnitto Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

akosyakov Jul 8, 2019

Choose a reason for hiding this comment

Uh oh!

vince-fugnitto Jul 8, 2019

Choose a reason for hiding this comment

Uh oh!

akosyakov Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

akosyakov commented Jul 5, 2019

Uh oh!

Uh oh!

vince-fugnitto commented Jul 5, 2019

Uh oh!

akosyakov commented Jul 5, 2019

Uh oh!

vince-fugnitto commented Jul 5, 2019

Uh oh!

Uh oh!

akosyakov commented Jul 5, 2019

Uh oh!

vince-fugnitto commented Jul 5, 2019

Uh oh!

lmcbout left a comment

Choose a reason for hiding this comment

Uh oh!

vince-fugnitto commented Jul 8, 2019

Uh oh!

akosyakov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vince-fugnitto commented Jul 5, 2019 •

edited

Loading