frontend/jupyter: add cell-type switches to the button menu of a cell #8473
Conversation
haraldschilly
force-pushed
the
jupyter-celltype-cellmenu-8472
branch
from
55e07ae to
a5fb1d2
Compare
Running a markdown cell makes sense -- it means changing it from edit mode to non-edit mode. |
…, to have everything in one place. With that, the cell menu appears on markdown cells as well
haraldschilly
force-pushed
the
jupyter-celltype-cellmenu-8472
branch
from
c8cf870 to
3e0373d
Compare
haraldschilly
force-pushed
the
jupyter-celltype-cellmenu-8472
branch
from
5c5b04a to
51b2432
Compare
|
But what's being run? Well, in any case, I added the run button. I had to experiment a bit, because the shift evaluation, the menu run button, and the cell run button are slightly different. I got it working, though. The key was to change the cell buttonbar run button's |
The definition of "run" is "compile the markdown source into a visible rendered view". That said, this would be super annoying if it happened for all viewers at once, so only the particular frame that you're viewing has its mode changed from edit to view. That's why it's |
| @@ -421,8 +422,7 @@ export class NotebookFrameActions { | |||
| undefined, | |||
| false, | |||
| ); | |||
| let md_edit_ids = this.store.get("md_edit_ids"); | |||
| if (md_edit_ids == null) md_edit_ids = Set(); | |||
| const md_edit_ids = this.store.get("md_edit_ids", Set()); | |||
There was a problem hiding this comment.
This change you just made is NOT equivalent and it pretty dangerous. If 'md_edit_ids' is actually set to undefined or to null in the store, then this.store.get("md_edit_ids", Set()); would be undefined or null, NOT Set(). It's much, much safer to do:
const md_edit_ids = this.store.get("md_edit_ids") ?? Set()
It's also more efficient since above doesn't evaluate Set() unless needed, whereas this.store.get("md_edit_ids", Set()); always does.
There was a problem hiding this comment.
That said, the function that implement this.store.get(...) could also be changed to basically do the ?? thing. That said, I generally think ?? is just better, and we shouldn't write new code using that second input to get. I probably did so also in the last week, but I'm going to stop.
| @@ -487,7 +487,7 @@ export class NotebookFrameActions { | |||
| const cell = this.get_cell_by_id(id); | |||
| if (cell == null) return; | |||
|
|
|||
| if (cell.getIn(["metadata", "editable"]) === false) { | |||
| if (!this.jupyter_actions.store.is_cell_editable(id)) { | |||
| // TODO: NEVER ever silently fail! | |||
There was a problem hiding this comment.
feel free to delete my comment "// TODO: NEVER ever silently fail!" -- I've learned people greatly prefer silent failures to errors they don't understand. At most, log something to the console...
|
I had a nitpick comment or two, but no actual bug, so I merged this. |
This is how it looks (including some reorg), but needs more work.
The only usability issue is, if you switch away from a code type, that cell menu is gone as well. Then you're stuck in markdown and the menu where you did the change can't be reached. So, the extra work here is to make that menu available for all cell types, and only show "run" and so on if it makes sense.
What I did is to remove the "Edit" button from rendering it in the "cell input", down to render it in the "cell-buttonbar". With that, everything is in one place. Bonus: it is trivial to enable the "Format" button for markdown as well, although it is a bit strange if the cell isn't in edit mode and edited as markdown. So, I left it disabled as before.
Ok, and here is with the additional changes for markdown and raw cells. The "Edit" button is actually a toggle now. The menu now allows undo/redo, moving, cutting, etc. for markdown as well.