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

Make file browser respond to focussed elements #13577

Merged
merged 38 commits into from
Apr 7, 2023
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
1879719
Make file browser respond to focussed elements
gabalafou Nov 23, 2022
56bb994
Do not prevent space-click on check-all checkbox
gabalafou Dec 13, 2022
89275eb
void goUp() to make the linter happy and avoid run-time errors
gabalafou Dec 13, 2022
1ba8ead
Merge branch 'master' into filebrowser-keyboard-focus
gabalafou Dec 23, 2022
798a761
undo unintended change to browser.json
gabalafou Jan 6, 2023
b9fac45
rewrite directory/file pending code
gabalafou Jan 6, 2023
942dcf3
rewrite doRename using async/await syntax
gabalafou Jan 9, 2023
62ade7f
use theme rather than system colors for focus ring on file
gabalafou Jan 10, 2023
1f79c74
unify keyboard + mouse multiSelect
gabalafou Jan 10, 2023
976cf25
doRename -> userInputForRename
gabalafou Jan 10, 2023
1e10b87
Automatic application of license header
github-actions[bot] Jan 10, 2023
e14210e
fix tests
gabalafou Jan 10, 2023
b79602f
Add comments and rewrite special case in multi select
gabalafou Jan 10, 2023
81b30d7
fix license headers
gabalafou Jan 10, 2023
3486214
remove Backspace keyboard shortcut from JSON file (now handled by class)
gabalafou Jan 10, 2023
8f8a562
only handle arrow press on item to avoid confusing UX
gabalafou Jan 11, 2023
1fe9396
Merge branch 'master' into filebrowser-keyboard-focus
gabalafou Jan 11, 2023
d9edb05
fix imports causing github test action on linux to fail
gabalafou Jan 13, 2023
edb3e0f
try to help git diff
gabalafou Feb 9, 2023
3f1fc1b
incorporate #13682 changes to listing.spec.ts
gabalafou Feb 9, 2023
67754d5
Merge branch 'master' into filebrowser-keyboard-focus
gabalafou Feb 9, 2023
ce360b7
fix test
gabalafou Feb 9, 2023
fd11744
Merge branch 'master' into filebrowser-keyboard-focus
gabalafou Feb 9, 2023
9d41cd8
oh well my attempt to make git diff prettier failed
gabalafou Feb 9, 2023
4964bdd
restore backspace (not sure why it was removed)
gabalafou Feb 9, 2023
1825188
Merge branch 'master' into filebrowser-keyboard-focus
gabalafou Feb 13, 2023
bdf0525
Revert "restore backspace (not sure why it was removed)"
gabalafou Feb 24, 2023
37663ad
Merge branch 'master' into filebrowser-keyboard-focus
gabalafou Feb 24, 2023
27b17b5
Merge branch 'master' into filebrowser-keyboard-focus
gabalafou Mar 2, 2023
63ef7cf
Merge branch 'master' into filebrowser-keyboard-focus
gabalafou Mar 7, 2023
1ce98ba
fix test timeouts
gabalafou Mar 7, 2023
0ec939a
fix linter error
gabalafou Mar 7, 2023
70266d5
Merge branch 'master' into filebrowser-keyboard-focus
gabalafou Mar 7, 2023
c762fe1
Revert "Revert "restore backspace (not sure why it was removed)""
gabalafou Mar 17, 2023
f7dffae
restore Backspace as user-configurable shortcut
gabalafou Mar 17, 2023
4e7f36b
adjust selector
gabalafou Mar 17, 2023
91408e2
Merge branch 'master' into pr/gabalafou/13577
fcollonval Apr 7, 2023
7a2f007
Fix Jest unit tests
fcollonval Apr 7, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/filebrowser-extension/schema/browser.json
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@
"title": "File Browser",
"description": "File Browser settings.",
"jupyter.lab.shortcuts": [
{
"command": "filebrowser:go-up",
"keys": ["Backspace"],
"selector": ".jp-DirListing:focus"
},
Comment on lines +139 to +143
Copy link
Contributor Author

@gabalafou gabalafou Mar 17, 2023

Choose a reason for hiding this comment

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

Okay, this seems to do the trick: just add another selector to handle the case when the directory is empty and leave the other selector alone. I wish I could leave a comment in this JSON file.

This :focus selector works for when the directory is empty because the only node (within the directory listing widget) that can receive focus at that point is the directory listing node itself. We cannot use the selector without :focus though because then it would get triggered if the user were in the middle of renaming a file, makes a typo, and then presses backspace to delete the typo.

I tested this change three ways:

  1. Opening an empty folder then pressing backspace
  2. Opening a non-empty folder then pressing backspace
  3. Renaming a file and pressing backspace while editing the file name

{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved backspace handling into the DirListing class. You can reason from the way it's configured here that it doesn't work if the directory is empty, because if it's empty there is no .jp-DirListing-itemText node.

"command": "filebrowser:go-up",
"keys": ["Backspace"],
Expand Down
27 changes: 14 additions & 13 deletions packages/filebrowser-extension/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,11 @@ const downloadPlugin: JupyterFrontEndPlugin<void> = {
Clipboard.copyToSystem(url);
});
},
isVisible: () =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "copy download link" command does not accept multiple files, so do not show it in the (right-click) context menu when multiple files are selected.

// So long as this command only handles one file at time, don't show it
// if multiple files are selected.
!!tracker.currentWidget &&
Array.from(tracker.currentWidget.selectedItems()).length === 1,
icon: copyIcon.bindprops({ stylesheet: 'menuItem' }),
label: trans.__('Copy Download Link'),
mnemonic: 0
Expand Down Expand Up @@ -986,19 +991,8 @@ function addCommands(
return;
}
const { model } = browserForPath;

await model.restored;
if (model.path === model.rootPath) {
return;
}
try {
await model.cd('..');
} catch (reason) {
console.warn(
`${CommandIDs.goUp} failed to go to parent directory of ${model.path}`,
reason
);
}
void browserForPath.goUp();
}
});

Expand Down Expand Up @@ -1181,6 +1175,11 @@ function addCommands(
return widget.rename();
}
},
isVisible: () =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no batch, multi-file "rename" command, so do not show it in the (right-click) context menu when multiple files are selected.

// So long as this command only handles one file at time, don't show it
// if multiple files are selected.
!!tracker.currentWidget &&
Array.from(tracker.currentWidget.selectedItems()).length === 1,
icon: editIcon.bindprops({ stylesheet: 'menuItem' }),
label: trans.__('Rename'),
mnemonic: 0
Expand All @@ -1200,8 +1199,10 @@ function addCommands(
Clipboard.copyToSystem(item.value.path);
},
isVisible: () =>
// So long as this command only handles one file at time, don't show it
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "copy path" command does not support multiple files, so do not show it in the (right-click) context menu when multiple files are selected.

Note: in Windows, when multiple files are selected, the copy-path command puts each path on a newline in the clipboard.

// if multiple files are selected.
!!tracker.currentWidget &&
!tracker.currentWidget.selectedItems().next().done,
Array.from(tracker.currentWidget.selectedItems()).length === 1,
icon: fileIcon.bindprops({ stylesheet: 'menuItem' }),
label: trans.__('Copy Path')
});
Expand Down
103 changes: 52 additions & 51 deletions packages/filebrowser/src/browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ export class FileBrowser extends SidePanel {
this._trans.__('file browser')
);

this._directoryPending = false;

// File browser widgets container
this.mainPanel = new Panel();
this.mainPanel.addClass(FILE_BROWSER_PANEL_CLASS);
Expand Down Expand Up @@ -226,63 +224,57 @@ export class FileBrowser extends SidePanel {
return this.listing.paste();
}

private async _createNew(
options: Contents.ICreateOptions
): Promise<Contents.IModel> {
try {
const model = await this._manager.newUntitled(options);
await this.listing.selectItemByName(model.name, true);
await this.rename();
return model;
} catch (err) {
void showErrorMessage(this._trans.__('Error'), err);
throw err;
}
}

/**
* Create a new directory
*/
createNewDirectory(): void {
if (this._directoryPending === true) {
return;
async createNewDirectory(): Promise<Contents.IModel> {
if (this._directoryPending) {
return this._directoryPending;
}
this._directoryPending = this._createNew({
path: this.model.path,
type: 'directory'
});
try {
return await this._directoryPending;
} finally {
this._directoryPending = null;
}
this._directoryPending = true;
// TODO: We should provide a hook into when the
// directory is done being created. This probably
// means storing a pendingDirectory promise and
// returning that if there is already a directory
// request.
void this._manager
.newUntitled({
path: this.model.path,
type: 'directory'
})
.then(async model => {
await this.listing.selectItemByName(model.name);
await this.rename();
this._directoryPending = false;
})
.catch(err => {
void showErrorMessage(this._trans.__('Error'), err);
this._directoryPending = false;
});
}

/**
* Create a new file
*/
createNewFile(options: FileBrowser.IFileOptions): void {
if (this._filePending === true) {
return;
async createNewFile(
options: FileBrowser.IFileOptions
): Promise<Contents.IModel> {
if (this._filePending) {
return this._filePending;
}
this._filePending = this._createNew({
path: this.model.path,
type: 'file',
ext: options.ext
});
try {
return await this._filePending;
} finally {
this._filePending = null;
}
this._filePending = true;
// TODO: We should provide a hook into when the
// file is done being created. This probably
// means storing a pendingFile promise and
// returning that if there is already a file
// request.
void this._manager
.newUntitled({
path: this.model.path,
type: 'file',
ext: options.ext
})
.then(async model => {
await this.listing.selectItemByName(model.name);
await this.rename();
this._filePending = false;
})
.catch(err => {
void showErrorMessage(this._trans.__('Error'), err);
this._filePending = false;
});
}

/**
Expand Down Expand Up @@ -310,6 +302,15 @@ export class FileBrowser extends SidePanel {
return this.listing.download();
}

/**
* cd ..
*
* Go up one level in the directory tree.
*/
async goUp() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.listing is a protected member so I can't do browserForPath.listing.goUp() in the execute function of the "filebrower:go-up" command, I need to expose the method this way and call browserForPath.goUp()

return this.listing.goUp();
}

/**
* Shut down kernels on the applicable currently selected items.
*
Expand Down Expand Up @@ -382,8 +383,8 @@ export class FileBrowser extends SidePanel {
protected mainPanel: Panel;

private _manager: IDocumentManager;
private _directoryPending: boolean;
private _filePending: boolean;
private _directoryPending: Promise<Contents.IModel> | null = null;
private _filePending: Promise<Contents.IModel> | null = null;
private _navigateToCurrentDirectory: boolean;
private _showLastModifiedColumn: boolean = true;
private _showFileSizeColumn: boolean = false;
Expand Down
Loading