improve: Move highlighted code to newly created cells #2484

wasimsandhu · 2024-10-03T05:42:36Z

📝 Summary

Implements #2474.

🔍 Description of Changes

When a new cell is created, any highlighted code is cut from the original cell and pasted into the new cell.

📋 Checklist

I have read the contributor guidelines.
For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
I have added tests for the changes made.
I have run the code and verified that it works as expected.

📜 Reviewers

@akshayka OR @mscolnick

vercel · 2024-10-03T05:42:39Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
marimo-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 5, 2024 10:40pm
marimo-storybook	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 5, 2024 10:40pm

akshayka

Nice QOL improvement! One thing — after splitting, both cells should be marked as stale

wasimsandhu · 2024-10-03T06:54:33Z

@akshayka Good catch thanks!

mscolnick · 2024-10-03T13:10:34Z

@wasimsandhu - can you verify this works with SQL and markdown? We probably have to do something similar to splitEditor. could we also write a test similar to splitEditor(utils.test.ts) and splitCell (cells.test.ts)

mscolnick · 2024-10-03T13:42:55Z

frontend/src/core/cells/cells.ts

@@ -214,17 +216,24 @@ const {
        : state.cellIds.topLevelIds.indexOf(cellId);
    const insertionIndex = before ? index : index + 1;

+    let cellContents = code;
+    const cellEditorView = getCellEditorView(cellId as CellId);


i think we should pass a param to createNewCell whether we should includeSelectionAsInitialCode.

we run createNewCell in many places and this would be weird for most cases (e.g. snippets, data-source panel, AI cell, etc).

could we also assert code and includeSelectionAsInitialCode are never both true? there is a typescript way to do this:

- code?: string; + code?: string | {type: 'from-selection', cellId: CellId}

edit: Implemented includeSelectionAsInitialCode, but not sure I understand the type for code

wasimsandhu · 2024-10-04T05:21:02Z

can you verify this works with SQL and markdown?

When editing a SQL/markdown cell and triggering a create new cell above/below (via button or shortcut), a Python cell is created. Do we want to change this behavior so that a cell of the same type is created? (This makes sense to me.)

mscolnick · 2024-10-04T11:30:32Z

When editing a SQL/markdown cell and triggering a create new cell above/below (via button or shortcut), a Python cell is created. Do we want to change this behavior so that a cell of the same type is created? (This makes sense to me.)

This might take some getting used to. I would keep it python for new cells, but keep the same language for "yanked" cells. thoughts? is that weird ux?

mscolnick · 2024-10-04T14:48:34Z

frontend/src/core/cells/cells.ts

+      const cellEditorView = getCellEditorView(id);
+      if (cellEditorView) {
+        const [highlighted, leftover] = extractHighlightedCode(cellEditorView);
+        cellContents = highlighted;


we should only do this this if highlighted is not empty

can you add a couple test cases for this?

createCell when includeSelectionAsInitialCode is true and there is selection

createCell when includeSelectionAsInitialCode is false and there is selection

createCell when includeSelectionAsInitialCode is true and there is no selection

createCell when includeSelectionAsInitialCode is false and there is no selection

i think it will catch small bugs like state.cellData[id].edited=true being set incorrectly.

also, doing state.cellData[id].edited is mutable, we will need to be immutable when using react.

state.cellData = { ...state.cellData, [id] : { ...state.cellData[id], code: ..., edited: ... } }

wasimsandhu · 2024-10-05T04:41:42Z

This might take some getting used to. I would keep it python for new cells, but keep the same language for "yanked" cells.

Works for me!

mscolnick · 2024-10-05T13:03:38Z

frontend/src/core/cells/__tests__/cells.test.ts

@@ -1276,4 +1278,130 @@ describe("cell reducer", () => {
    const cell = cells[0];
    expect(cell.consoleOutputs).toEqual([STDOUT1, STDOUT2]);
  });
+
+  it("can create a cell using selected code", () => {


can you put each of these in there own tests and share setup code?

describe("can create a cell using selected code" beforeEach(() => { // setup code }) it("can create when includeSelectionAsInitialCode is true and there is selection", () => { ...test one }) ... });

this makes it easier to follow given the same initial setup.

could you also:

verify edited state

do a test with markdown includeSelectionAsInitialCode - i noticed a bug when i add a cell above/below a markdown cell with includeSelectionAsInitialCode=true, it says it is edited

mscolnick · 2024-10-05T13:05:56Z

frontend/src/core/cells/cells.ts

+            [id]: {
+              ...state.cellData[id],
+              code: leftover,
+              edited: true,


this might need to be edited: Boolean(leftover) && state.cellData[id].code !== state.cellData[id].code,

i noticed a bug with markdown with no selection being made stale, it would be great to prove this in a test: get the test to fail first and then fix this code so the tests passes

made a suggestion above that you may not need this.

mscolnick · 2024-10-05T13:07:29Z

frontend/src/core/codemirror/language/__tests__/utils.test.ts

+    expect(highlighted).toEqual(thirdLine);
+    expect(leftover).toEqual(`${firstLine}\n${secondLine}`);
+  });
+});


can you write some tests for markdown and sql?

sql x no selection

sql x with selection

markdown x no selection

markdown x with selection

mscolnick · 2024-10-05T13:17:19Z

frontend/src/core/cells/cells.ts

+      const editorView = state.cellHandles[id].current?.editorView;
+      if (editorView) {
+        const [highlighted, leftover] = extractHighlightedCode(editorView);
+        if (highlighted.length > 0) {


i think the issue is that for markdown/sql, highlighted > 0. if you write a test for extractHighlightedCode, can we get verify that no selection -> no highlighted

mscolnick · 2024-10-05T13:17:44Z

frontend/src/core/codemirror/language/utils.ts

+
+export function extractHighlightedCode(editor: EditorView) {
+  const code = editor.state.doc.toString();
+  let { from, to } = editor.state.selection.main;


you could for if (from === to) {return ["", code]

or could do if (from === to) {return null to signify nothing was highlighted. it will force consumers of this downstream to check null.

i think that is better / clearer

Implemented this and it seems to have fixed the stale issue

mscolnick

awesome work!

github-actions · 2024-10-06T01:24:09Z

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.9.2-dev17

This reverts commit 25afc55. After some experimentation we decided that moving highlighted code to newly created cells is not intuitive. It's common to have code highlighted without an intention to split the cell. Splitting is destructive because it marks cells as stale, so it should be something that is done explicitly by the user -- either by the split hotkey or by simply copy pasting.

improve: Move highlighted code to newly created cells

Loading
Loading status checks…

5c4824c

wasimsandhu added the enhancement label Oct 3, 2024

wasimsandhu requested a review from mscolnick October 3, 2024 05:42

wasimsandhu self-assigned this Oct 3, 2024

vercel bot deployed to Preview – marimo-docs October 3, 2024 05:44 View deployment

wasimsandhu added 2 commits October 2, 2024 22:52

test: Fix getCellEditorView

4fd9ae9

chore: Lint, typecheck

8df7c3f

vercel bot deployed to Preview – marimo-storybook October 3, 2024 05:55 View deployment

vercel bot deployed to Preview – marimo-docs October 3, 2024 05:57 View deployment

akshayka reviewed Oct 3, 2024

View reviewed changes

fix: Mark cells as stale

caac635

vercel bot deployed to Preview – marimo-docs October 3, 2024 06:56 View deployment

vercel bot deployed to Preview – marimo-storybook October 3, 2024 06:57 View deployment

fix: Mark cells as stale (properly)

f1aa340

vercel bot deployed to Preview – marimo-storybook October 3, 2024 07:00 View deployment

vercel bot deployed to Preview – marimo-docs October 3, 2024 07:02 View deployment

mscolnick reviewed Oct 3, 2024

View reviewed changes

fix: Update state correctly, use includeSelectionAsInitialCode

70ad2e1

vercel bot deployed to Preview – marimo-storybook October 4, 2024 05:23 View deployment

vercel bot deployed to Preview – marimo-docs October 4, 2024 05:23 View deployment

mscolnick reviewed Oct 4, 2024

View reviewed changes

fix: Split SQL, Markdown cells correctly

6eaf8ce

vercel bot deployed to Preview – marimo-storybook October 5, 2024 06:09 View deployment

vercel bot deployed to Preview – marimo-docs October 5, 2024 06:09 View deployment

test: Write cell unit tests

f37f5d2

vercel bot deployed to Preview – marimo-docs October 5, 2024 06:41 View deployment

vercel bot deployed to Preview – marimo-storybook October 5, 2024 06:42 View deployment

test: Unit tests for extractHighlightedCode

d860f11

wasimsandhu requested a review from mscolnick October 5, 2024 06:50

vercel bot deployed to Preview – marimo-storybook October 5, 2024 06:51 View deployment

vercel bot deployed to Preview – marimo-docs October 5, 2024 06:53 View deployment

mscolnick reviewed Oct 5, 2024

View reviewed changes

fix: Cells going stale with no selection, add tests

f7a239c

vercel bot deployed to Preview – marimo-storybook October 5, 2024 22:01 View deployment

vercel bot deployed to Preview – marimo-docs October 5, 2024 22:03 View deployment

fix: SQL extract unit tests

a5df548

vercel bot deployed to Preview – marimo-storybook October 5, 2024 22:39 View deployment

vercel bot deployed to Preview – marimo-docs October 5, 2024 22:40 View deployment

wasimsandhu requested a review from mscolnick October 5, 2024 22:40

mscolnick approved these changes Oct 6, 2024

View reviewed changes

mscolnick merged commit 25afc55 into main Oct 6, 2024
22 checks passed

mscolnick deleted the wasim/split-highlighted-code branch October 6, 2024 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve: Move highlighted code to newly created cells #2484

improve: Move highlighted code to newly created cells #2484

wasimsandhu commented Oct 3, 2024 •

edited

Loading

vercel bot commented Oct 3, 2024 •

edited

Loading

akshayka left a comment

wasimsandhu commented Oct 3, 2024

mscolnick commented Oct 3, 2024

mscolnick Oct 3, 2024 •

edited

Loading

wasimsandhu Oct 4, 2024 •

edited

Loading

wasimsandhu commented Oct 4, 2024

mscolnick commented Oct 4, 2024

mscolnick Oct 4, 2024

mscolnick Oct 4, 2024

mscolnick Oct 4, 2024

wasimsandhu commented Oct 5, 2024

mscolnick Oct 5, 2024

mscolnick Oct 5, 2024

mscolnick Oct 5, 2024

mscolnick Oct 5, 2024

mscolnick Oct 5, 2024

mscolnick Oct 5, 2024

mscolnick Oct 5, 2024

wasimsandhu Oct 5, 2024

mscolnick left a comment

github-actions bot commented Oct 6, 2024

improve: Move highlighted code to newly created cells #2484

improve: Move highlighted code to newly created cells #2484

Conversation

wasimsandhu commented Oct 3, 2024 • edited Loading

📝 Summary

🔍 Description of Changes

📋 Checklist

📜 Reviewers

vercel bot commented Oct 3, 2024 • edited Loading

akshayka left a comment

Choose a reason for hiding this comment

wasimsandhu commented Oct 3, 2024

mscolnick commented Oct 3, 2024

mscolnick Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

wasimsandhu Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

wasimsandhu commented Oct 4, 2024

mscolnick commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wasimsandhu commented Oct 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscolnick left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 6, 2024

wasimsandhu commented Oct 3, 2024 •

edited

Loading

vercel bot commented Oct 3, 2024 •

edited

Loading

mscolnick Oct 3, 2024 •

edited

Loading

wasimsandhu Oct 4, 2024 •

edited

Loading