make minimal completion display more bash-like #2241

melvinw · 2025-01-31T00:30:33Z

There are three commits here. One makes it so only the nice completion display sets horizontal-scroll-mode. This should help with #2081. The other changes the display style of the minimal completion display. Instead of only printing a subset of matches in a single column and saying that there are more, we now just print them all in the packed format. This behavior will be more similar to what bash users are familiar with and just generally more useful. The last change makes the minimal completion display the default. This should minimize friction for newcomers.

melvinw · 2025-01-31T00:32:46Z

Here are some screencasts of how the new minimal mode behaves.

Line wrapping: https://asciinema.org/a/0kltGyy5yse6oYavHwRkite2y

Packed match printing and search: https://asciinema.org/a/iWcwC8m0XOa7lpnB2sRtKmzCy

This commit changes the display style of the minimal completion display. Instead of only printing a subset of matches in a single column and saying that there are more, we now just print them all in the packed format. This behavior will be more similar to what bash users are familiar with and just generally more useful.

andychu

OK great, I looked at the code

Let me try out the behavior a bit

andychu · 2025-01-31T17:27:03Z

core/comp_ui.py

    def _RedrawPrompt(self):
        # type: () -> None
        # NOTE: This has to reprint the prompt and the command line!
        # Like bash, we SAVE the prompt and print it, rather than re-evaluating it.
        self.f.write(self.prompt_state.last_prompt_str)
        self.f.write(self.comp_state.line_until_tab)

+    def _GetTerminalWidth(self):


I think we can move this _GetTerminalWidth method up to the base class _IDisplay?

andychu · 2025-01-31T17:28:32Z

core/comp_ui.py

+                # This shouldn't raise IOError because we did it at startup!  Under
+                # rare circumstances stdin can change, e.g. if you do exec <&
+                # input.txt.  So we have a fallback.
+                self.term_width = 80


We can change this 80 to DEFAULT_TERMINAL_WIDTH I guess

andychu · 2025-01-31T17:29:09Z

core/shell.py

+            try:
+                term_width = libc.get_terminal_width()
+            except (IOError, OSError):  # stdin not a terminal
+                pass


I think we should set term_width to DEFAULT_TERM_WIDTH here, for BOTH 'nice' and 'minimal'

Then the logic below will be cleaner

We can also get rid of term_width = 0 then, since it will be initialized in both cases

andychu · 2025-01-31T17:29:47Z

core/shell.py

@@ -1106,7 +1111,8 @@ def Main(

        else:  # Without readline module
            display = comp_ui.MinimalDisplay(comp_ui_state, prompt_state,
-                                             debug_f)
+                                             debug_f, DEFAULT_TERM_WIDTH,


without GNU readline, I think we can still use the real term width?

andychu · 2025-01-31T17:39:54Z

I tried it out -- the main thing is if I hit TAB with nothing on the prompt, it writes over the entire screen in an undesirable way, with thousands of candidates for the first word / executable

Can we set max_lines to maybe 5 or 8 ?

That could be overridden with some user setting, but I think the "out of the box" / "zero config" experience should be at least decent

I don't like the bash behavior of "show 1888 possibilities?"

andychu

Thanks, looks good!

andychu · 2025-02-01T04:31:46Z

core/comp_ui.py

@@ -340,6 +354,10 @@ def __init__(
        # hash of matches -> count.  Has exactly ONE entry at a time.
        self.dupes = {}  # type: Dict[int, int]

+    def ReadlineInitCommands(self):
+        # type: () -> List[str]
+        return ['set horizontal-scroll-mode on']


Please add a comment with this link, so we remember what it's for!

#257

Nice catch! Added a comment

andychu · 2025-02-01T04:33:12Z

core/comp_ui.py

        self.comp_state = comp_state
        self.prompt_state = prompt_state
        self.num_lines_cap = num_lines_cap
        self.f = f
        self.debug_f = debug_f
+        self.term_width = DEFAULT_TERM_WIDTH


I think we should have a free function _GetTermWidth() that does the try/except, and that will be called in __init__ here

And then we will have a method self._MaybeGetTermWidth() that does the SIGWINCH thing

that will have a bit less duplication with DEFAULT_TERM_WIDTH

Good idea. Pulled that logic out of the method. I left the method name alone, though. "Maybe" suggests that it might fail or something and return None/-1, which I thought was more confusing than the name collision (the "self." disambiguates this enough IMO)

andychu · 2025-02-02T04:20:31Z

Thanks, code looks good

I was about to merge and then I saw this failure!

http://mb.oilshell.org/uuu/github-jobs/9044/

That's a little scary, but I think it is a mycpp bug. I think we need a different name for the method and function?

Because in C++ you can call a method like MyMethod(), it's not this->MyMethod() . So I think the method is getting called in the constructor, not the function.

I am not sure that is the cause but I think so ... we are doing a naive syntactic translation

Also I was wondering if it is weird to call a function in the constructor, in Alloc<T>(), but that might be OK

AddressSanitizer:DEADLYSIGNAL
=================================================================
==15622==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000001c (pc 0x55cb21d55fd3 bp 0x7fff089e6910 sp 0x7fff089e68f0 T0)
==15622==The signal is caused by a READ memory access.
==15622==Hint: address points to the zero page.
    #0 0x55cb21d55fd2 in iolib::SignalSafe::PollSigWinch() (/home/uke/oil/_bin/cxx-asan/oils-for-unix+0x13afd2)
    #1 0x55cb21e09948 in comp_ui::_IDisplay::_GetTerminalWidth() _gen/bin/oils_for_unix.mycpp.cc:21086
    #2 0x55cb21e09896 in comp_ui::_IDisplay::_IDisplay(comp_ui::State*, comp_ui::PromptState*, int, mylib::Writer*, util::_DebugFile*, iolib::SignalSafe*) _gen/bin/oils_for_unix.mycpp.cc:21081
    #3 0x55cb21e09f88 in comp_ui::MinimalDisplay::MinimalDisplay(comp_ui::State*, comp_ui::PromptState*, util::_DebugFile*, iolib::SignalSafe*) _gen/bin/oils_for_unix.mycpp.cc:21128
    #4 0x55cb22119b2f in comp_ui::MinimalDisplay* Alloc<comp_ui::MinimalDisplay, comp_ui::State*&, comp_ui::PromptState*&, util::_DebugFile*&, iolib::SignalSafe*&>(comp_ui::State*&, comp_ui::PromptState*&, util::_DebugFile*&, iolib::SignalSafe*&) (/home/uke/oil/_bin/cxx-asan/oils-for-unix+0x4feb2f)
    #5 0x55cb22005b76 in shell::Main(BigStr*, args::Reader*, Dict<BigStr*, BigStr*>*, bool, pyutil::_ResourceLoader*, py_readline::Readline*) _gen/bin/oils_for_unix.mycpp.cc:59043
    #6 0x55cb21d75732 in oils_for_unix::AppBundleMain(List<BigStr*>*) _gen/bin/oils_for_unix.mycpp.cc:10731
    #7 0x55cb21d75de5 in oils_for_unix::main(List<BigStr*>*) _gen/bin/oils_for_unix.mycpp.cc:10767
    #8 0x55cb220091d8 in main _gen/bin/oils_for_unix.mycpp.cc:59180
    #9 0x7f0aa4ce509a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #10 0x55cb21d53fd9 in _start (/home/uke/oil/_bin/cxx-asan/oils-for-unix+0x138fd9)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/home/uke/oil/_bin/cxx-asan/oils-for-unix+0x13afd2) in iolib::SignalSafe::PollSigWinch()
==15622==ABORTING

test/ysh-runtime-errors.sh: fatal: Should fail under YSH: expected status 3, got 1
FAIL  test-bug-2129

melvinw · 2025-02-02T04:30:13Z

Oh weird... Pushed a fix to rename the method

melvinw · 2025-02-02T04:33:16Z

Also, it should be safe to call _GetTerminalWidth() in the constructor. It catches any exceptions that might otherwise bubble up and cause problems

melvinw · 2025-02-02T04:41:02Z

Huh. Looks like the translation bug is fixed, but there's still this other failure in cpp-spec:

...
done: all-tests-to-html
builtin-trap failed with status 1

*** 1 tests FAILED
test/spec-version.sh: line 71: zsh: command not found

Results: file:///home/uke/oil/_tmp/spec/osh-cpp/index.html

real	3m6.446s
user	5m12.757s
sys	3m40.627s
Comparison: file:///home/uke/oil/_tmp/spec/osh-cpp/compare.html

https://mb.oilshell.org/uuu/github-jobs/9045/cpp-spec.wwz/_tmp/soil/logs/osh-all.txt

andychu · 2025-02-02T04:42:52Z

I think the error might be because some of the members aren't initialized yet or something ... ASAN is catching it

But in any case we are lucky that it caught it

It looks like this test is failing in Python only, not sure why:

http://mb.oilshell.org/uuu/github-jobs/9045/cpp-spec.wwz/_tmp/spec/osh-cpp/builtin-trap.html

http://mb.oilshell.org/uuu/github-jobs/9045/cpp-spec.wwz/_tmp/spec/osh-cpp/builtin-trap.html#details-20-osh

hmmmm

andychu · 2025-02-02T15:58:41Z

Hm I restarted the job, and it passed ... that is annoying, I haven't seen this flaky test before

Anyway it's merged now - thanks!

only set horizontal-scroll-mode for nice display

88d0dcc

melvinw added 2 commits January 30, 2025 17:12

use minimal completion display by default

10ae484

melvinw force-pushed the melvinw-bash-completion-display branch from 1e490f5 to 10ae484 Compare January 31, 2025 01:13

melvinw changed the base branch from master to soil-staging January 31, 2025 01:24

andychu reviewed Jan 31, 2025

View reviewed changes

address reviews

066da5f

andychu reviewed Feb 1, 2025

View reviewed changes

refactor term width getter, leave breadcrumb

e5df1c9

workaround for translation bug

2553f71

andychu merged commit 04df113 into soil-staging Feb 2, 2025
18 checks passed

melvinw deleted the melvinw-bash-completion-display branch February 2, 2025 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make minimal completion display more bash-like #2241

make minimal completion display more bash-like #2241

melvinw commented Jan 31, 2025

melvinw commented Jan 31, 2025 •

edited

Loading

andychu left a comment

andychu Jan 31, 2025

andychu Jan 31, 2025

andychu Jan 31, 2025

andychu Jan 31, 2025

andychu Jan 31, 2025

andychu commented Jan 31, 2025 •

edited

Loading

andychu left a comment

andychu Feb 1, 2025

melvinw Feb 2, 2025

andychu Feb 1, 2025

melvinw Feb 2, 2025

andychu commented Feb 2, 2025 •

edited

Loading

melvinw commented Feb 2, 2025

melvinw commented Feb 2, 2025 •

edited

Loading

melvinw commented Feb 2, 2025 •

edited

Loading

andychu commented Feb 2, 2025

andychu commented Feb 2, 2025

make minimal completion display more bash-like #2241

make minimal completion display more bash-like #2241

Conversation

melvinw commented Jan 31, 2025

melvinw commented Jan 31, 2025 • edited Loading

andychu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andychu commented Jan 31, 2025 • edited Loading

andychu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andychu commented Feb 2, 2025 • edited Loading

melvinw commented Feb 2, 2025

melvinw commented Feb 2, 2025 • edited Loading

melvinw commented Feb 2, 2025 • edited Loading

andychu commented Feb 2, 2025

andychu commented Feb 2, 2025

melvinw commented Jan 31, 2025 •

edited

Loading

andychu commented Jan 31, 2025 •

edited

Loading

andychu commented Feb 2, 2025 •

edited

Loading

melvinw commented Feb 2, 2025 •

edited

Loading

melvinw commented Feb 2, 2025 •

edited

Loading