[ZEPPELIN-6429] Focus paragraph editor on clone/insert in New UI#5267
[ZEPPELIN-6429] Focus paragraph editor on clone/insert in New UI#5267voidmatcha wants to merge 1 commit into
Conversation
|
@voidmatcha By the way, is the CI failure irrelevant? |
|
@jongyoul The job failed is only relevant to the legacy frontend app, so we could ignore it for here. |
There was a problem hiding this comment.
Thanks for working on this. It works as intended, I just have a suggestion about the approach.
localAddFocusPending (message.service.ts:43) assumes the next PARAGRAPH_ADDED is the one caused by this client's own request. That's fine for a single user, but it can break with more than one client on the same note. For example: A clicks clone (flag set to true), and before A's own response returns, a PARAGRAPH_ADDED for a paragraph that user B just added arrives first. A's flag gets consumed by B's paragraph, so A's cursor lands on a paragraph A never created. The root issue is that the client can't tell whether an incoming PARAGRAPH_ADDED came from its own request, so it has to guess.
The msgId we already pass around solves this cleanly. The client sends a msgId on every request (message.ts:165), the server knows it (Message.java:234-239), and several broadcasts already echo it back (e.g. broadcastParagraph, NotebookServer.java:673-674). The only gap is broadcastNewParagraph (NotebookServer.java:695-701), so it's a small change, the same thing broadcastParagraph already does:
// NotebookServer.java:695: take a msgId and pass it through (same as broadcastParagraph)
Message message = new Message(OP.PARAGRAPH_ADDED)
.withMsgId(msgId) // <-- this line
.put("paragraph", para).put("index", paraIndex);The server then only reports which request an event came from, and each client decides for itself whether to focus by checking if the msgId is its own:
// focus only if the incoming PARAGRAPH_ADDED's msgId is one this client sent
// (the msgId contains a client id, so match by prefix, or track the msgIds you send)
if (isOwnMessage(msgId)) {
// focus the new paragraph's editor
}So before going with the heuristic, what do you think about doing it this way instead? It already works today, so this is really about the approach, and I'm happy to help with it.
What is this PR for?
After cloning or inserting a paragraph in the New UI, the cursor stays on the wrapper element instead of the editor, so you have to click before typing. This focuses the new paragraph's editor one tick after
PARAGRAPH_ADDED, gated to clone/insert initiated by this client so auto-append on run and other clients' inserts don't steal focus. It also skips dirty-marking on programmatic editorsetValue(isFlush) so the cloned content isn't discarded. (The clone content loss itself is handled separately in #5254 (review); this covers the cursor part.)What type of PR is it?
Bug Fix
Todos
What is the Jira issue?
ZEPPELIN-6429
How should this be tested?
Screenshots (if appropriate)
M0_COMPARISON_master_asis-vs-tobe.mp4
Questions: