-
Notifications
You must be signed in to change notification settings - Fork 18
Feat: cross-page single-pod copy-paste in mouse-bound ctrl-v way
#158
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
Feat: cross-page single-pod copy-paste in mouse-bound ctrl-v way
#158
Conversation
ui/src/components/Canvas.tsx
Outdated
| style, | ||
| }; | ||
|
|
||
| addPod(apolloClient, { |
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.
In the future, we can defer the write to DB until the final drop of the node (when we allow a user to press Esc to cancel the paste). No need to implement that now because that seems to require non-trivial logic. Adding a TODO comment is good enough.
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.
Also, since we called addPod in observer.YMapEvent.changes:
Lines 106 to 111 in a67b5c8
| const observer = (YMapEvent) => { | |
| YMapEvent.changes.keys.forEach((change, key) => { | |
| if (change.action === "add") { | |
| const node = nodesMap.get(key); | |
| if (!node) return; | |
| addPod(null, { |
I think all addPod calls in Canvas.tsx are duplicate, right? Should we remove them? I.e. this line (Line 1170 in this PR) and the following line (Line 901 in main):
codepod/ui/src/components/Canvas.tsx
Line 901 in a67b5c8
| addPod(apolloClient, { |
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.
addPod calls in Canvas.tsx are duplicate, right?
That depends on the behavior of ymap.observe. It is triggered if "other users" make the change. Would it get triggered as well if the "local user" makes the change?
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.
In the future, we can defer the write to DB until the final drop of the node
In fact, when a user press the shortcut, the half-transparent node is immediately added to the pods array in local store, and nodesMap, but have not been synced to the DB (see the addpod arguments, I set the dirty false, doRemoteAdd will not be executed.
When the user determines the destination by clicking, I delete the temporary transparent node in nodesMap, and add a new formal node with everything identical but style.opacity and clientId removed, then write it formally to the DB.
ymap.observe on adding events here serves like the way in which we use useEffect to sync pod.name and pod.position before, only sync the data in nodesMap with local store.pods on each user's own side.
To avoid the duplicated pod adding in local store, I check if the pod is already in store.pods here: https://github.com/li-xin-yi/codepod-1/blob/experimental%2Fpaste-on-canvas/ui/src/lib/nodes.tsx#L114
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.
Sorry, my bad, I noticed that I set the dirty=true for the temporary pod adding just now. I will fix it.
| nodesMap.set(id, newNode as any); | ||
| setPasting(id); |
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.
If we set nodesMap, it would automatically sync with collaborators, right? UPDATE resolved; I saw that you checked clientID in various places in lib/nodes.tsx. That's a clever trick, looks good to me!
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, it is an adding event in nodesMap, usually, ymap.observe triggers a addPod call without remote DB writing, just adds it to the store.pods in each user's side. In this case, since I add the checking statement for clientId, collaborators will ignore it and won't call addPod until we delete the temporary floating transparent node and re-add the formal node.
| node.position = position; | ||
| nodesMap.set(pasting, node); | ||
| }; | ||
| const mouseClick = (event) => { |
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.
It would be helpful if the user could cancel the pasting before the drop by pressing Esc.
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.
I agree, sounds great, I will work on this feature later.
LGTM
LGTM |
LGTM. Let's forget about the right-click context menu for now. |
|
Update:
Code still needs cleaning up and performance improvement. |
|
Update:
To-do:
|
|
Thanks, all look and work great! |
|
Updata: Clean up the code
|
ctrl-v way
lihebi
left a comment
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.
Awesome, thanks!
| y: position.y, | ||
| width: style.width, | ||
| height: style.height, | ||
| dirty: true, |
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.
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.
The dirty parameter is removed in store.addPod but somehow remains here. It's a bug but shouldn't have any effect on addPod. Can you show me how to reproduce the error?
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.
It does not reproduce on localhost. If you go to app.codepod.io, creating pods would sometime fail to sync.
But never mind, I've fixed it.

[Done] This PR is finished, ready to review now.
(Only support one single code pod by now,
Ctrl-Cis not supported yet)Related to: #155
I gave up
clipboard.read/write, removed the paste option in right-click menu, tested on Chrome, Edges, and Firefox, all works well.How to use:
Ctrl-V, a duplicated pod shows up in the center of the canvas (FIXME: because the cursor may not be in the canvas when pasting, so I just choose the center as the initial position, we can discuss the detailed rule for the initial position later)Don't merge, just a demo to convey my idea and for test usages from others. The code is too dirty now, if you agree on this idea, I can continue to improve the implementation.