CLI window for MCEdit

[psprint]

Hi
The patch implements a simple pipe-based (g_spawn_*) command line window for MCEdit. It is implemented as follows:

• for the container for the output text of entered commands WEdit is extended and used,
• a process (/bin/bash) is being spawned with complete set of 3 pipes for IO,
• the pipes are opened as GIO channels with watching GSources,
• then the glib main loop is being feed from tty_get_event() which is changed to timeout after 200 ms and run the loop iteration then,
• the GSource read/write callbacks are being thus called 5 times per second for a quite smooth update of the WEdit.

Is it a fix or wontfix? I think that such window is nice as it allows to run e.g.: make straight from MCEdit. I know about Ctrl-O however an embedded window provides better experience IMO.

​asciinema recording

[ossi]

i doubt that it's a good idea to have an actual interactive prompt there. instead, the "user menu" from the manager should be extended to the editor.

i also don't think that windowing the command output is a particularly good idea. to cover the "make" use case sensibly, you'd want an output filter that automatically generates "bookmarks" (not necessarily the ones that are already supported). other than that, i wouldn't know why that should be any better than ctrl-o, and do in fact feel rather skeptical about introducing functionality that is largely redundant with a core aspect of mc.

code-wise, you duplicated low-level functionality that is already present in the "external formatter" support.

that integration with the glib mainloop is "suboptimal". how about you properly migrate mc's event loop to glib?

[psprint]

improved the patch

[andrew_b]

Is it duplicate of #4135?

lib/ must not depend on src/.

glib.h is included in lib/global.h. There is no need to include any other GLib headers excepting glib-unix.h.

g_free() handles NULL itself. There is no need to check for NULL before.

We have MC_PTR_FREE.

[psprint]

Replying to andrew_b:

Done – no src/, glib.h and use of MC_PTR_FREE.

PS. Yes it's a duplicate of #4135.

Replying to ossi:

i doubt that it's a good idea to have an actual interactive prompt there. instead, the "user menu" from the manager should be extended to the editor.

There is also a user menu in mcedit, isn't it?

i also don't think that windowing the command output is a particularly good idea. to cover the "make" use case sensibly, you'd want an output filter that automatically generates "bookmarks" (not necessarily the ones that are already supported).

That could be added in next version of the CLI.

other than that, i wouldn't know why that should be any better than ctrl-o, and do in fact feel rather skeptical about introducing functionality that is largely redundant with a core aspect of mc.

I think that it's more common way of adding a CLI to an editor.

that integration with the glib mainloop is "suboptimal". how about you properly migrate mc's event loop to glib?

Why suboptimal? It feeds the loop with a CPU time in a graceful way, not too much, once every 200 ms. It opens up a way to use the goodness in glib, like the GSources and timers. BTW. I rather didn't duplicate the external process handling as I've used GSource/callbacks/main loop.

[andrew_b]

Replying to psprint:

Replying to andrew_b:

Done – no src/

Unfortunately, no. Try again.

[psprint]

Replying to andrew_b:

Replying to psprint:

Replying to andrew_b:

Done – no src/

Unfortunately, no. Try again.

I've had to leave a single src/editor/editwidget.h only in C source, as this class is extended and used in code. I've removed it from instr_stat.h header.

I've improved the patch – found a way to have the prompt always and correctly shown.

[psprint]

always correct prompt

[psprint]

This comes to my mind – to have a checkbox in the "Paste output of..." (ExternalCommand) dialog, saying: "Run in CLI window" or something similar. It would simply run the InstructionStation action with the interpreter command set to the given one. It could be done via the data parameter of the action.

[andrew_b]

Replying to psprint:

I've had to leave a single src/editor/editwidget.h only in C source, as this class is extended and used in code. I've removed it from instr_stat.h header.

I just can say again: nothing in lib/ must depend on src/. Since lib/ can be compiled as libmc.so it must be independent of any suff outside of /lib.

[andrew_b]

Ticket #4135 has been marked as a duplicate of this ticket.

[ossi]

There is also a user menu in mcedit, isn't it?

oh, indeed - under a different key (F11 instead of F2, because, duh), and in a different menu (file instead of command), but it's there.

I think that it's more common way of adding a CLI to an editor.

no, it's not, when you compare apples to apples, which in this case would be TUI (and not GUI) editors.

mc has a certain paradigm for CLI integration, and it's _the_ reason why i use mc even under X, rather than for example krusader. i don't want redundant concepts introduced without a really solid justification.

a proper cli window would imply providing a pty like the subshell integration does, as otherwise bash's readline doesn't work, which leads to a sub-standard user experience. and suddenly you're implementing a complete terminal emulator (or importing the source of 'screen'), because due to the windowing you can't do simple pass-through any more.

It feeds the loop with a CPU time in a graceful way, not too much, once every 200 ms.

there is nothing graceful about that whatsoever, and you have a long way to go if you don't immediately understand why i would say that.

[psprint]

Replying to andrew_b:

Replying to psprint:

I've had to leave a single src/editor/editwidget.h only in C source, as this class is extended and used in code. I've removed it from instr_stat.h header.

I just can say again: nothing in lib/ must depend on src/. Since lib/ can be compiled as libmc.so it must be independent of any suff outside of /lib.

OK, I get it now – this means that the widget should be under src/editor, not lib/widget. It's somewhat obvious after thinking about it for a moment. I send an updated patch.

[psprint]

Replying to ossi:

I think that it's more common way of adding a CLI to an editor.

no, it's not, when you compare apples to apples, which in this case would be TUI (and not GUI) editors.

vim's terminal support, for example, is doing the same – it supports regular window with terminal inside it.

mc has a certain paradigm for CLI integration, and it's _the_ reason why i use mc even under X, rather than for example krusader. i don't want redundant concepts introduced without a really solid justification.

But ctrl-o doesn't even work in mcedit – it hangs after I press it. Also, ctrl-o has inherent drawbacks:

• it isn't mcedit anymore, so to e.g.: copy and paste one has to use mouse,
• it hides either the edited file, or the terminal – one cannot have a look at it without basically leaving mcedit for a moment.

I think that those are enough justification for a simple CLI window.

a proper cli window would imply providing a pty like the subshell integration does, as otherwise bash's readline doesn't work, which leads to a sub-standard user experience.

no, that would be overkill. And, it's possible to emulate readline within the window, I'll code such feature in some update to the feature in future.

It feeds the loop with a CPU time in a graceful way, not too much, once every 200 ms.

there is nothing graceful about that whatsoever, and you have a long way to go if you don't immediately understand why i would say that.

What would be graceful, then? To have frontend_dlg_run() broken down to some bits and hooked up to different points in GLib? I don't think that it's a good idea, it would be asking for problems and I wouldn't want to be the programmer that would be doing this.

PS. Referring to the experience – it's possible to have multi column listing with ls, via a simple pipe to the standard pagination tool pr (​example). User could set up a simple ls alias/function that would do this automatically when run from mc.

[zaytsev]

Ctl+O works just fine in mcedit - in fact I use it all the time, instead of some half-backed CLI-window-"emulator" thing. However, it seems that persistent subshell patch broke our support for zsh, and you are obviously using zsh. Maybe you could fix zsh support instead? See #4198.

[ossi]

Replying to psprint:

vim's terminal support, for example, is doing the same – it supports regular window with terminal inside it.

vim is (almost like emacs) almost an operating system, and generally rather different than every other editor, so it isn't really a good reference for de-facto standards. i see it more as a source of inspiration (or frustration, depending on circumstances :D).

ctrl-o has inherent drawbacks:

• it isn't mcedit anymore, so to e.g.: copy and paste one has to use mouse,

that doesn't seem like much of a problem to me. if you really need to paste a lot of output into the editor, then it would be better to actually directly pipe the command output into it and then cut it down.

• it hides either the edited file, or the terminal – one cannot have a look at it without basically leaving mcedit for a moment.

that's true, but arguably, if that significantly impacts your workflow, then your workflow is broken. i'm assuming you're implying the make/gcc output case here, which i already addressed.

it's possible to emulate readline within the window, I'll code such feature in some update to the feature in future.

so you're piling up more workarounds to make a bad design kinda work. that really isn't a reasonable approach.

What would be graceful, then? To have frontend_dlg_run() broken down to some bits and hooked up to different points in GLib?

if that's your way to say "to rewrite mc's main loop(s) to be properly glib-based", then yes.

I don't think that it's a good idea, it would be asking for problems and I wouldn't want to be the programmer that would be doing this.

yeah, because that's boring maintenance work, right? better have fun with creating an unmaintainable mess instead.

[psprint]

I've extended patch with the "Paste output of..." idea:

• a checkbox "Open in CLI window" is now under the text input (for the command to run),
• I've added a new (but it's just a thin frontend) function input_dialog_ext() where the …_ext is for: a new, optional last argument that can hold an array of quick_widget_t structures,
• the widgets from the array are placed after the main text input and before Ok/Cancel buttons,
• example use:

    quick_widget_t chbox[3] = { QUICK_SEPARATOR(TRUE),
                         QUICK_CHECKBOX("&Open in CLI window", &run_in_cli, NULL), 
                         QUICK_END};
    
    input_dialog_ext(…, chbox);

• the "Open in CLI window" feature is implemented via message's void *data argument,
• a final fine-feature: invoking CK_InstructionStation when in a station window restarts the station with the same command (instead of starting a new station),
• ​asciicast presenting the "Open in…" feature.

[psprint]

Added open in … checkbox

[psprint]

Cleanup + #ifdefs for GLib 2.30

[psprint]

Replying to ossi:

Replying to psprint:

it's possible to emulate readline within the window, I'll code such feature in some update to the feature in future.

so you're piling up more workarounds to make a bad design kinda work. that really isn't a reasonable approach.

There is no "bad design". The gio/GSource/main-loop is a very good quality code, believe me. The "workaround" would be a short, elegant snippet of code, placed in a few places designated for them.

I don't think that it's a good idea, it would be asking for problems and I wouldn't want to be the programmer that would be doing this.

yeah, because that's boring maintenance work, right? better have fun with creating an unmaintainable mess instead.

Because it would be a 99% rejection, as I expect the maintainers would highly hesitate to introduce such a revolutionary change… Also, I think that moving mc operation behind a curtain of GLib API lights up "wrong!" lamps in most of programmers' heads… It just doesn't feel right, to decompose and spread mc core over GLib. The 5x per second introduction of GLib main loop might be a good way of getting familiar with it in a gradual fashion, without revolutionary rewrite.

[ossi]

There is no "bad design". The gio/GSource/main-loop is a very good quality code, believe me.

as indicated by the blockquotes, the "bad design" refers to the very idea of embedding a pseudo-cli window into the editor.

of course, specifically regarding the input side of things, it must be observed that the panels' command prompt is kinda the same thing (and it does come with downsides - esp. now with the better subshell integration, i find it irritating that the histories are not shared; i think i'll file an issue with some ideas at some point). however, the integration of that prompt with the panels provides a significant counter-weight to the downsides, which i don't really see in the case of the editor.

i actually wanted to give your patch a try to be able to be more specific, but it doesn't apply to current master. please fix or specify the dependencies.

Because it would be a 99% rejection, [...]

instead of using baseless speculation as an excuse you could just ask. ;-)
specifically, you could take it as a hint that i already had suggested that change and nobody contradicted (as a matter of fact, that proposal has a much longer history than you can be expected to be aware of; it got mild support from one of the core devs some years ago).

It just doesn't feel right, to decompose and spread mc core over GLib.

but loose coupling is exactly how event-driven applications (a pattern that perfectly fits GUIs) have been done for several decades. mc's current tight coupling has repeatedly caused challenges, which is why the idea to get rid of it isn't new.

of course, this isn't going to be a trivial change, as modality always requires some kind of nested event loop, which comes with its own challenges. but i'm positive that there is enough glib-based code around to get inspiration from.

[psprint]

Replying to ossi:

Because it would be a 99% rejection, [...]

instead of using baseless speculation as an excuse you could just ask. ;-)
specifically, you could take it as a hint that i already had suggested that change and nobody contradicted (as a matter of fact, that proposal has a much longer history than you can be expected to be aware of; it got mild support from one of the core devs some years ago).

I can imagine that such rewrite becomes a well known, popular myth among the team. However I suspect that when it would come to actually implementing it, the enthusiasm and the well established rationale would quickly fade.

I attach fixed patch.

[ossi]

the enthusiasm and the well established rationale would quickly fade.

as you can infer from the historical evidence, there is no enthusiasm in the first place. but there is awareness that something probably should be done. we won't know more until someone actually musters that enthusiasm and provides a high-quality patch. i for one think that this would be a good way to familiarize oneself with a lot of the code, and to gain trust from the current maintainers as someone who actually cares for the bigger picture. i'm actually tempted to do it myself, but you know, priorities.

[ossi]

so i applied the patch. first impressions:

• m-i doesn't work as a shortcut, as it clashes with "prev bookmark" (see search menu)
• as expected, that editor-cli-mashup feels completely wrong in every imaginable way
• the window doesn't appear in the "open files" list, so it cannot be focused that way

ok, moving on, m-u:

• yay, without checking that checkbox, that is totally useful and intuitive
• an integration with user menu (think mailcap's copiousoutput flag) seems potentially desirable, but otoh the history will effectively build a menu of its own, so it might be pointless
• checking that checkbox makes things become just weird. what opens up isn't actually a cli window; it isn't even an actual "instruction station". it's an editor that doesn't react to 'enter'. wtf?
• i for one would just add a "open in new editor window" checkbox instead
• i don't know what you meant with that auto-repeat functionality (i can't reproduce it according to your description), but i strongly recommend against any "magic", as it is poorly discoverable and potentially confusing
• when there are multiple of these windows open and you close them in turn, then starting with the second one, the "main" editor in the background blanks out while the confirmation popup is shown. oh, and when i open the menu the first time after closing these windows, the blankout also happens. i suppose this may be unrelated to your patch.
• under circumstances i could not identify, the invocation of the "paste output" dialog is suppressed, but the editor is also "stuck" until another key is pressed

on a general note, the editor's menus are a complete mess, and you're not exactly making it better. i created #4205 for that.

on a tangent, #2842 is peripherally related to what you're doing. maybe pick it up?

[psprint]

The CLI is simple mechanism, not a fully featured terminal. It allows to: run make and feel like in home because no need to leave or hide mcedit. The repeat feature currently works only when invoked via shortcut (not by menu).

[ossi]

oh, something else regarding integration and redundancy: mcedit has this "external formatter" menu item. your new "paste output" is functionally equivalent with making an empty selection and piping it through an interactively selectable filter that ignores its input. and i just checked, mcedit's user menu is actually a menu of filters (so that external formatter should be actually subsumed into it), so it really seems wrong to have your functionality completely independent of that.

speaking of the user menu, if one invokes any (?) command from it after using your instruction station, mc will lock up:

#0  0x00007fdc2a65612a in __GI___sigsuspend (set=set@entry=0x7ffd9efef330) at ../sysdeps/unix/sysv/linux/sigsuspend.c:26
#1  0x000055c78b1825a9 in synchronize () at common.c:490
#2  feed_subshell (how=how@entry=1, fail_on_error=fail_on_error@entry=0) at common.c:834
#3  0x000055c78b1835b6 in invoke_subshell (command=<optimized out>, how=how@entry=1, new_dir_vpath=new_dir_vpath@entry=0x7ffd9efef428) at common.c:1467
#4  0x000055c78b126744 in do_executev (shell=shell@entry=0x55c78b9d2850 "/bin/bash", flags=flags@entry=4, argv=0x55c78baf3370) at execute.c:321
#5  0x000055c78b1268d9 in do_execute (shell=0x55c78b9d2850 "/bin/bash", command=0x55c78bb38750 " /bin/sh /tmp/mc-root/mcusrTYB0Y0", flags=4) at execute.c:391
#6  0x000055c78b12698a in shell_execute (command=<optimized out>, flags=<optimized out>) at execute.c:441
#7  0x000055c78b185aef in execute_menu_command (show_prompt=1,
    commands=0x55c78bb03281 "\n+ f \\.h$ | f \\.c$ | f \\.cc$\n2       GPL description header\n        cat >>%b <<EOF\n        /*\n        *  This program is free software: you can redistribute it and/or modify\n        *  it under the te"..., edit_widget=0x55c78bb27000) at usermenu.c:561
#8  user_menu_cmd (edit_widget=edit_widget@entry=0x55c78bb27000, menu_file=menu_file@entry=0x0, selected_entry=selected_entry@entry=-1) at usermenu.c:1149
#9  0x000055c78b171b32 in user_menu (edit=edit@entry=0x55c78bb27000, menu_file=menu_file@entry=0x0, selected_entry=selected_entry@entry=-1) at edit.c:1785
#10 0x000055c78b17412c in edit_execute_cmd (edit=0x55c78bb27000, command=36, char_for_insertion=-1) at edit.c:3925
#11 0x000055c78b174413 in edit_execute_key_command (edit=edit@entry=0x55c78bb27000, command=<optimized out>, char_for_insertion=-1) at edit.c:3253
#12 0x000055c78b17cf4e in edit_callback (w=0x55c78bb27000, sender=<optimized out>, msg=<optimized out>, parm=36, data=<optimized out>) at editwidget.c:1020
#13 0x000055c78b144edb in send_message (msg=MSG_ACTION, data=0x0, parm=<optimized out>, sender=0x55c78ba5f490, w=<optimized out>) at ../../lib/widget/widget-common.h:243
#14 menubar_execute (menubar=0x55c78ba5f490) at menu.c:354
#15 0x000055c78b1444fc in mouse_process_event (event=0x7ffd9efef9b4, w=0x55c78ba5f490) at mouse.c:190
#16 mouse_handle_event (w=0x55c78ba5f490, event=0x7ffd9efefa44) at mouse.c:222
#17 0x000055c78b136858 in group_handle_mouse_event (w=<optimized out>, event=0x7ffd9efefa44) at group.c:662
#18 0x000055c78b12d8f2 in dlg_process_event (key=-2, event=<optimized out>, h=0x55c78bb3d100) at dialog.c:536
#19 dlg_process_event (h=0x55c78bb3d100, key=-2, event=<optimized out>) at dialog.c:523
#20 0x000055c78b12dc0b in frontend_dlg_run (h=0x55c78bb3d100) at dialog.c:320
#21 dlg_run (h=h@entry=0x55c78bb3d100) at dialog.c:575
#22 0x000055c78b17d302 in edit_files (files=files@entry=0x55c78ba5c6c0 = {...}) at editwidget.c:1294
#23 0x000055c78b17d336 in edit_file (file_vpath=file_vpath@entry=0x55c78bad65e0, line=<optimized out>) at editwidget.c:1219
#24 0x000055c78b18c548 in edit_file_at_line (what_vpath=0x55c78bad65e0, internal=<optimized out>, start_line=<optimized out>) at cmd.c:683
#25 0x000055c78b18c5d3 in do_edit (what_vpath=<optimized out>) at cmd.c:167
#26 0x000055c78b18c614 in edit_cmd (panel=<optimized out>) at cmd.c:721
#27 0x000055c78b131f14 in midnight_execute_cmd (sender=0x55c78ba85ab0, command=44) at filemanager.c:1221
#28 0x000055c78b12b024 in buttonbar_callback (w=0x55c78ba85ab0, sender=<optimized out>, msg=<optimized out>, parm=<optimized out>, data=<optimized out>) at buttonbar.c:171
#29 0x000055c78b136ede in group_handle_hotkey (key=1004, g=0x55c78ba69600) at group.c:497
#30 group_default_callback (w=0x55c78ba69600, sender=<optimized out>, msg=<optimized out>, parm=<optimized out>, data=<optimized out>) at group.c:570
#31 0x000055c78b136d93 in group_handle_key (key=1004, g=0x55c78ba69600) at group.c:434
#32 group_default_callback (w=0x55c78ba69600, sender=<optimized out>, msg=<optimized out>, parm=1004, data=<optimized out>) at group.c:567
#33 0x000055c78b12d984 in dlg_key_event (d_key=1004, h=0x55c78ba69600) at dialog.c:251
#34 dlg_process_event (h=0x55c78ba69600, key=1004, event=<optimized out>) at dialog.c:541
#35 0x000055c78b12dc0b in frontend_dlg_run (h=0x55c78ba69600) at dialog.c:320
#36 dlg_run (h=0x55c78ba69600) at dialog.c:575
#37 0x000055c78b13332c in do_nc () at filemanager.c:1837
#38 0x000055c78b125339 in main (argc=<optimized out>, argv=<optimized out>) at main.c:463

it seems likely that this is related to the redundant low-level process handling.

[ossi]

It allows to: run make and feel like in home because no need to leave or hide mcedit.

the question would be why anyone would want to handle 'make' that way. it seems like an incredibly inefficient and idiosyncratic way to approach things.

on a more general note: back in the day, i also used mc/mcedit as a poor man's IDE, and keep doing so when loading up a project into a "proper" IDE doesn't seem worth it (there is always some setup overhead). but the productivity boost one gets from a proper IDE (project manager / build system integration, a real code model instead of ctags, proper compiler output parsers, etc.) with an editor that can natively use the windowing system's features instead of being constrained by its containing terminal emulator is insane. there is no way mcedit could compete with that, so one has to ask oneself in how far it makes sense to even try.

[psprint]

Replying to ossi:

It allows to: run make and feel like in home because no need to leave or hide mcedit.

the question would be why anyone would want to handle 'make' that way. it seems like an incredibly inefficient and idiosyncratic way to approach things.

I, for example, like not switching terminal tabs, not leaving mcedit / stopping editing / distracting. Besides I can search for "error" in output.

there is no way mcedit could compete with that, so one has to ask oneself in how far it makes sense to even try.

I'm having fun with mcedit. Let's hope that some big patches, like #4165, will be accepted so that my work will not be futile.

I've extended the patch with:

• a second checkbox "Discard output" in ExternalCommand dialog – to run command without recording any output; I think that such option is quite usefull, I for example run ctags this way to refresh TAGS,
• a control flow for the CLI –> ability to scroll up during the command is run and produces text,
• changed shortcut to Alt-s.

[ossi]

I, for example, like not switching terminal tabs, not leaving mcedit / stopping editing / distracting. Besides I can search for "error" in output.

that's besides the point. i already described how to do it properly. why settle for less?

for example run ctags this way to refresh TAGS,

that's totally wrong. such commands should be directed to the console instead, i.e., behave like the panel command prompt and user menu. one can still redirect stdout to /dev/null if so desired, but stderr should always go somewhere if failure isn't explicitly expected.

[andrew_b]

Let's end the discussion.

[ossi]

closing the issue seems appropriate, as the "instruction station" itself is obviously a reject. but there are several useful things being discussed here, so shutting down the discussion is imo not ok. it would help if you as a maintainer would confirm specific claims i made, to encourage psprint to open new issues and pursue these ideas.

[andrew_b]

@ossi, could you please summarize the essence of your thoughts here?

[ossi]

there are several of these:

• the fundamental idea to invoke commands interactively is not wrong, but it should be integrated with existing concepts like the user menu and the subshell
• the 'make' case would be better served by parsing the compiler output to create an issue list that allows for direct navigation to the error location (btw, qt creator's <foo>2tasks.pl scripts could be directly lifted; see ​https://code.qt.io/cgit/qt-creator/qt-creator.git/tree/scripts)
• some infrastructure work should be done in the form of porting mc to the glib main loop