Ticket #2661 (closed enhancement: fixed)

Opened 13 years ago

Last modified 11 years ago

copy/paste autodetect in the mcedit

Reported by: angel_il Owned by: andrew_b
Priority: major Milestone: 4.8.11
Component: mcedit Version: 4.8.0-pre2
Keywords: Cc: egmont@…, mrmazda@…, pmiscml@…
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

copy/paste autodetection in the mcedit

Attachments

mc-4.8.10-editor-bracketed-paste.patch (7.1 KB) - added by egmont 11 years ago.
enable bracketed paste
mc-4.8.10-editor-paste-newline-tab.patch (4.1 KB) - added by egmont 11 years ago.
Handle newline and tab with shift/ctrl modifiers correctly

Change History

comment:1 Changed 13 years ago by angel_il

Version 0, edited 13 years ago by angel_il (next)

comment:2 Changed 11 years ago by egmont

Relying on timing sounds quite error-prone to me.

Xterm introduced an extension called "bracketed paste mode" (http://invisible-island.net/xterm/ctlseqs/ctlseqs.html#Bracketed%20Paste%20Mode) for exactly this purpose, it's also supported by gnome-terminal and probably white a few others too. I think mc should use this.

comment:3 Changed 11 years ago by egmont

  • Cc egmont@… added

comment:4 Changed 11 years ago by mrmazda

  • Cc mrmazda@… added

Changed 11 years ago by egmont

enable bracketed paste

comment:5 Changed 11 years ago by egmont

Please test the attached patch.

The tricky bit is that due to the "end of bracket" escape the editor sees input with is_idle()==False (expects more characters) and hence doesn't yet refresh the screen, but then no further characters arrive (there's only an "end of bracket" which is swallowed by tty_get_event()), so you end up with a screen that's not refreshed after pasting. So I'm triggering an IDLE signal.

TODO(?): I think it's not only autoindent that should be disabled, but also Wrap mode = None, Fake half tabs = Off, Fill tabs with spaces = Off should be set. What do you think?

Changed 11 years ago by egmont

Handle newline and tab with shift/ctrl modifiers correctly

comment:6 Changed 11 years ago by egmont

One half of the story is the bracketed paste mode which is the right way to solve the problem.

The second half of the story is that MC already has its own half-ready trick: when pasting with Shift-Insert, using the X11 extension, the newline ("Enter" as mc calls it) with the Shift modifier pressed gets converted to a "Return", and in the editor the Return character inserts a non-indenting newline. This makes pasting better in terminals not supporting bracketed paste, however, it has some problems that my patch addresses:

  • Shift+newline gets this special treatment, but Ctrl+newline gets dropped. Hence e.g. when pasting in Gnome-terminal with Ctrl+Shift+V all the newlines will be missing. My patch adds the same non-indenting newline behavior to Ctrl+Newline and Ctrl+Shift+Newline.
  • The code forgets about Tab that also needs special treatment:
    • Most terminals send \e[Z on Shift+Tab, this is not handled by MC at all, moreover it causes a hang for about a second. My patch teaches this sequence to MC. This is especially useful when no X11 is available, because there Ctrl+Tab is identical to Tab, so the backwards tab feature is not available. With my patch Shift+Tab becomes a backwards tab too on all terminals that emit \e[Z.
    • When pasting to the editor, Shift+Tab, Ctrl+Tab and Ctrl+Shift+Tab should all insert a tab for the same reason mentioned at the newline.
    • It would look inconsistent in the keymap files to have logical code such as "backtab" instead of "shift-tab" and friends, hence I went in the direction of getting rid of KEY_BTAB and using KEY_M_SHIFT | '\n' instead. I'm not sure if we can totally get rid of KEY_BTAB or the "learn keys" stuff requires it.

Please test the patches on various kinds of terminals, various ways of copy-pasting, both with and without X11 available.

Both of my patches make sense and can be applied on their own, the ideal is to apply both. The first one modifies pasting behavior in the editor when the terminal supports bracketed paste. The second one modifies pasting behavior when the terminal does not support bracketed paste, and also changes (hopefully for the better) the behavior of modifiers+{tab,enter} throughout mc.

comment:7 Changed 11 years ago by pfalcon

  • Cc pmiscml@… added

comment:8 Changed 11 years ago by pfalcon

Most terminals send \e[Z on Shift+Tab, this is not handled by MC at all, moreover it causes a hang for about a second. My patch teaches this sequence to MC. This is especially useful when no X11 is available, because there Ctrl+Tab is identical to Tab, so the backwards tab feature is not available. With my patch Shift+Tab becomes a backwards tab too on all terminals that emit \e[Z.

Seconded, Shift+Z always input just "Z" in the command line, which was annoying. And it's big surprise to hear that Ctrl+Tab was used as back-tab. I never even tried that in mc! Because well, Shift+Tab has always been back-tab, and Ctrl+Tab, is, well, a switch to another tab in a tabbed user interface (like web browser). Testing a (random) Gnome dialog, Tab goes thru all controls forward, Shift+Tab - backwards, and Ctrl+Tab - goes thru "top-level" controls like main dialog pane and Ok/Cancel? buttons.

comment:9 Changed 11 years ago by pfalcon

hence I went in the direction of getting rid of KEY_BTAB and using KEY_M_SHIFT | '\n' instead.

I guess it's typo here and KEY_M_SHIFT | '\t' is meant.

comment:10 follow-up: ↓ 12 Changed 11 years ago by pfalcon

TODO(?): I think it's not only autoindent that should be disabled, but also Wrap mode = None, Fake half tabs = Off, Fill tabs with spaces = Off should be set. What do you think?

Well, both common sense and how I'd expect it to work in for example, FAR, is that Copy/Paste? should be verbatim. But seeing how it works now in mc, it could be treated as "feature". For example, suppose you copy from ancient tab-using codestyle, and you automagically get clean 4-spaces in your code. But that breaks down when trying to copy/paste Makefile content... So, I'd personally give 0.75 vote for verbatim copy/paste behavior (*), but consider current behavior acceptable, until someone will come screaming and shouting that it's boldly incorrect.

(*) Except for possibly resetting Wrap mode - if someone really has it as something not "None", then I may imagine they expect text pasted from web pages to be formatted, not one long line per paragraph.

Last edited 11 years ago by pfalcon (previous) (diff)

comment:11 Changed 11 years ago by pfalcon

All in all, I quickly tested both patches on current git master (mc-4.8.10-editor-paste-newline-tab.patch​ has minor applicability issues as mc.keymap is a symlink), and they appear to work as expected (Gnome and MATE terminals), what a relief! Of course, need to test a bit longer in real work scenarios to be sure completely.

comment:12 in reply to: ↑ 10 Changed 11 years ago by mrmazda

Replying to pfalcon:
"...if someone really has it as something not "None", then I may imagine they expect text pasted from web pages to be formatted, not one long line per paragraph."

If I copy something that I want to select and copy e.g. from a web page that is formatted by inclusion of newlines, I expect the newlines to be preserved. e.g., PRE text typically has newlines, so I expect its newlines to be included. OTOH, I would not expect P text that has zero newlines to magically have any newlines inserted after pasting, and include only any that the P text actually did include, such as those that may cause an apparent paragraph break. Auto indentation to me only makes sense for what is actually being typed, not anything being pasted, regardless of whatever mechanism actually implements the paste.

comment:13 Changed 11 years ago by pfalcon

So, I've been testing this for 10 days now, with various projects, languages, coding styles. This works very well, I never saw any issue.

+1 for merging this, and thanks much!

comment:14 Changed 11 years ago by pfalcon

Dup: #2826

Other issues with copy/paste: #3080, #3081

comment:15 Changed 11 years ago by andrew_b

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

comment:16 Changed 11 years ago by andrew_b

  • Status changed from new to accepted
  • Owner set to andrew_b
  • Votes for changeset set to andrew_b
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.11

Great!

Branch: 2661_mcedit_paste.
Initial changeset:062de30b8659628d88da8661651b666d02f353e7

mc-4.8.10-editor-bracketed-paste.patch was split in two parts: implemetation of "bracketed paste mode" and usage of it in mcedit. Since we can paste into one editor window only, I removed the chain of MSG_IDLE handling. Current window is updated directly in MSG_IDLE handler of editor.

comment:17 Changed 11 years ago by egmont

Thanks for applying!

In comment 5 I was pondering if probably wrapping and tab->space conversion should also be disabled while pasting. I'm not sure. Does anyone have a firm opinion on this?

comment:18 Changed 11 years ago by andrew_b

It seems, #1797 is fixed here.

comment:19 Changed 11 years ago by angel_il

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

comment:20 Changed 11 years ago by andrew_b

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

Merged to master: [c55b4808f69b98338e7ce55e2595b7471391abbc].

git log --pretty=oneline d87bef7..c55b480

comment:21 Changed 11 years ago by andrew_b

  • Status changed from testing to closed

comment:22 Changed 11 years ago by sknaumov

Now it works, but not exactly as standard mc clipboard:

  • earlier every tab was replaced by single space
  • now tabs only preserved if "fill tabs with spaces" is disabled, else tabs are replaced by a few spaces.

This is for standard mc clipboard:
XTerm*VT100*translations: #override Shift <KeyPress?> Insert:insert-eight-bit()

and this is for X clipboard:
XTerm*VT100*translations: #override Shift <KeyPress?> Insert:insert-selection(CLIPBOARD)

Last edited 11 years ago by sknaumov (previous) (diff)

comment:23 Changed 11 years ago by pfalcon

works, but not exactly as standard mc clipboard

What do you mean by "standard mc clipboard"? That mcedit.clip file? Then it doesn't work as normal standart clipboard works (doesn't move cursor on paste, etc.), so hardly a good target to mimic.

earlier every tab was replaced by single space

now tabs only preserved if "fill tabs with spaces" is disabled, else tabs are replaced by a few spaces.

Yes, if you read comments above, you'll see that it was discussed how to handle that. I personally shared my opinion that automagic behavior from the initial patch was good enough. Just think about it: you edit file with tabs in it (and apparently, use mcedit instance with "fill tabs with spaces: off"), you copy some line chunk to clipboard, then switch to another window, where you don't use tabs (and thus have "fill tabs with spaces: on"), and paste that chunk in. What result do you expect? I would lie to myself if I said "I expect tabs to be pasted - then I like monkey will go thru each and every line, delete each tab, and put bunch of spaces instead". Instead, I pragmatically expect the current behavior - tabs automagically converted to spaces.

Whoever doesn't want that, should not be using "fill tabs with spaces: on" - for particular session, or at all.

XTerm*VT100*translations: #override Shift <KeyPress??> Insert:insert-eight-bit()

What's this?

comment:24 Changed 11 years ago by sknaumov

Just think about it: you edit file with tabs in it (and apparently, use mcedit instance with "fill tabs with spaces: off"), you copy some line chunk to clipboard, then switch to another window, where you don't use tabs (and thus have "fill tabs with spaces: on"), and paste that chunk in. What result do you expect?

But let's imagine that your default setting is to fill tabs with spaces, but occasionally you have to edit files where there is a convention that tabs have not to be filled with spaces, or, moreover, where tabs are part of syntax (Makefile), you copy a line from that file and want to paste it there with some modifications and... tabs are filled with spaces.

May be there is some shortcut to toggle filling behavior just for a single file?

I think that both my reason and your reason are reasonable =) so the best decision would be to make this behavior configurable in some way.

P.S: XTerm*... are strings from .Xresources to enable either paste from mc.clip or from X clipboard.

Note: See TracTickets for help on using tickets.