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

gh-119034, REPL: Change page up/down keys to search in history #123607

Merged
merged 5 commits into from
Sep 6, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Sep 2, 2024

Change page up and page down keys of the Python REPL to history search forward/backward.

Change <page up> and <page down> keys of the Python REPL to history
search forward/backward.
@vstinner
Copy link
Member Author

vstinner commented Sep 2, 2024

Example 1: Search history with imp prefix.

$ ./python
>>> import sys
>>> 1 1
>>> import os
>>> imp<PAGE UP>
  • If you press once, you get import os with the cursor after p.
  • If you press twice, you get import sys with the cursor after p.

Example 2: Navigate history.

$ ./python
>>> import sys
>>> 1 1
>>> import os
>>> <PAGE UP>
  • If you press once, you get import os with the cursor at the start.
  • If you press twice, you get 1 1 with the cursor at the start.

@encukou
Copy link
Member

encukou commented Sep 3, 2024

Thanks! This feature would make the new REPL usable for me.
It doesn't work quite like the readline equivalent though: starting with a blank line, PGUP and PGDN behave like arrows: the cursor stays at the end of the line, and PGDN after PGUP gets me back to an empty line.

@hroncok
Copy link
Contributor

hroncok commented Sep 3, 2024

Example 1: Search history with imp prefix.

$ ./python
>>> import sys
>>> 1 1
>>> import os
>>> imp<PAGE UP>
  • If you press once, you get import os with the cursor after p.
  • If you press twice, you get import sys with the cursor after p.

This behavior is consistent with the old REPL. I verified it works.

Example 2: Navigate history.

$ ./python
>>> import sys
>>> 1 1
>>> import os
>>> <PAGE UP>
  • If you press once, you get import os with the cursor at the start.
  • If you press twice, you get 1 1 with the cursor at the start.

As said by @encukou, this behavior is not consistent with the old REPL. I have verified it indeed behaves as you describe, which is not ideal (but still better than not having this at all).

@vstinner
Copy link
Member Author

vstinner commented Sep 3, 2024

I don't know pyrepl code base, if you have advices to make these commands (page up/down) closer to readline, i'm interested.

Lib/_pyrepl/historical_reader.py Outdated Show resolved Hide resolved
Lib/_pyrepl/historical_reader.py Outdated Show resolved Hide resolved
@pablogsal
Copy link
Member

pablogsal commented Sep 3, 2024

Isn't this a duplicate of #121859 for #120767?

@pablogsal
Copy link
Member

Thanks! This feature would make the new REPL usable for me.

How so? I mean, you can use Ctrl-r and similar to go to specific entries no?

@hroncok
Copy link
Contributor

hroncok commented Sep 3, 2024

Isn't this a duplicate of #121859 for #120767?

No. See my comment from #119034 (comment)

This particular readline feature is about PageUp and PageDown, not about arrows up and down. What this proposes is to change the behavior of PageUp and PageDown in line with what was established in #119034 (comment)

What #121859 / #120767 proposes is to change the behavior of arrow up/down which was not established in #119034 (comment)

Thanks! This feature would make the new REPL usable for me.

How so? I mean, you can use Ctrl-r and similar to go to specific entries no?

That's a different functionality of the REPL. This allows us to write partial text first and press PageUp after. Ctrl r requires us to use it before we start to type. The PageUp/PageDown functionality has been the default in Fedora readline for 15 years and not having it is the reason for me not to use the new REPL.

@pablogsal
Copy link
Member

pablogsal commented Sep 3, 2024

No. See my comment from #119034 (comment)

But isn't that the same feature with different bindings? What am I missing?

@pablogsal
Copy link
Member

and not having it is the reason for me not to use the new REPL.

I understand and respect that but I am a bit surprised that the lack of this outweights multi-line editing and paste mode, among other new features. So I am trying to understand why is this so fundamental.

@vstinner
Copy link
Member Author

vstinner commented Sep 3, 2024

@encukou:

It doesn't work quite like the readline equivalent though: starting with a blank line, PGUP and PGDN behave like arrows: the cursor stays at the end of the line, and PGDN after PGUP gets me back to an empty line.

I updated my PR to behave as readline. I managed to find a way to implement the expected behavior.

@hroncok
Copy link
Contributor

hroncok commented Sep 3, 2024

No. See my comment from #119034 (comment)

But isn't that the same feature with different bindings? What am I missing?

The apparent consensus of #119034 (comment)

It is the same (similar?) feature, but binding it to different keys (which already have different functionality) is against that consensus.

and not having it is the reason for me not to use the new REPL.

I understand and respect that but I am a bit surprised that the lack of this outweights multi-line editing and paste mode, among other new features. So I am trying to understand why is this so fundamental.

The new REPL has many nice improvements and I am looking forward to using it, yet it fails to behave the way I expect a REPL to behave. I reported that problem and I was told yes, this is a problem, we are planning to fix this, but instead, a different fix was proposed.

Imagine I give you a new bicycle with perpetual motion automatic power, a super comfy seat, and it can be folded into a matchbox-size box, so you can carry it around in your pocket. Yet it does not allow you to steer. The lack of steering capabilities outweighs all the perks.

For me, the basic functionality of REPL outweighs all the fancy new stuff. I open the REPL, I type imp, I press PageUp and it replaces my prompt with the first line of my ~/python_history. Imagine you press i in the REPL and instead of typing i it would type E. That's the same level of discomfort for me when I use the new REPL.

@hroncok
Copy link
Contributor

hroncok commented Sep 3, 2024

@encukou:

It doesn't work quite like the readline equivalent though: starting with a blank line, PGUP and PGDN behave like arrows: the cursor stays at the end of the line, and PGDN after PGUP gets me back to an empty line.

I updated my PR to behave as readline. I managed to find a way to implement the expected behavior.

Thank you. It work almost as readline now, except I cannot PageDown back to empty prompt.

Old REPL

>>> [page up]
>>> whatever[page down]
>>> 

New REPL with this PR

>>> [page up]
>>> whatever[page down]
>>> whatever

@vstinner
Copy link
Member Author

vstinner commented Sep 3, 2024

@pablogsal:

Isn't this a duplicate of #121859 for #120767?

Right, the behavior looks very similar: match the beginning of the line to navigate history.

I tested gh-121859 : there are subtle but major differences. The main one is that in my PR (page up/down), the cursor doesn't move when matching the prefix, whereas it moves at the end with gh-121859. The usability/behavior is different.

Example:

>>> import sys
>>> 1 1
>>> import os
>>> 2 2
>>> imp

Using <page up> and my PR, you only get import from the history:

  • import os (cursor after "p")
  • import sys (cursor after "p")
  • (stop here)

Using <up> and gh-121859, the first <up> looks for imp, but then it just navigates the history:

  • import os
  • 1 1 <= unwanted entry :-(
  • import sys (cursor at the end) <= in fact, `<up> just navigates the history
  • import sys (cursor at the start)
  • (stop here)

I suggest you to play with both PR to see/understand the subtle differences.

This PR is navigate all entries starting with imp without moving the cursor.

@vstinner
Copy link
Member Author

vstinner commented Sep 3, 2024

@hroncok:

Thank you. It work almost as readline now, except I cannot PageDown back to empty prompt.

I fixed the last PageDown to be able to go back to an empty prompt.

@hroncok
Copy link
Contributor

hroncok commented Sep 3, 2024

There seem to be some kind of state leak, see (| is the cursor):

>>> imp|[page up]
>>> imp|ort os
>>> imp|[Ctrl k]ort os
>>> imp|[page up]
>>> imp|ort sys
>>> imp|[page down]ort sys
>>> imp|[page down]
>>>

While in the old REPL:

>>> imp|[page up]
>>> imp|ort os
>>> imp|[Ctrl k]ort os
>>> imp|[page up]
>>> imp|ort os
>>> imp|[page down]ort os
>>> imp|[page down]ort os
>>> imp|[page up]ort os
>>> imp|ort sys

@pablogsal
Copy link
Member

The apparent consensus of #119034 (comment)

It is the same (similar?) feature, but binding it to different keys (which already have different functionality) is against that consensus.

Just to be clear, what I am a bit concerned is that the contributor in #121859 may feel that we ignored her PR and implemented most of the required changes separately, leaving the original issue just as a rebinding or (maybe?) closed because the lack of consensus.

@pablogsal
Copy link
Member

pablogsal commented Sep 3, 2024

Imagine I give you a new bicycle with perpetual motion automatic power, a super comfy seat, and it can be folded into a matchbox-size box, so you can carry it around in your pocket. Yet it does not allow you to steer. The lack of steering capabilities outweighs all the perks.

For me, the basic functionality of REPL outweighs all the fancy new stuff. I open the REPL, I type imp, I press PageUp and it replaces my prompt with the first line of my ~/python_history. Imagine you press i in the REPL and instead of typing i it would type E. That's the same level of discomfort for me when I use the new REPL.

Ok, I understand your point of view, but I personally would not elevate this particular feature as a "basic functionality". The basic functionality of the REPL is to execute what you type, anything else it's additional (nice to have) features. This particular feature we are talking about was even a readline functionality that's only available if you have configured the inputrc so it was not even something built into the old REPL. Furthermore, binding against edit line makes it work totally different in many cases.

But in any case, I understand your point and it makes sense. Let's fix it so is as close as possible to the old REPL and the bycicle can steer again.

But @vstinner @encukou please take #123607 (comment) into account here

@hroncok
Copy link
Contributor

hroncok commented Sep 3, 2024

This particular feature we are talking about was even a readline functionality that's only available if you have configured the inputrc so it was not even something built into the old REPL.

It has been enabled by default for all the Fedora Linux, RHEL and CentOS users for 15 years. So from my PoV it might as well have been "built into" the old REPL.

I agree that if all other functionality of REPL beyond "input code and output the result" is additional, then this is as well. By "basic" I've meant "important enough to stop me from using it".

@hroncok
Copy link
Contributor

hroncok commented Sep 3, 2024

Just to be clear, what I am a bit concerned is that the contributor in #121859 may feel that we ignored her PR and implemented most of the required changes separately, leaving the original issue just as a rebinding or (maybe?) closed because the lack of consensus.

I don't think that the partial history on arrows feature is contradicticting this one. I have not seen it in any REPL (and my opinion whether Python REPL needs it is not relevant), but whether or not it is added does not need to block the PageUp/Down feature.

@pablogsal
Copy link
Member

I don't think that the partial history on arrows feature is contradicticting this one. I have not seen it in any REPL (and my opinion whether Python REPL needs it is not relevant), but whether or not it is added does not need to block the PageUp/Down feature.

It's not the feature: it's the fact that giving that the code will overlap, merging this PR first for example may eliminate the need for a lot of code in the other one and leave it just as rebinding, which may be a bad experience for the contributor. That's what I am worried about

@pablogsal
Copy link
Member

It has been enabled by default for all the Fedora Linux, RHEL and CentOS users for 15 years. So from my PoV it might as well have been "built into" the old REPL.

I understand 👍

I agree that if all other functionality of REPL beyond "input code and output the result" is additional, than this is as well. By "basic" I've meant "important enough to stop me from using it".

Gotcha

@ambv
Copy link
Contributor

ambv commented Sep 5, 2024

This is mostly good. It currently doesn't support searching within subsequent lines of multiline history items, which I think it should be doing.

@hroncok should the PgUp/PgDn search ignore leading whitespace or should it require leading whitespace in this scenario?

@hroncok
Copy link
Contributor

hroncok commented Sep 5, 2024

See my comment in #123607 (comment) -- I think this is not good.

It currently doesn't support searching within subsequent lines of multiline history items, which I think it should be doing.

Good point, I was not testing that.

should the PgUp/PgDn search ignore leading whitespace

It does not do that normally.

or should it require leading whitespace in this scenario?

I don't understand. What do you mean?

@vstinner
Copy link
Member Author

vstinner commented Sep 5, 2024

If somone wants to test readline's built-in commands on Linux other than Fedora, the following ~/.inputrc configuration file can be created:

"\e[5~": history-search-backward
"\e[6~": history-search-forward

@ambv ambv added the needs backport to 3.13 bugs and security fixes label Sep 6, 2024
@ambv ambv merged commit 8311b11 into python:main Sep 6, 2024
42 checks passed
@miss-islington-app
Copy link

Thanks @vstinner for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 6, 2024
…ythonGH-123607)

Change <page up> and <page down> keys of the Python REPL to history
search forward/backward.

(cherry picked from commit 8311b11)

Co-authored-by: Victor Stinner <[email protected]>
Co-authored-by: Łukasz Langa <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Sep 6, 2024

GH-123773 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Sep 6, 2024
@hroncok
Copy link
Contributor

hroncok commented Sep 6, 2024

Thak you, I'll check the new behavior out, but perhaps not in time for rc2

@vstinner vstinner deleted the repl_page_up branch September 6, 2024 11:31
@vstinner
Copy link
Member Author

vstinner commented Sep 6, 2024

See my comment in #123607 (comment) -- I think this is not good.

I looked at this scenario, but I failed to mimick readline's exact behavior in pyrepl which works on the temporary history while the history is being modified.

ambv added a commit that referenced this pull request Sep 6, 2024
…GH-123607) (GH-123773)

Change <page up> and <page down> keys of the Python REPL to history
search forward/backward.

(cherry picked from commit 8311b11)

Co-authored-by: Victor Stinner <[email protected]>
Co-authored-by: Łukasz Langa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants