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

Allow single option with REPL.TerminalMenus #36369

Merged
merged 8 commits into from
Jun 22, 2020

Conversation

tomyun
Copy link
Contributor

@tomyun tomyun commented Jun 20, 2020

As briefly mentioned in the recent API change of REPL.TerminalMenus (#35915), single choice menu (nick-paul/TerminalMenus.jl#20) is a feature implemented in the original repo then somehow missed a chance to be part of the stdlib.

The first half of this PR is mostly identical to the above patch with small changes. Basically it relaxes minimum value of pagesize to 1 instead of 2. A notable thing added to this PR was to fix the condition for page offset update in move_down!() to ensure proper scrolling when there is only one line in the page (pagesize=1). The recently changed code for this function has a slightly different condition than to what was originally implemented (old vs new).

The remaining part is not directly related to the single choice menu, but to improve handling of pagesize=1 which is now inevitably open to multiple options if users insist on using it. A main issue is that the current scrolling arrow can be only placed for one direction, but now we may end up with a single line with multiple options that can be scrolled to either direction.

For this, the logic in printcursor() to print out empty space when no arrow presents is now taken over by print_arrow(). updown_arrow() provides an additional symbol for the new arrow (I in ASCII, in Unicode by default). Then printcursor() is always called for each line regardless of existence of arrows.

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -362,10 +368,15 @@ down_arrow(c::AbstractConfig) = down_arrow(c.config)
down_arrow(c::Config) = c.down_arrow
down_arrow(::AbstractMenu) = CONFIG[:down_arrow]

print_arrow(buf, ::ConfiguredMenu, c::Char) = print(buf, c, " ")
updown_arrow(m::ConfiguredMenu) = updown_arrow(m.config)
updown_arrow(m::AbstractConfig) = updown_arrow(c.config)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
updown_arrow(m::AbstractConfig) = updown_arrow(c.config)
updown_arrow(c::AbstractConfig) = updown_arrow(c.config)

Maybe a sign that there is a missing test?

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 believe now it should get covered by a test with charset=:ascii added to trigger ConfiguredMenu instead of legacy Dict-based menu.

stdlib/REPL/src/TerminalMenus/AbstractMenu.jl Outdated Show resolved Hide resolved
@@ -57,3 +53,5 @@ end
# Test SDTIN
multi_menu = MultiSelectMenu(string.(1:10), charset=:ascii)
@test simulate_input(Set([1,2]), multi_menu, :enter, :down, :enter, 'd')
multi_menu = MultiSelectMenu(["single option"])
Copy link
Member

Choose a reason for hiding this comment

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

You have a CI failure due to tests being run with --depwarn=error.

Suggested change
multi_menu = MultiSelectMenu(["single option"])
multi_menu = MultiSelectMenu(["single option"], charset=:ascii)

and similarly for RadioMenu below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added charset=:ascii to other non-legacy tests.

I feel like it's a bit confusing behavior though, especially when we have an exception like pagesize kwarg doesn't trigger new interface.

Copy link
Member

Choose a reason for hiding this comment

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

I know. I don't think there's another way to get to a sane interface without breaking backwards compatibility. I was extremely pleased to find something so unintrusive, even if it's far from perfect. We can drop the CONFIG interface in Julia 2.0, but that will likely be years away.

@tomyun
Copy link
Contributor Author

tomyun commented Jun 20, 2020

To cope with some dynamic_menu UI errors, move_down!() is updated to behave identical to the original implementation.

@timholy timholy merged commit 76a2e36 into JuliaLang:master Jun 22, 2020
@timholy
Copy link
Member

timholy commented Jun 22, 2020

Thanks, @tomyun!

mbauman added a commit to dlfivefifty/julia that referenced this pull request Jun 26, 2020
* origin/master: (232 commits)
  Add passthrough for non-Markdown docs (JuliaLang#36091)
  Fix pointer to no longer assume contiguity (JuliaLang#36405)
  Ensure string-hashing is defined before it gets used (JuliaLang#36411)
  Make compilecache atomic (JuliaLang#36416)
  add a test for JuliaLang#30739 (JuliaLang#36395)
  Fix broken links in docstring of `repeat` (JuliaLang#36376)
  fix and de-dup cached calls to `methods_by_ftype` in compiler (JuliaLang#36404)
  ml-matches: skip unnecessary work, when possible (JuliaLang#36413)
  gf: fix some issues with the move from using a tree to a hash lookup of leaf types (JuliaLang#36413)
  Add news and manual entry for sincospi (JuliaLang#36403)
  Check axes in Array(::AbstractArray) (fixes JuliaLang#36220) (JuliaLang#36397)
  add versions of `code_typed` and `which` that accept tuple types (JuliaLang#36389)
  Fix spelling of readdir. (JuliaLang#36409)
  add sincospi (JuliaLang#35816)
  fix showing methods with unicode gensymed variable names (JuliaLang#36396)
  Add doctest: eachslice (JuliaLang#36386)
  fix documentation typo ("Ingeger")
  Refactor `abstract_eval` to separate out statements and values (JuliaLang#36350)
  fix return type of `get!` on `IdDict` (JuliaLang#36383)
  Allow single option with REPL.TerminalMenus (JuliaLang#36369)
  ...
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
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.

2 participants