New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #900 Listen to ESC, enter, double-click, and single click #901
Conversation
Improve selection listener concept. Ensure, elements can be selected with keyboard or mouse. Extract method for revealing the selected element in editor.
org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/symbols/LSPSymbolInFileDialog.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/symbols/LSPSymbolInFileDialog.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/symbols/LSPSymbolInFileDialog.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/symbols/LSPSymbolInFileDialog.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/symbols/LSPSymbolInFileDialog.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/symbols/LSPSymbolInFileDialog.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/symbols/LSPSymbolInFileDialog.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/symbols/LSPSymbolInFileDialog.java
Show resolved
Hide resolved
It seems, there is again a time-out problem. Unfortunately, I cannot trigger a new build. |
org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/symbols/LSPSymbolInFileDialog.java
Outdated
Show resolved
Hide resolved
|
||
viewer.setInput(outlineViewerInput); | ||
return filteredTree; | ||
close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have moved this now out of the if (range != null) {
? Should it not be inside as before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my next comment
Moving the close operation to the end of the steps did not fix time-out problems. This original order does make more sense to me.
Object selectedElement= getSelectedElement(); | ||
|
||
if (selectedElement != null) { | ||
close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it not go after the catch on the block contaning textEditor.selectAndReveal(offset, endOffset - offset);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. My rationale was, if I want the editor to jump to a certain position and I have a user-confirmed selection in the quick outline view, i.e. the user did hit enter or clicked on the selected element (that are the only cases when goToSelectedElement()
is called), then I want the quick outline view to be closed, even then if I detect some problem like a missing range. Otherwise, the view keeps being open and the user has no opportunity to close it (except of hitting ESC key).
Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sound sensible, but if you do not mind, I would prefer to have it in new PR, and keep this one to fix the movement in the list with the keyboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean, I should keep the close()
operation as last statement in the block if (range != null) {...}
and create PR for moving it where it is now, after if (selectedElement != null) {
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I changed the close() call as you requested and created a PR #904 for moving the close operation as I suggested.
thanks |
Listen to ESC, enter, double-click, and single click.
Improve selection listener concept.
Ensure, elements can be selected with keyboard or mouse. Extract method for revealing the selected element in editor.