Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REPL: tab/backspace aligns to 4 #22939

Merged
merged 4 commits into from
Aug 16, 2017
Merged

REPL: tab/backspace aligns to 4 #22939

merged 4 commits into from
Aug 16, 2017

Conversation

rfourquet
Copy link
Member

@rfourquet rfourquet commented Jul 24, 2017

When pressing tab, compute the number of spaces to insert so that the cursor is aligned to a multiple of 4 chars.
When pressing backspace, delete up to 4 spaces so that the cursor is aligned to a multiple of 4 chars.

EDIT: to disable the backspace part of this, you can put the following in your custom keymap: '\b' => (s,o...)->Base.LineEdit.edit_backspace(s, false),.
To bind "shift-tab" to this instead: "\e[Z" => (s,o...)->Base.LineEdit.edit_backspace(s, true),.

@rfourquet rfourquet added the REPL Julia's REPL (Read Eval Print Loop) label Jul 24, 2017
@tkelman
Copy link
Contributor

tkelman commented Jul 24, 2017

-1 on the backspace part of this, that could get annoying

@KristofferC
Copy link
Sponsor Member

-1 on backspace, +1 for a shift tab?

@rfourquet
Copy link
Member Author

Note that the backspace is more or less the behavior in IJulia. I personally find the current behavior annoying, when I pressed once too much on tab, then I have to press 4 times on backspace (but I basically gave up indenting correctly at the REPL). When would backspace aligning to 4 chars be annoying?

@ararslan
Copy link
Member

The only situation I can think of in which backspacing to a multiple of 4 would be annoying would be if you're trying to use spaces for vertical alignment with a line above.

@KristofferC
Copy link
Sponsor Member

Like aligning function arguments.

@ararslan
Copy link
Member

Yeah, maybe shift+backspace would be good for multiples of 4 and backspace by itself could stay as-is. That would avoid messing up anyone's current workflow and would provide opt-in convenience.

FWIW I actually dislike the auto-backspacing behavior in Jupyter, I think mostly because I always forget it's the default.

@Keno
Copy link
Member

Keno commented Jul 24, 2017

Most editors do what is proposed in this PR, and it has never bothered me. The general thing you do in the (rare) case of doing alignments is to backspace and then backfill the right number of spaces. I'd also point out that in the REPL it's even rarer to want to do alignments.

@KristofferC
Copy link
Sponsor Member

Noticing that this is actually how all the editors I use behave and it hasn't bothered me, I change my -1 to a +1 for backspace :P

@tkelman
Copy link
Contributor

tkelman commented Jul 24, 2017

bothers me every time, I always turn this behavior off when possible. typing tab too many times is a lot rarer for me than wanting a backspace to go back one space, rather than having context-dependent meaning.

@rfourquet
Copy link
Member Author

Another case where I feel the need to delete spaces faster is when I want to merge a line with the previous line (disclaimer: I don't use the proposed feature in my daily editing, so am not so qualified to discuss its merits).

backspacing to a multiple of 4 would be annoying would be if you're trying to use spaces for vertical alignment with a line above.

If #22948 gets merged, then the vertical alignment will happen automatically.

There seems to be enough support here so that at least this functionality gets included as opt-in. I will need someone to make the decision here.

@tkelman
Copy link
Contributor

tkelman commented Jul 25, 2017

I'd probably use this plenty if it were shift-tab (or I guess shift-backspace), which doesn't look like it's bound to anything right now. IJulia's shift-tab behaves a little differently, not paying attention to multiples of 4, but that seems to be consistent with the way tab behaves there.

In windows cmd it seems to be ignoring the shift and acting as normal tab, which makes me think the other modifiers probably don't work there either, but that's likely a separate issue.

@rfourquet
Copy link
Member Author

I would have no problem with shift-tab or another key by default, as it's possible to remap keys anyway. But it would be far less discoverable; so I guess the question is whether we think most people (who don't take the time to read this part of the doc) would prefer backspace to remove up-to-4-spaces or always 1 space - tough call! (for sure I don't know).

@StefanKarpinski
Copy link
Sponsor Member

Can we make backspace delete an entire indent and shift-backspace delete a single space?

@StefanKarpinski
Copy link
Sponsor Member

I do think that having backspace unindent is a sensible default and we could make it configurable.

@rfourquet
Copy link
Member Author

I don't know how to map shift-backspace, but it's oterwise already configurable in this PR: e.g. add '\b' => (s,o...)->edit_backspace(s, false) in a custom keymap to reset backspace to single char deletion (the second arg to edit_backspace, which decides whether multiple spaces can be deleted, could be changed to default to false maybe).
Also, I think that in search-mode, I should revert back to single space deletion, as it doesn't seem so useful there to delete indentations.

@ChristianKurz
Copy link
Contributor

As nobody has replied to @KristofferC 's suggestion of shift+tab: I think this is very reasonable.
I also think backspace to delete an indent is reasonable for the REPL.

Why not both shift+tab and backspace?

@StefanKarpinski
Copy link
Sponsor Member

I'm in favor of trying out backspace here (and maybe shift-tab as well) and making it configurable (it already is) for people who don't like that default behavior. We need an easier interface to customizing key bindings, but that's beyond the scope of this PR.

@KristofferC
Copy link
Sponsor Member

I also think we should just try it out. If people will dislike it, I'm sure we will hear about it. :)

@tkelman
Copy link
Contributor

tkelman commented Jul 27, 2017

Why wouldn't shift-tab be discoverable? Put it in the docs, and it's something that IJulia and several editors already use.

@rfourquet
Copy link
Member Author

I believed it would not be very discoverable because it's a functionality a user would have to reach for in the docs, or find by accident (possibly because she knows this key-binding from IJulia), whereas most user would naturally use frequently the backspace key.
Also, I'm not sure shift-tab or shift-backspace can easily be key-binded: does someone know the corresponding key-codes to put in the keymap?

@StefanKarpinski
Copy link
Sponsor Member

Let's follow editors here. Tony doesn't like it, but he's the only one and it's configurable, same as it is in editors – if you go to the trouble of changing it in your editor, then you can do the same here.

@tkelman
Copy link
Contributor

tkelman commented Jul 27, 2017

@ararslan above

FWIW I actually dislike the auto-backspacing behavior in Jupyter

@ararslan
Copy link
Member

I was actually thinking of a different behavior in Jupyter; auto-backspacing is what Vim does and Vim is what I use, so I guess I like that behavior.

@rfourquet
Copy link
Member Author

rfourquet commented Aug 5, 2017

For both functions, I added some options:

  • backspace: if you were pressing backspace when the cursor is not aligned, it becomes aligned, but what is on its right side was loosing alignment; so the second parameter of edit_backspace now tells whether we try to keep all aligned, by trying to delete 4 spaces in total (deleting on the right of the cursor if needed) - the premise is that code will usually be correctly indented and we don't want to alter that.
  • tab: something which I find useful in emacs: when the cursor is at the beginning of a line (or actually pointing on a space), tab can jump the cursor to the first non-space char (without adding indentation, again with the premise that it's already correctly indented); pressing tab again then adds indentation. A second parameter can tell to ignore (and delete) trailing whitespaces when there are only blanks between the cursor and the end of line.
    EDIT: this feature also aims at avoiding unindenting inadvertently what is on the right of the cursor, e.g. cursor is at position 1 (pointing at a space) and code at position 4: with the simpler behavior, the cursor is moved at position 4 by inserting 3 spaces, but the beginning of the code is now at position 7, which is unindented now.

This may seem a bit abstract, but in practice it seems useful to me. So I propose to go with it, with those options enabled by default, and we will soon have feedback if people are unhappy with it!

Also, suggestions welcome for handling some corner-cases.

i = position(buf)
if i != 0 &&
let c = buf.data[i]
c == UInt8('\n') || c == UInt8('\t') ||
Copy link
Contributor

@tkelman tkelman Aug 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this using c for anything? nevermind, this reads a bit unclearly with the let looking like normal code instead of a continuation of the if condition

Copy link
Member Author

@rfourquet rfourquet Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I got the indentation of the lines below wrong (they should be aligned with the line above) (edit: actually, my editor does that, and I'm not sure what is the "correct" indentation!). If you think it's more clear, I can put the let on the right side of &&.

Copy link
Contributor

@tkelman tkelman Aug 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd maybe make a local internal helper function of buf and i for it

@StefanKarpinski
Copy link
Sponsor Member

Failures are the now-usual Travis timeout and Arnoldi error.

@ararslan
Copy link
Member

ararslan commented Aug 7, 2017

At first I thought this would be annoying, but the other day I accidentally pressed tab a few too many times, and had to backspace for a million years to get where I wanted. (Okay, that's hyperbole, but still.) Then I remembered this PR and thought, "There is hope."

+1, especially to the behavior Rafael laid out above.

@StefanKarpinski
Copy link
Sponsor Member

Let's get this merged! I don't think there's anything holding this up aside from spurious CI failures and now a small merge conflict...

* when pressing tab, compute the number of spaces to insert
  so that the cursor is aligned to a multiple of 4 chars
* when pressing backspace, delete up to 4 spaces so that
  the cursor is aligned to a multiple of 4 chars
@rfourquet
Copy link
Member Author

I hoped that #9596 would get merged first, to spare @srp some more rebasing, and also because I stole here the function bufferdata from that PR, but I guess this is not blocking. Also, I wondered if the keywords names I chose are fine; the customization story is not clear yet: adding an entry in the keymap is a bit tedious, but we could imagine to have later one mode keyword accepting a symbol, which can be set from the ENV, or in ".juliarc.jl", e.g. JULIA_BACKSPACE=:align_adjust; this is probably not blocking either.

I've been using this alignment feature for a couple of weeks without problem, so I guess it's ready to go; I will leave to Stefan or someone to do the honours, I prefer to share the responsibility on this one!

@StefanKarpinski
Copy link
Sponsor Member

Having a nice way to configure this is definitely not blocking: it's interactive so not subject to the same level of compatibility requiremenst as production code; and adding an option after-the-fact isn't a breaking change. Let's get this merged to stop conflict and CI churn!

@KristofferC
Copy link
Sponsor Member

Restarted failing worker, log at: https://gist.github.com/KristofferC/683d1a39d64e86d238a09700c3fb8f22

@KristofferC KristofferC merged commit 67f3fc0 into master Aug 16, 2017
@ararslan ararslan deleted the rf/repl-spaces branch August 16, 2017 17:49
@KristofferC
Copy link
Sponsor Member

I vuw it already.

@ararslan
Copy link
Member

Yes, this is really nice! Good work, @rfourquet!

@vtjnash
Copy link
Sponsor Member

vtjnash commented Aug 30, 2017

How do I turn off just the backspace part of the PR for myself?

@rfourquet
Copy link
Member Author

How do I turn off just the backspace part of the PR for myself?

You can put the following in your keymap: '\b' => (s,o...)->edit_backspace(s, false),

@rfourquet
Copy link
Member Author

I realized I already answered to this question in an earlier comment of this thread, so will edit the OP with this info. I also now know how to bind "shift+tab", but I would expect this key to only delete indentation, not characters. I'm open to update the code if someone wants this configurable.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Aug 30, 2017

Thanks. That didn't work, but I was able to use it to figure out how to fix it to work:

Base.atreplinit() do repl
   mykeys = Dict{Any, Any}(
        '\b' => (s, o...) -> Base.LineEdit.edit_backspace(s, false),
    )
    repl.interface = Base.REPL.setup_interface(repl; extra_repl_keymap = mykeys)
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants