Ticket #4056 (closed defect: fixed)

Opened 4 years ago

Last modified 4 years ago

mcedit: useless subshell warnings

Reported by: onlyjob Owned by: andrew_b
Priority: minor Milestone: 4.8.25
Component: mc-core Version: 4.8.24
Keywords: Cc: slyfox
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

Starting with 4.8.24, mcedit exhibiting minor but frequent and annoying warning:

"GNU Midnight Commander is already running on this terminal. Subshell support will be disabled."

Basically I'm in MC (all the time), with "mcedit" as default editor. Whenever I do "git commit" I see this warning; whenever I edit anything at all with "mcedit" (e.g. 'mcedit ~/.bashrc') I get this warning, etc.

This requires an additional keystroke whenever I invoke "mcedit"...

Is this warning even necessary? If so then I hope there is another, less intrusive way to show this warning in a way that does not require explicit dismissal.

Please consider disabling this warning to revert to former behaviour.

Thanks.

Change History

comment:2 Changed 4 years ago by onlyjob

Fair enough but how to suppress the warning then?

Also why "wontfix"? Only warning is the problem (and only because it requires acknowledgement).
I'm not asking to revert any features...

comment:3 Changed 4 years ago by zaytsev

  • Status changed from new to closed
  • Resolution set to wontfix

The problem is exactly that you are not running mc with default mcedit. What you are doing is to run mc with an external editor, which is set to mcedit on your account (and for this reason doesn't make any sense). This is caused by a Debian-specific patch, which sets external editor as a default, so most users don't even know about that...

This is a very annoying patch from my point of view and I was trying to get rid of, but I'm not a DD and no mentor wanted to upload the package with dropped patch for me, because this is their strange interpretation of Debian Policy.

I assert that leaving the upstream default to correctly use internal editor and leaving users the option to switch on external editor (which users can select using their distro tool and this choice will be complied with as the Policy requires) if they know what they are doing does not violate the Policy in any way, is a sensible thing to do, and I have got many user requests to do it this way, but... again - I'm not a DD, so I don't have a say here.

If you prefer to stick with the patch that forces mc to start mcedit as an external editor within mc with a new subshell, then either your users will have to live with an annoying warning, which will hopefully bring them to set mcedit as an internal editor after they google and find this ticket, or else you can patch out the warning to "solve" the problem, but this will have to remain your distro patch, sorry.

comment:4 Changed 4 years ago by andrew_b

I think, mcedit -u can help you to disable this warning.

comment:5 Changed 4 years ago by onlyjob

@zaytsev, what patch are you talking about?
I'm not convinced that you understand the problem. What about the case when "mcedit" is invoked indirectly by "git"? Are you saying that MC can detect/handle such case? It probably _should_ and that's exactly what this bug is about.

Thanks for the hint, @andrew_b, but I think "mcedit -u" is not enough because editor may be called indirectly by "git". Also that is merely a different way to dismiss the warning that still requires additional keystrokes ...

Last edited 4 years ago by onlyjob (previous) (diff)

comment:6 Changed 4 years ago by onlyjob

Besides my MC is configured to "[x] Use internal edit" so I reckon everything that @zaytsev wrote is incorrect. The problem is not caused by Debian patch.

Please re-open.

comment:7 Changed 4 years ago by zaytsev

Hi Dmitry,

Sorry, I have indeed missed the part that you are starting mcedit from the subshell from within Midnight Commander instead of pressing F4 to edit the file.

However, my comment is not wrong, it's just irrelevant to your use case. Try creating a new user, start mc 4.8.24 from the Debian package, and press F4 on a file - you'll get an editor choice prompt from sensible-editor, and then a warning from mcedit about not being able to initialise the subshell, if mcedit is what you are going to select. The patch that I was talking about is this one:

https://salsa.debian.org/debian/mc/blob/master/debian/patches/disable_internal_editor.patch

I'm not sure how I feel about the use case of starting mcedit from the subshell. On one hand, I like being able to use the subshell when stating stand-alone mcedit. On the other hand, it's annoying to get the warning if you set $EDITOR to mcedit, and then it gets invoked by tools run from within mc like git.

I'm not sure how the editor selection works in Debian nowadays - is it possible to pass -u to mcedit, or we would need a wrapper (mcedit-nosubshell?) along those lines?

#!/bin/sh
mcedit -u -- "$@"

Yury.

comment:8 Changed 4 years ago by onlyjob

Thanks for your reply, Yury.

First of all the patch is correct and necessary despite the side effect you described. It merely changes default (from internal editor to external one) which users can adjust at their convenience.
The patch is there because people don't like when MC the file manager hijacks their default editor.
As much as I like "mcedit" for its convenience, it is neither the most popular nor the most powerful editor.
I'm yet to meet a colleague who prefers "mcedit" to other editors so given that most people prefer other editors it is reasonable to use default system (or user preferred) editor instead of internal MC's editor.

Apart from 'EDITOR' environment variable there are two system binaries that can be used to select default editor:

  • '/usr/bin/editor' which is a symlink to default system editor maintained by 'sudo update-alternatives --config editor' command.
  • '/usr/bin/sensible-editor` which launches editor from the '~/.selected_editor' file (as per 'SELECTED_EDITOR' definition).

'git' uses default system editor unless 'EDITOR' environment variable is defined.

But let's get back to the original problem. Users like myself may have a habit of typing the particular editor's name (e.g. 'mcedit'). I do that when I need to edit a file away from current working directory under MC. And since 4.8.24 I've already seen enough annoying subshell warnings, too many times, even enough to start re-considering my choice of the editor.
Shell wrapper won't do because that's too much work to introduce it to all systems where I use "mcedit".

---
So here is the suggestion: show the warning only when user tries to use subshell (i.e. on Ctrl+O keystroke). This way you will never see the warning until you actually try to use the feature and only when it is not available.

Showing this warning on start is silly because users start the editor to edit and subshell is not a primary feature of the editor.
---

Also worth noticing that using external "mcedit" (or any other editor) under MC is annoying due to the more serious problem (I'm not sure there is a relevant bug report): start MC, type something like 'mcedit ~/.bashrc' then press Ctrl+O twice -- first time you'll see MC's panels but the second time brings black terminal instead of the editor which is still opened. One have to realise that the editor is still active but not rendering the edited file properly. Even exiting the editor may be difficult under such circumstance...

comment:9 Changed 4 years ago by ossi

the matter of the editor default was discussed here, with my conclusion being here (linked explicitly, as the archive is too stupid to link threads across months).

the idea to show the dialog as-is only once the user ctrl-o's won't work:

  • the subshell is started right away, and it would have quite some implications to change that
  • the dialog offers an option to quit, which wouldn't cut it in the middle of editing
  • the ctrl-o shortcut itself is hijacked, so you wouldn't know where you end up upon using it - a similar situation manifests when starting ssh from within mc and then starting mc on the remote host again

the problem of the empty screen is related to the fact that mc's tty redirection apparently doesn't handle alternate screen switching (which is kinda ironic, because mc uses the alternative screen itself). ctrl-l usually helps ...

comment:10 Changed 4 years ago by ossi

now onto possible solutions ...

the first thought would be to simply not start a subshell and not show the dialog when nesting is detected while launched as mcedit.

however, that has the problem that mcedit is still a "subshell application", so ctrl-o'ing won't do what is expected (switching to the already running subshell). it also makes the "alternate screen" bug manifest.

so a better idea might be to instead "suspend" the already running mc (in particular its handling of ctrl-o) and let the new mcedit instance take over the tty entirely, letting it have its own subshell.

i can imagine implementing that suspension via custom tty escape sequences; this would also make it possible to handle the case of nesting via ssh.

comment:11 Changed 4 years ago by slyfox

  • Cc slyfox added

I'm also an EDITOR=/usr/bin/mcedit user. I get mcedit popped up every time I do git commit from under running mc. I don't think every tool out there understands $EDITOR value with whitespace for arguments.

Looks like I'll have to create a system-wide shell wrapper in every machine to get old behaviour back.

comment:12 Changed 4 years ago by andrew_b

  • Status changed from closed to reopened
  • Resolution wontfix deleted

comment:13 Changed 4 years ago by andrew_b

  • Status changed from reopened to accepted
  • Owner set to andrew_b
  • Component changed from mcedit to mc-core
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.25

Branch: 4056_mcedit_subshell
changeset:85e74c3568414c36783ebf6fc4b7722022b72d95

This warning is shown only if mc is run under mc. Standalone mceditor/viewer/diffviewer runs w/o warning.

comment:14 Changed 4 years ago by onlyjob

The patch works perfectly as far as I can say. Thank you very much Andrew.

IMHO good to merge.

comment:15 Changed 4 years ago by andrew_b

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

comment:16 Changed 4 years ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from onlyjob andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

comment:17 Changed 4 years ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.