gh-118878: Pyrepl: show completions menu below the current line #118939
Conversation
A crash of the new pyrepl was triggered by pressing up arrow when tab completion menu was displayed.
| screen[ly:ly] = self.cmpltn_menu | ||
| self.screeninfo[ly:ly] = [(0, [])]*len(self.cmpltn_menu) | ||
| self.cxy = self.cxy[0], self.cxy[1] + len(self.cmpltn_menu) |
There was a problem hiding this comment.
This is the big win here, we no longer need to mess with the cursor when displaying the menu!
The main advantage of this is that the behaviour is less janky, since the current line no longer jumps up and down. This also allows fixing the behaviour of arrow keys when the menu is displayed.
danielhollas
force-pushed
the
pyrepl/completions-below
branch
from
a1d95c9
to
eae92aa
Compare
This test does not work yet :-(
| @@ -274,7 +274,7 @@ def do(self) -> None: | |||
| x, y = r.pos2xy() | |||
| new_y = y + 1 | |||
|
|
|||
| if new_y > r.max_row(): | |||
| if r.eol() == len(b): | |||
There was a problem hiding this comment.
This an equivalent fix as in #118936
This reverts commit 9e2170c.
danielhollas
force-pushed
the
pyrepl/completions-below
branch
from
99af855
to
dd90c2e
Compare
danielhollas
requested review from
pablogsal,
lysnikolaou and
ambv
as code owners
|
@pablogsal would you mind adding the I am not sure there is enough time to get this in for next beta, but I am happy to work on this further if this is something people are happy with. |
|
CC @lysnikolaou as he was working on something like this for a bugfix |
| @@ -258,10 +258,15 @@ def after_command(self, cmd: Command) -> None: | |||
| def calc_complete_screen(self) -> list[str]: | |||
| screen = super().calc_complete_screen() | |||
| if self.cmpltn_menu_visible: | |||
| ly = self.lxy[1] | |||
| # We display the completions menu below the current prompt | |||
| ly = self.lxy[1] + 1 | |||
There was a problem hiding this comment.
I had a version of this already in the works and was going to open a PR but delayed it for the Windows PR to get in first. In my version, I just do screen.extend and it works without the need for the hack below. Or am I missing something?
There was a problem hiding this comment.
I am not sure, would you mind posting the relevant part of your patch? The problem is not with screen but screeninfo, which is used to calculate cursor position. Have you tried pressing the left or down arrow while the completion menu is displayed? Without the hack below the cursor would jump down into the completions menu in my case, but perhaps you solution works better.
There was a problem hiding this comment.
I think that this should be enough once #120042 lands.
diff --git a/Lib/_pyrepl/completing_reader.py b/Lib/_pyrepl/completing_reader.py
index c11d2dabdd2..db3c26c0627 100644
--- a/Lib/_pyrepl/completing_reader.py
+++ b/Lib/_pyrepl/completing_reader.py
@@ -258,10 +258,7 @@ def after_command(self, cmd: Command) -> None:
def calc_complete_screen(self) -> list[str]:
screen = super().calc_complete_screen()
if self.cmpltn_menu_visible:
- ly = self.lxy[1]
- screen[ly:ly] = self.cmpltn_menu
- self.screeninfo[ly:ly] = [(0, [])]*len(self.cmpltn_menu)
- self.cxy = self.cxy[0], self.cxy[1] + len(self.cmpltn_menu)
+ screen.extend(self.cmpltn_menu)
return screen
def finish(self) -> None:There was a problem hiding this comment.
I am going to have to look at that PR more closely, but just looking at this patch, it seems that the completions menu would always get appended at the very bottom of the screen right?
I thought about doing it this way as well, but I don't know if this will work well during multiline editing. You can imagine a corner case where you would be editing a large multiline code, and the completions menu will appear well below the current cursor position or even outside the visible screen.
(I am currently AFK so cannot test this)
There was a problem hiding this comment.
I am going to have to look at that PR more closely, but just looking at this patch, it seems that the completions menu would always get appended at the very bottom of the screen right?
I thought about doing it this way as well, but I don't know if this will work well during multiline editing. You can imagine a corner case where you would be editing a large multiline code, and the completions menu will appear well below the current cursor position or even outside the visible screen.
@lysnikolaou I have now tested your version and for large multiline editing it is indeed a problematic since the menu will always be displayed at the very bottom, even if you're editing a line at the very top of screen. For large enough editing context, the menu will not be visible at all (e.g. when editing a first line in a function with many lines. WDYT?
There was a problem hiding this comment.
Is this problem still relevant?
There was a problem hiding this comment.
To clarify, this PR has no problem afaik. I was pointing out issues with the suggestion above.
|
This has the problem that if the cursor is on the last line of the screen, completions are not shown (only when editing a multi-line block) |
danielhollas
force-pushed
the
pyrepl/completions-below
branch
from
f29168e
to
af90634
Compare
@pablogsal was your comment meant on my version of the code or on @lysnikolaou patch proposed in #118939 (comment)? I can reproduce the issue you describe with his patch but I cannot with the current version of PR. I merged in main and fixed up a test, and things still seem to be working. |
Lib/test/test_pyrepl/test_pyrepl.py
Outdated
| @@ -658,6 +659,33 @@ def test_updown_arrow_with_completion_menu(self): | |||
| # so we should end up where we were when we initiated tab completion. | |||
| output = multiline_input(reader, namespace) | |||
| self.assertEqual(output, "os.") | |||
| # TODO: The menu should be visible, but currently isn't? | |||
There was a problem hiding this comment.
Essentially, I wanted to assert stuff while the tab completions menu is displayed, but I failed to figure out how to do it. I suspect the multiline_input function is not a good vehicle for that.
It would be nice to have a test that asserts that the menu is indeed displayed below and that the cursor behaves in a sane way when user presses various arrow keys while the menu is displayed. But I will not have time to figure it out in the next week I am afraid.
For now I've removed the TODO and the test below.
danielhollas
force-pushed
the
pyrepl/completions-below
branch
from
f1e76ce
to
aa9a3a3
Compare
As proposed in #118878, it feels much more natural to me to display the completions menu below the current line, not above. The main advantage of this is that the UI is less janky,
since the current line that is being edited no longer jumps up and down.
This also allows fixing the behaviour of arrow keys when the menu is displayed, described in #119257.
The current behaviour is still not great when tab completing in the middle of multiline edit, but it is at least not worse as before AFAIK. I think ultimately this should be solved by separating out the completions menu, like ipython does it, but that seems outside of scope for this PR.
TODO:
Closes #119257. Closes #118878