Skip to content

Fix: Support Multi-selection in ctrl way #125

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 7, 2022
Merged

Fix: Support Multi-selection in ctrl way #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 7 additions & 29 deletions ui/src/components/Canvas.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ const ScopeNode = memo<Props>(({ data, id, isConnectable }) => {
const [frame] = React.useState({
translate: [0, 0],
});
const selected = useStore(store, (state) => state.selected);
const selected = useStore(store, (state) => state.pods[id]?.selected);
const role = useStore(store, (state) => state.role);

const onResize = useCallback(({ width, height, offx, offy }) => {
Expand Down Expand Up @@ -154,7 +154,7 @@ const ScopeNode = memo<Props>(({ data, id, isConnectable }) => {
id="right"
isConnectable={isConnectable}
/>
{selected === id && role !== RoleType.GUEST && (
{selected && role !== RoleType.GUEST && (
<Moveable
target={target}
resizable={true}
Expand Down Expand Up @@ -339,7 +339,7 @@ const CodeNode = memo<Props>(({ data, id, isConnectable }) => {
const isRightLayout = layout === "right";
const { setNodes } = useReactFlow();
// const selected = useStore(store, (state) => state.selected);
const setSelected = useStore(store, (state) => state.setSelected);
const setPodSelected = useStore(store, (state) => state.setPodSelected);
const setCurrentEditor = useStore(store, (state) => state.setCurrentEditor);
const getPod = useStore(store, (state) => state.getPod);
const pod = getPod(id);
Expand Down Expand Up @@ -494,7 +494,7 @@ const CodeNode = memo<Props>(({ data, id, isConnectable }) => {
onClick={(e) => {
// If the node is selected (for resize), the cursor is not shown. So
// we need to deselect it when we re-focus on the editor.
setSelected(null);
setPodSelected(id, false);
setNodes((nds) =>
applyNodeChanges(
[
Expand Down Expand Up @@ -717,6 +717,8 @@ export function Canvas() {
},
level: 0,
extent: "parent",
//otherwise, throws a lot of warnings, see https://reactflow.dev/docs/guides/troubleshooting/#only-child-nodes-can-use-a-parent-extent
parentNode: undefined,
dragHandle: ".custom-drag-handle",
};

Expand Down Expand Up @@ -782,18 +784,6 @@ export function Canvas() {
},
});
}
setNodes((nds) =>
applyNodeChanges(
[
{
id: node.id,
type: "select",
selected: true,
},
],
nds
)
);
},
[nodesMap, setNodes, userColor]
);
Expand Down Expand Up @@ -888,19 +878,6 @@ export function Canvas() {

nodesMap.set(node.id, currentNode);
}

setNodes((nds) =>
applyNodeChanges(
[
{
id: node.id,
type: "select",
selected: true,
},
],
nds
)
);
},
// We need to monitor nodes, so that getScopeAt can have all the nodes.
[
Expand Down Expand Up @@ -970,6 +947,7 @@ export function Canvas() {
nodesDraggable={role !== RoleType.GUEST}
// disable node delete on backspace when the user is a guest.
deleteKeyCode={role === RoleType.GUEST ? null : "Backspace"}
multiSelectionKeyCode={"Control"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not work on Mac, because control triggers right-click context menu. The default "Meta" works better (which is the command key).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I propose to disable option 1 and use "shift" for option 2, i.e.,

+          selectionKeyCode={null}
+          multiSelectionKeyCode={"Shift"}

Ref: #112 (comment)

>
<Box>
<MiniMap
Expand Down
17 changes: 9 additions & 8 deletions ui/src/lib/nodes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export function useNodesStateSynced(nodeList) {
const getPod = useStore(store, (state) => state.getPod);
const deletePod = useStore(store, (state) => state.deletePod);
const updatePod = useStore(store, (state) => state.updatePod);
const setSelected = useStore(store, (state) => state.setSelected);
const setPodSelected = useStore(store, (state) => state.setPodSelected);
const role = useStore(store, (state) => state.role);
const ydoc = useStore(store, (state) => state.ydoc);
const nodesMap = ydoc.getMap<Node>("pods");
Expand All @@ -35,17 +35,17 @@ export function useNodesStateSynced(nodeList) {
}

changes.forEach((change) => {
if (!isNodeAddChange(change) && !isNodeResetChange(change)) {
if (!isNodeAddChange(change)) {
if (isNodeRemoveChange(change)) {
nodesMap.delete(change.id);
return;
}
const node = nextNodes.find((n) => n.id === change.id);

if (!node) return;

if (change.type === "select" && change.selected) {
// FIXME: consider the case where only unselect is called
setSelected(node.id);
if (isNodeResetChange(change) || change.type === "select") {
setPodSelected(node.id, change.selected as boolean);
return;
}

Expand Down Expand Up @@ -98,10 +98,11 @@ export function useNodesStateSynced(nodeList) {

// TOFIX: a node may be shadowed behind its parent, due to the order to render reactflow node, to fix this, comment out the following sorted method, which brings in a large overhead.
setNodes(
Array.from(nodesMap.values()).sort(
(a: Node & { level }, b: Node & { level }) => a.level - b.level
)
Array.from(nodesMap.values())
.sort((a: Node & { level }, b: Node & { level }) => a.level - b.level)
.map((node) => ({ ...node, selected: getPod(node.id)?.selected }))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the only way to un-sync the nodesMap[id].selected field from yjs? For example, is it possible to still use nodesMap[id].selected, but stop applying the sync (in either observer or useNodesStateSynced::onNodesChanges)?

Managing the field in Zustand state sounds a bit over-complicated to me. And it looks like we have to implement the de-select logic when we do this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only ground truth source is nodesMap, which is shared by all users. We have to maintain an individual selected state for each user. Actually, we have already implemented de-selection.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I got what you meant. I realized its cons just now: new selection change will not be applied if no update in the remote nodesMap, un-selecting a node by clicking on the pane is a very typical case in which no update is written into nodesMap, so it will not sync the selection change in time.

I have an idea about the possible solution: stop writing selected fields of nodes into nodesMap, keep a selectedNode set in store, observe any update both on nodesMap and selectedNode to compose the complete data of nodes. But notice nodes in reactflow is just a list not map, which may cause some performance issue.

);

// setNodes(Array.from(nodesMap.values()));
};

Expand Down
26 changes: 18 additions & 8 deletions ui/src/lib/store.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ const initialState = {
socket: null,
socketIntervalId: null,
// keep different seletced info on each user themselves
selected: null,
// to fixed maco editor command bug
currentEditor: null,
//TODO: all presence information are now saved in clients map for future usage. create a modern UI to show those information from clients (e.g., online users)
Expand Down Expand Up @@ -123,6 +122,7 @@ export type Pod = {
ns?: string;
running?: boolean;
focus?: boolean;
selected?: boolean;
};

export interface RepoSlice {
Expand Down Expand Up @@ -180,21 +180,20 @@ export interface RepoSlice {
}) => void;
setPodPosition: ({ id, x, y }: any) => void;
setPodParent: ({ id, parent }: any) => void;
selected: string | null;
setSelected: (id: string | null) => void;
currentEditor: string | null;
setCurrentEditor: (id: string | null) => void;
setUser: (user: any) => void;
addClient: (clientId: any, name, color) => void;
deleteClient: (clientId: any) => void;
flipShowLineNumbers: () => void;
disconnect: () => void;
getPod: (string) => Pod;
getPod: (id: string) => Pod;
getPods: () => Record<string, Pod>;
getId2children: (string) => string[];
setPodVisibility: (id, visible) => void;
setPodFocus: (id) => void;
setPodBlur: (id) => void;
setPodVisibility: (id: any, visible: any) => void;
setPodFocus: (id: string) => void;
setPodBlur: (id: string) => void;
setPodSelected: (id: string, target: boolean) => void;
}

type BearState = RepoSlice & RuntimeSlice;
Expand Down Expand Up @@ -244,7 +243,6 @@ const createRepoSlice: StateCreator<
setSessionId: (id) => set({ sessionId: id }),
addError: (error) => set({ error }),
clearError: () => set({ error: null }),
setSelected: (id) => set({ selected: id }),
setCurrentEditor: (id) => set({ currentEditor: id }),
addPod: async (
client,
Expand Down Expand Up @@ -286,6 +284,8 @@ const createRepoSlice: StateCreator<
// the backend instead.
children: [],
io: {},
selected: false,
focus: false,
// from payload
parent,
index,
Expand Down Expand Up @@ -835,6 +835,16 @@ const createRepoSlice: StateCreator<
}
})
),
setPodSelected: (id: string, target: boolean) => {
set(
produce((state) => {
if (state.pods[id]) {
state.pods[id].selected = target;
}
})
);
},
getPodSelected: (id: string) => get().pods[id]?.selected as boolean,
});

export const createRepoStore = () =>
Expand Down