Implement the code formatting shortcut

[trevinhofmann]

Overview

This pull request implements #1957 by adding a shortcut to format selected text as code.

It currently uses the Bold icon as a placeholder. When the designer creates a dedicated icon and animation (most likely a </> icon), we can replace the placeholder.

Since the c key is already used for multiple shortcuts, this change uses k for the keyboard shortcut.

We are not able to use the same document.queryCommandState, so I updated getCommandState to also work for selected text for determining whether the shortcut icon should be active or inactive. The only change needed to make it work was to consider when the cursor is within text but not selecting any, which returns selected HTML such as <b><code></code></b> without any non-tag characters. With document.queryCommandState also being deprecated and non-standard, I also switched to using getCommandState for selected text (rather than only an entire selected thought) for the other commands as well.

We also cannot use document.execCommand within the formatSelection action, so I added a new formatWithTag action that manually adds/removes the provided tag. This is currently only used for <code>, but it could be reused for other commands that are not supported by document.execCommand.

Demo

Screen.Recording.2024-11-22.at.10.50.57.PM.mov

[raineorshine]

Awesome! This looks fantastic. Thanks for your thoroughness, and for fitting it in with the other formatting commands.

With document.queryCommandState also being deprecated and non-standard, I also switched to using getCommandState for selected text (rather than only an entire selected thought) for the other commands as well.

That sounds fine, thanks.

Fwiw, I did some research on queryCommandState before adding it to the codebase. Despite its deprecation status, it is very well supported and unlikely to be dropped. But if we have reliable alternatives that are performant and offer the same behavior then I'm happy to use them.

---

The only issue I found is that it does work in Context View. While Context View is admittedly incomplete, other formatting commands like bold, italic, underline do work there, so I think it's appropriate to add support for format as code. (Notably, text color does not work in Context View.)

Steps to Reproduce

- a
  - m
    - x
- b
  - m
    - y

1. Set the cursor on a/m
2. Activate Context View.
3. Set the cursor on a/m~/a.
4. Format as code.

Current Behavior

- a
  - <code>a</code>
    - x
- b
  - m
    - y

Expected Behavior

- <code>a</code>
  - m
    - x
- b
  - m
    - y

[trevinhofmann]

Thanks for the review!

It appears that simplifyPath may have an issue. When the cursor is pointing to a/m~/a, the returned simple path points to a->m rather than pointing to a. That caused us to update the wrong thought.

One fix for this which worked was to change

em/src/selectors/lastThoughtsFromContextChain.ts

Line 51 in e39ebc4

return appendToPath(contextPath, ...simplePath.slice(1))

to:

return appendToPath(contextPath, ...simplePath)

However, I am less familiar with this part of the code and I'm currently unable to determine if that is the correct fix or if it will cause other regressions. It does cause several unit tests related to the context view to fail, so I assume this is not the correct approach.

Rather than diving deeper into simplifyPath, I found thoughtToPath as an alternative that appears to work as expected. I've made the change in 092549b. Here is a demo of the corrected functionality:

Screen.Recording.2024-11-30.at.12.02.47.PM.mov

[raineorshine]

It appears that simplifyPath may have an issue. When the cursor is pointing to a/m~/a, the returned simple path points to a->m rather than pointing to a. That caused us to update the wrong thought.

Yes, that is confusing. The reason that simplifyPath behaves that way is that for most purposes, a/m is actually the correct thought to operate on when the cursor is on a/m~/a. Each of the rendered children of the context view a/m~ represents a context that m appears in (i.e. parents of thoughts that match the Lexeme m). m appears in a and b, so those are rendered in the context view. When a user activates Delete Thought on cursor a/m~/a, for example, the expected behavior is not to delete a as you might think; it is to delete m from the context a. That is, in order to remove a from the rendered contexts, it must no longer be a parent of one of the contexts of the m Lexeme, and the way to do that is to delete a/m.

Of course, this logic is wrong when formatting a thought, as we expect the visible thought a to become bold, italic, code, etc.

The root cause is that the context view has ambiguous semantics due to it being the transitional space between two different parts of the tree. The particular SimplePath that a context view thought should be resolved to will depend on the command that is being executed.

The design of the cursor as a linear sequence of thoughts that can be resolved into a sequence of nested context views is possibly not the best architecture. If I were to redesign it, I might store the contextChain in the store and have different methods for getting the current thought depending on the circumstance. However, the basic ambiguity of operations in the context view will always make this a somewhat contested space.

Rather than diving deeper into simplifyPath, I found thoughtToPath as an alternative that appears to work as expected. I've made the change in 092549b.

That should be fine. Another way is parentOf(simplifyPath(state, cursor)) when in the context view.

[raineorshine]

The unit tests are stalling for some reason. I thought it was just a flaky CI run, but I re-ran it several times and got the same result, and then confirmed that they halt in the same place on my local machine. Also confirmed that it does not occur in main.

Let me know if you're able to reproduce. Thanks!

[trevinhofmann]

I am able to reproduce it locally, and it is very strange. The tests appear successful but some stall indefinitely. I would expect there to be a relatively short timeout, but the tests stall until I forcefully quit. Quitting them produces these error messages, which don't make add much value for me:

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Errors ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

Vitest caught 2 unhandled errors during the test run.
This might cause false positive tests. Resolve unhandled errors to make sure your tests are not affected.

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Error ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Error: Terminating worker thread
 ❯ Object.ThreadTermination node_modules/tinypool/dist/index.js:489:28
 ❯ node_modules/tinypool/dist/index.js:540:30
 ❯ process.processTicksAndRejections node:internal/process/task_queues:95:5


⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Error ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Error: Terminating worker thread
 ❯ Object.ThreadTermination node_modules/tinypool/dist/index.js:489:28
 ❯ node_modules/tinypool/dist/index.js:540:30
 ❯ process.processTicksAndRejections node:internal/process/task_queues:95:5

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 Test Files  123 passed | 9 skipped (134)
      Tests  1077 passed | 61 skipped (1171)
     Errors  2 errors
   Start at  12:10:50
   Duration  142.19s (transform 10.18s, setup 46.42s, collect 241.61s, tests 43.74s, environment 110.80s, prepare 19.18s)

 FAIL  Tests failed. Watching for file changes...
       press h to show help, press q to quit

Through some bisecting / trial-and-error, I found the apparent culprit to be selection.isOnThought() in the commandStateStore. I still don't understand why that would cause tests to time out, but I found a suitable replacement for the desired functionality by checking if selection.html() is empty. I tried out some others such as selection.isThought(), selection.isEditable(), and state.editing, but they do not consistently give the value needed.

What we're trying to determine is whether the selection is only part of a thought or the entire thought, for deciding whether the command buttons should be active. I'd be happy to consider an alternative if you're familiar with a better option, but this change is relatively simple, working as expected, and passing the tests: 0b73af9

[raineorshine]

Thanks, that sounds like a reasonable solution. However, I think isOnThought will come back to bite us if we leave it as is.

I was already thinking it was probably an infinite loop, because the tests could only be quit by killing the process. Thanks for narrowing it down to isOnThought. Try this:

diff --git a/src/device/selection.ts b/src/device/selection.ts
index 485d3065665..f01c4371a8e 100644
--- a/src/device/selection.ts
+++ b/src/device/selection.ts
@@ -73,8 +73,7 @@ export const isThought = (): boolean => {
 /** Returns true if the selection is on a thought. */
 export const isOnThought = (): boolean => {
   let focusNode = window.getSelection()?.focusNode
-  if (!focusNode) return false
-  while ((focusNode as HTMLElement)?.tagName !== 'DIV') {
+  while (focusNode && (focusNode as HTMLElement)?.tagName !== 'DIV') {
     if (isEditable(focusNode)) return true
     focusNode = focusNode?.parentNode
   }