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.

Sign up for GitHub

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

Jump to bottom

gh-118878: Pyrepl: show completions menu below the current line #118939

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
+17 −9
Open

gh-118878: Pyrepl: show completions menu below the current line #118939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
dc98b55
gh-118877: Fix AssertionError crash in pyrepl
danielhollas May 10, 2024
eae92aa
gh-118878: pyrepl: Show completion menu below current line
danielhollas May 11, 2024
9e2170c
WIP: Add test
danielhollas May 12, 2024
da921c1
Revert "WIP: Add test"
danielhollas May 22, 2024
dd90c2e
Merge branch 'main' into pyrepl/completions-below
danielhollas May 22, 2024
4498c29
Add test
danielhollas May 22, 2024
14805a8
Fix test
danielhollas May 22, 2024
119c01d
Get rid of spurious output in test_interact.py
danielhollas May 22, 2024
c5407d5
Merge branch 'main' into pyrepl/completions-below
danielhollas May 31, 2024
af90634
Merge branch 'main' into pyrepl/completions-below
danielhollas Jun 24, 2024
d150136
Fix test
danielhollas Jun 24, 2024
f7f346a
Merge branch 'main' into pyrepl/completions-below
danielhollas Jul 14, 2024
ce0641d
Add blurb
danielhollas Jul 14, 2024
aa9a3a3
Remove test
danielhollas Jul 14, 2024
c686ce2
Merge branch 'main' into pyrepl/completions-below
danielhollas Sep 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Lib/_pyrepl/commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ def do(self) -> None:
x, y = r.pos2xy()
new_y = y + 1

if new_y > r.max_row():
if r.eol() == len(b):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This an equivalent fix as in #118936

if r.historyi < len(r.history):
r.select_item(r.historyi + 1)
r.pos = r.eol(0)
Expand All @@ -309,7 +309,7 @@ def do(self) -> None:
class left(MotionCommand):
def do(self) -> None:
r = self.reader
for i in range(r.get_arg()):
for _ in range(r.get_arg()):
p = r.pos - 1
if p >= 0:
r.pos = p
Expand All @@ -321,7 +321,7 @@ class right(MotionCommand):
def do(self) -> None:
r = self.reader
b = r.buffer
for i in range(r.get_arg()):
for _ in range(r.get_arg()):
p = r.pos + 1
if p <= len(b):
r.pos = p
Expand Down
11 changes: 8 additions & 3 deletions Lib/_pyrepl/completing_reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,10 +260,15 @@ def after_command(self, cmd: Command) -> None:
def calc_screen(self) -> list[str]:
screen = super().calc_screen()
if self.cmpltn_menu_visible:
ly = self.lxy[1]
# We display the completions menu below the current prompt
ly = self.lxy[1] + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Is this problem still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, this PR has no problem afaik. I was pointing out issues with the suggestion above.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the big win here, we no longer need to mess with the cursor when displaying the menu!

# If we're not in the middle of multiline edit, don't append to screeninfo
# since that screws up the position calculation in pos2xy function.
# This is a hack to prevent the cursor jumping
# into the completions menu when pressing left or down arrow.
if self.pos != len(self.buffer):
self.screeninfo[ly:ly] = [(0, [])]*len(self.cmpltn_menu)
return screen

def finish(self) -> None:
Expand Down
3 changes: 2 additions & 1 deletion Lib/test/test_pyrepl/test_pyrepl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,7 +763,7 @@ def test_global_namespace_completion(self):
output = multiline_input(reader, namespace)
self.assertEqual(output, "python")

def test_updown_arrow_with_completion_menu(self):
def test_up_down_arrow_with_completion_menu(self):
"""Up arrow in the middle of unfinished tab completion when the menu is displayed
should work and trigger going back in history. Down arrow should subsequently
get us back to the incomplete command."""
Expand All @@ -773,6 +773,7 @@ def test_updown_arrow_with_completion_menu(self):
events = itertools.chain(
code_to_events(code),
[
Event(evt="key", data="down", raw=bytearray(b"\x1bOB")),
Event(evt="key", data="up", raw=bytearray(b"\x1bOA")),
Event(evt="key", data="down", raw=bytearray(b"\x1bOB")),
],
Expand Down
4 changes: 2 additions & 2 deletions Lib/test/test_pyrepl/test_reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,8 @@ def test_completions_updated_on_key_press(self):

actual = reader.screen
self.assertEqual(len(actual), 2)
self.assertEqual(actual[0].rstrip(), "itertools.accumulate(")
self.assertEqual(actual[1], f"{code}a")
self.assertEqual(actual[0], f"{code}a")
self.assertEqual(actual[1].rstrip(), "itertools.accumulate(")

def test_key_press_on_tab_press_once(self):
namespace = {"itertools": itertools}
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2024-07-14-23-19-20.gh-issue-119257.9OEzcN.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Show tab completions menu below the current line, which results in less
janky behaviour, and fixes a cursor movement bug. Patch by Daniel Hollas
Loading