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

Delete Object Keyboard Shortcut #388

Merged
merged 8 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
47 changes: 47 additions & 0 deletions packages/base/src/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
ISelection,
Parts
} from '@jupytercad/schema';
import { CommandRegistry } from '@lumino/commands';
import { JupyterFrontEnd } from '@jupyterlab/application';
import { showErrorMessage, WidgetTracker } from '@jupyterlab/apputils';
import { ITranslator } from '@jupyterlab/translation';
Expand All @@ -33,9 +34,11 @@ import {
chamferIcon,
filletIcon
} from './tools';
import keybindings from './keybindings.json';
import { JupyterCadPanel, JupyterCadWidget } from './widget';
import { DocumentRegistry } from '@jupyterlab/docregistry';
import { PathExt } from '@jupyterlab/coreutils';
import { handleRemoveObject } from './panelview';

export function newName(type: string, model: IJupyterCadModel): string {
const sharedModel = model.sharedModel;
Expand Down Expand Up @@ -638,6 +641,22 @@ const EXPORT_FORM = {
}
};

function loadKeybindings(commands: CommandRegistry, keybindings: any[]) {
keybindings.forEach(binding => {
commands.addKeyBinding({
command: binding.command,
keys: binding.keys,
selector: binding.selector
});
});
}

function getSelectedObjectId(widget: JupyterCadWidget): string {
const selected =
widget.context.model.sharedModel.awareness.getLocalState()?.selected;
return selected ? Object.keys(selected.value)[0] : '';
}

/**
* Add the FreeCAD commands to the application's command registry.
*/
Expand Down Expand Up @@ -714,6 +733,31 @@ export function addCommands(
}
});

commands.addCommand(CommandIDs.removeObject, {
label: trans.__('Remove Object'),
isEnabled: () => {
const current = tracker.currentWidget;
return current ? current.context.model.sharedModel.editable : false;
},
execute: () => {
const current = tracker.currentWidget;
if (!current) {
return;
}

const objectId = getSelectedObjectId(current);
if (!objectId) {
console.warn('No object is selected.');
return;
}
const sharedModel = current.context.model.sharedModel;

handleRemoveObject(objectId, sharedModel, () =>
sharedModel.awareness.setLocalStateField('selected', {})
);
}
});

commands.addCommand(CommandIDs.newBox, {
label: trans.__('New Box'),
isEnabled: () => {
Expand Down Expand Up @@ -953,6 +997,7 @@ export function addCommands(
await dialog.launch();
}
});
loadKeybindings(commands, keybindings);
}

/**
Expand All @@ -964,6 +1009,8 @@ export namespace CommandIDs {

export const newSketch = 'jupytercad:sketch';

export const removeObject = 'jupytercad:removeObject';

export const newBox = 'jupytercad:newBox';
export const newCylinder = 'jupytercad:newCylinder';
export const newSphere = 'jupytercad:newSphere';
Expand Down
7 changes: 7 additions & 0 deletions packages/base/src/keybindings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[
{
"command": "jupytercad:removeObject",
"keys": ["Accel X"],
Copy link
Member

Choose a reason for hiding this comment

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

I feel like "Accel X" should be the keybinding for "cut", though we don't have any copy/paste logic available so I'm fine keeping this keybinding for now if you like 👍🏽

Though should we also add "Delete" there too in the list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that makes much sense as xref:QuantStack/jupytergis#68 also uses the [Delete] key for the same.

Also it sounds nice to have a feature request open for cut/copy/paste logic. I can open one if it's fine.

Copy link
Member

Choose a reason for hiding this comment

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

I can open one if it's fine

Sounds good!

"selector": ".jp-jupytercad-panel"
Copy link
Member

Choose a reason for hiding this comment

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

This results in the key binding to not be triggered if it's the left panel (object list) that has focus.

I wonder if we should add the keybinding to the #main application instead. Though I tested locally and that does not seem to work, I suppose it's because the command depends on the WidgetTracker<JupyterCadWidget> tracker, and that one only tracks the 3D view if I'm correct.

Let's not consider this a blocker for merging the PR, but let's keep track of this in an issue after merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, i can try adding the valid className for left panel too and see if the union of jupytercad-panel & leftpanel works.

Copy link
Member Author

@arjxn-py arjxn-py Jul 29, 2024

Choose a reason for hiding this comment

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

I thought it was easy.
But it was not as simple as I thought 😅, there seem to be a complexity with the Document Object Model that all the components are not naturally focusable on click, which was not letting me to have a specific component as a selector.
But at last it was done by defining tabIndex={0} & one can now delete object with key binding while being on ObjectTree too.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

}
]
92 changes: 53 additions & 39 deletions packages/base/src/panelview/objecttree.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,47 @@ interface IProps {
cpModel: IControlPanelModel;
}

export const handleRemoveObject = (
objectId: string,
sharedModel: any,
syncSelected: () => void
): void => {
if (!sharedModel) {
return;
}

const dependants = sharedModel.getDependants(objectId);

let body: React.JSX.Element;
if (dependants.length) {
body = (
<div>
{'Removing this object will also result in removing:'}
<ul>
{dependants.map(dependant => (
<li key={dependant}>{dependant}</li>
))}
</ul>
</div>
);
} else {
body = <div>Are you sure?</div>;
}

showDialog({
title: `Removing ${objectId}`,
body,
buttons: [Dialog.okButton(), Dialog.cancelButton()]
}).then(({ button: { accept } }) => {
if (accept) {
const toRemove = dependants.concat([objectId]);
sharedModel.removeObjects(toRemove);
}
});

syncSelected();
};

class ObjectTreeReact extends React.Component<IProps, IStates> {
constructor(props: IProps) {
super(props);
Expand Down Expand Up @@ -265,6 +306,17 @@ class ObjectTreeReact extends React.Component<IProps, IStates> {
this.setState(old => ({ ...old, options: sender.options }));
};

private _handleRemoveObject = (objectId: string): void => {
const sharedModel = this.props.cpModel.jcadModel?.sharedModel;
if (!sharedModel) {
return;
}

handleRemoveObject(objectId, sharedModel, () => {
this.props.cpModel.jcadModel?.syncSelected({});
});
};

render(): React.ReactNode {
const { selectedNodes, openNodes, options } = this.state;
const data = this.stateToTree();
Expand Down Expand Up @@ -394,45 +446,7 @@ class ObjectTreeReact extends React.Component<IProps, IStates> {
className={'jp-ToolbarButtonComponent'}
onClick={() => {
const objectId = opts.node.parentId as string;
const sharedModel =
this.props.cpModel.jcadModel?.sharedModel;
if (!sharedModel) {
return;
}

const dependants =
sharedModel.getDependants(objectId);

let body: React.JSX.Element;
if (dependants.length) {
body = (
<div>
{
'Removing this object will also result in removing:'
}
<ul>
{dependants.map(dependant => (
<li>{dependant}</li>
))}
</ul>
</div>
);
} else {
body = <div>Are you sure?</div>;
}

showDialog({
title: `Removing ${objectId}`,
body,
buttons: [Dialog.okButton(), Dialog.cancelButton()]
}).then(({ button: { accept } }) => {
if (accept) {
const toRemove = dependants.concat([objectId]);
sharedModel.removeObjects(toRemove);
}
});

this.props.cpModel.jcadModel?.syncSelected({});
this._handleRemoveObject(objectId);
}}
icon={closeIcon}
/>
Expand Down
7 changes: 6 additions & 1 deletion packages/base/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,10 @@
"outDir": "lib",
"rootDir": "src"
},
"include": ["src/**/*", "src/schema/*.json", "src/_interface/*.json"]
"include": [
"src/**/*",
"src/schema/*.json",
"src/_interface/*.json",
"src/*.json"
]
}
Loading