Ticket #4114 (closed enhancement: fixed)

Opened 2 months ago

Last modified 3 weeks ago

Persistent command line buffer for subshell

Reported by: congest Owned by: andrew_b
Priority: major Milestone: 4.8.26
Component: mc-core Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description (last modified by andrew_b) (diff)

This patch allows the user to start typing a command in the panel prompt, and then continue typing it in the subshell, or to start typing a command in the subshell, and then finish typing it in the panel prompt.

It also fixes #2269 and #2110, bugs in which mc can unexpectedly execute commands without the user's permission.

The new feature works with bash, zsh, and fish. The bugfix works on all shells.

How to test the feature:

run mc
Type any command in the command prompt, but don't hit enter.
ctrl-o
The command you typed will show up in the subshell.
Type any command in the subshell, but don't hit enter.
ctrl-o
The command you typed will show up in the prompt at the bottom of the screen below the panels.

Attachments

persistent_subshell_command_buffer.patch (20.2 KB) - added by congest 2 months ago.
persistent_subshell_command_buffer2.patch (24.1 KB) - added by congest 2 months ago.
persistent_subshell_command_buffer3.patch (26.3 KB) - added by congest 7 weeks ago.
persistent_subshell_command_buffer4.patch (26.2 KB) - added by congest 6 weeks ago.

Change History

Changed 2 months ago by congest

comment:1 Changed 2 months ago by andrew_b

Looks cool.

Some notes about your code.

  • Please read our coding guidelines.
  • Don't mix gboolean and integer:
    • read_command_line_buffer() should return gboolean but returns int;
    • subshell_should_clear_command_line should be gboolean;
    • should_read_new_prompt should be gboolean.
  • flush_subshell() should return gboolean.
  • subshell_prompt_temp_buffer should be freed in exit_subshell().
  • ESC_CHAR and ESC_STR should be used instead of \033.
  • All used ESC sequences should be defined as macros with clear comments.

Thanks!

comment:2 Changed 2 months ago by angel_il

amazing!!! congest, you are the best! Thanx!

comment:3 Changed 2 months ago by angel_il

congest: try input "ls " (without quotes) and press ctrl-o many times. mc hangsup or like this.

comment:4 Changed 2 months ago by angel_il

congest: try input "йцукенг" (without quotes) and press ctrl-o many times. mc hangsup too.

comment:5 Changed 2 months ago by congest

I should be able to fix those things. Let me get back to you.

comment:6 Changed 2 months ago by congest

I attached a new patch.

andrew_b, I fixed the issues that you brought up.

angel_il, I fixed the bug that you found, and I added unicode support. That bug was caused by a quirk in the way bash works.

I'm glad you guys like it. I put a lot of work into it, so I'm really happy that people appreciate it.

Perhaps there are more bugs. I noticed there is one problem where, if the panels are active, data from the subshell doesn't get written to stdout. However, that problem existed to begin with. You can see it if you type "ls&" in the panel prompt, with bash as your shell, then type ctrl-o. You would expect to see the output of the ls command, but mc doesn't display it. I'm not sure the best way to fix this, or if fixing it would have unintended side effects.

Let me know if you find any other problems.

Changed 2 months ago by congest

comment:7 Changed 2 months ago by ossi

i didn't try this yet, but the idea certainly sounds useful. however, i'm afraid this might badly interact with one usage pattern of mine: i sometimes start typing a command at the panel prompt, then find that i'm in the wrong directory, so i fix it by using ctrl-pgup/-pgdn, then submit the command. this will work fine if the shell prompt is kept empty while the panels are visible (or is emptied before cd commands are sent). can you confirm that this works, or make it work?

comment:8 Changed 2 months ago by congest

My patch will not affect your usage pattern.

comment:9 Changed 8 weeks ago by andrew_b

Thanks. But seems it doesn't work correctly with bash-3.

Run mc.
Press Ctrl-o. You're in the subshell now.
Press Ctrl-o. After aprrox. 1 sec. pause you're in the file panel now.
Press Ctrl-o. You're in the subshell now with following message:

bash: bash_execute_unix_command: cannot find keymap for command

Then this message printed every time when you moved to subshell.

Type something in the subshell. Don't press Enter.
Press Ctrl-o to switch to panels.
Command line is empty.

And again about the code.
Type inaccuracy (mix of int and gboolean) is present in your patch.
Please don't use hardcoded \e because of portability problem (see comment in lib/gblobal.h).
Please add your name to the list of authors in the src/subshell/common.c.

comment:10 Changed 7 weeks ago by congest

I attached a new patch.

Thanks. But seems it doesn't work correctly with bash-3.

It seems that bash-3 doesn't support reading the command buffer as an environment variable, so this feature can't work with it.

To fix this, I made it so that when mc starts, it tests out the persistent command buffer feature, and if it doesn't work, it turns the feature off. So now, if you try to use mc with bash-3 (or any other shell that doesn't support the feature), you will just get the traditional mc subshell behavior, instead of the 1 second pause that you described.

I also had to make the keybindings shorter, since bash-4 doesn't support long keybindings. I tried to use keybindings that are physically impossible to type on a real keyboard, so that I wouldn't interfere with any user's preexisting keybindings.

Please don't use hardcoded \e because of portability problem (see comment in lib/gblobal.h).

That's not '\e', that's "\\e", which is what bash and fish both happen to translate into the escape sequence, in that context. (zsh uses something else.)

Changed 7 weeks ago by congest

comment:11 Changed 6 weeks ago by andrew_b

+        while ((rc = select (maxfdp + 1, &read_set, NULL, NULL, &subshell_prompt_timer)) != 1)
+        {
+            if (rc == -1)
+            {
+                if (errno == EINTR)
+                    continue;
+                else
+                    return FALSE;
+            }
+            else if (rc == 1)
+                read (command_buffer_pipe[READ], subshell_command_buffer, sizeof(subshell_command_buffer));
+            else if (rc == 0)
+                break;
+        }

Is (rc == 1) condition correct? This test is already made in while ().

comment:12 Changed 6 weeks ago by zaytsev

  • Blocking 2269 added

comment:13 Changed 6 weeks ago by zaytsev

  • Blocking 2110 added

comment:14 Changed 6 weeks ago by zaytsev

What a cool feature! Always a pain to remember to clear the buffer before toggling the panels.

comment:15 Changed 6 weeks ago by andrew_b

  • Description modified (diff)

Changed 6 weeks ago by congest

comment:16 Changed 6 weeks ago by congest

Is (rc == 1) condition correct? This test is already made in while ()

That is a mistake. I just uploaded a patch that fixes that.

comment:17 Changed 6 weeks ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.26

Branch: 4114_persistent_subshell_command_buffer
Initial changeset:181bfa14ca8c8560fa385482aedab4eef7659ba1

I made some modifications in the second commit (coding stile and micro optimizations). This commit will be squashed before merge to master.

Please review and test.

comment:18 Changed 5 weeks ago by congest

I just tested it, and I didn't find anything wrong with it.

comment:19 Changed 5 weeks ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:20 Changed 5 weeks ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Blocking 2110, 2269 removed
  • Branch state changed from approved to merged

comment:21 Changed 5 weeks ago by andrew_b

  • Status changed from testing to closed

comment:22 Changed 4 weeks ago by ossi

awesome! :)

just a nit: when switching from the shell to the panels (but not the other way around), trailing spaces are removed, which is mildly annoying in this context.

comment:23 Changed 3 weeks ago by andrew_b

#4124: fix crash of standalone mcedit/mcview/mcdiffview at startup.
[2f611780d34f65dee733c600d6a900b7a4d073f8].

comment:24 Changed 3 weeks ago by andrew_b

Note: See TracTickets for help on using tickets.