Skip to content
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 unexpected click handling in File Explorer #255

Merged
merged 9 commits into from
Oct 28, 2021

Conversation

mofojed
Copy link
Member

@mofojed mofojed commented Oct 22, 2021

  • Pass the event that triggered the select through
  • Use that event to trigger whether we should focus the editor or not
  • When we don't want to focus the editor, maintain the focus
  • Fixes Unexpected File Explorer Click Handling #151

It's still focusing too much though
It shouldn't focus the editor, should keep focus in item list
@mofojed mofojed requested a review from mattrunyon October 22, 2021 15:04
Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to pull the changes and test functionality in addition to the requested changes

- Wire up a new `tabClicked` event in golden-layout
- Listen to `tabClicked` event in NotebookPanel, focus when that happens
- Explicitly call `focus()` from `ConsolePlugin` instead of relying on implicit `focus()` on shown event
- Deleted unused `_$shown` method in golden-layout
@mofojed mofojed requested a review from mattrunyon October 22, 2021 20:27
@mofojed mofojed requested a review from mattrunyon October 25, 2021 16:07
Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some issues w/ behavior I found

  • Fresh page. Single click on a file doesn't focus editor, but focuses the previewed tab. File explorer does not keep focus like it should. This seems to only be on first click
  • Single clicking an already selected file deselects it (inconsistent w/ vscode). In VSCode, it keeps it selected and does nothing really
  • Single clicking a file, clicking out (in console for example), then single clicking again deselects. Similar to above
  • Open a file (in preview or full mode) and then single clicking on the file name again doesn't maintain focus in the file explorer
  • Double clicking a file that is already open when it is not blurred selected in the file explorer results in no selection in file explorer

Something is off w/ the logic. Should be

  1. Single click always brings file tab into view (open preview if necessary). Always leave focus in file explorer
  2. Double click always brings file tab into view (open or promote from preview if necessary). Leave blurred focus in file explorer. Focus editor.

@mofojed
Copy link
Member Author

mofojed commented Oct 26, 2021

@dsmmcken Just want to confirm single click should keep it selected. Makes sense to me, but we explicitly deselect in that scenario in CommandHistory.

@dsmmcken
Copy link
Contributor

dsmmcken commented Oct 26, 2021

Yeah, that's fine for this. Especially if selection also changes depending on tab selected.

I general though, I like being able to de-select things.

@mofojed mofojed self-assigned this Oct 27, 2021
@mofojed mofojed added bug Something isn't working web-client-ui labels Oct 27, 2021
- The .append causes the existing elements to be added to a fragment outside of the document, so focus is lost
- Just add the focus back after appending the child.
- Clean up isFocusOnShow default value
@mofojed mofojed requested a review from mattrunyon October 28, 2021 13:29
@mofojed
Copy link
Member Author

mofojed commented Oct 28, 2021

@mattrunyon I think it should be good now, but I'm not really sure what you mean by your last checkbox item:

Double clicking a file that is already open when it is not blurred selected in the file explorer results in no selection in file explorer

Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last comment meant the faded selection state where the item was previously selected and then the panel was blurred.

Everything looks to be working properly now.

I did notice a few very minor inconsistencies, but I don't think they are due to this element/handling logic.

  1. Right clicking a file followed by left clicking the file does not select the file. I think that's a context menu problem. Pretty sure it creates an invisible overlay for click outside detection.
  2. Entering the rename dialog and then pressing escape/enter blurs the file input focus. In VSCode it maintains focus and just exits the rename input.

The only annoyance someone might face is if they want to rename several files in succession and use the keyboard to do it since confirming a rename also blurs input. We can make that a separate ticket since this one relates to click handling.

@mofojed mofojed merged commit 302b298 into deephaven:main Oct 28, 2021
@mofojed mofojed deleted the 151-file-explorer-click branch October 28, 2021 18:20
@mofojed mofojed mentioned this pull request Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working web-client-ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected File Explorer Click Handling
3 participants