Improve the API of REPL.TerminalMenus #35915

timholy · 2020-05-17T07:24:41Z

This eliminates camelCase, ~~fixes a typo (which unfortunately was documented)~~, adds a const and type info to the CONFIG dict, and does some renaming/consolidation. Finally, it eliminates a dangerous escape hatch from one of the tests.

EDIT: the most recent version fixes #31414, fixes #32349, closes #30043, and closes #32344

stdlib/REPL/src/TerminalMenus/util.jl

stdlib/REPL/test/TerminalMenus/runtests.jl

stdlib/REPL/src/TerminalMenus/TerminalMenus.jl

timholy · 2020-05-17T20:26:30Z

OK, the more I dug the more I found to fix. I tackled this because I was looking to create a new menu type, which gave me an opportunity to exercise the abstraction API. Perhaps not surprisingly I discovered some issues. Two issues are:

it turns out options(m) was never used other than to calculate the number of items in the menu. It seems more reasonable, then, to support an interface that allows you to just pass this number rather than a whole vector of strings. (For the menu type I am implementing, it's not a static list so not having to generate a bunch of strings that will just be thrown away seems like a win.)
there's a bad leak in the API, subtypes of AbstractMenu are expected to access TerminalMenus.CONFIG to get the characters for the cursor position and, for a multiple-selection menu, the strings that indicate the selection state. They are then expected to print these characters as well as any data associated with the menu options itself. It's quite weird to have the configuration be at the level of TerminalMenus but to have the printing be at the level of the subtype and then have the subtype need to access private data of TerminalMenus.

I've tried to clean this up here, but it's still not the API we probably want for 2.0:

if TerminalMenus maintains the settings for these character, it seems it should handle their printing and dispatch writeline only for the remainder of the info about the menu item
if the subtypes are going to be responsible for printing the cursor marker and selection state strings, it seems these are options you should pass when you construct the menu rather than a configuration option for TerminalMenus as a whole.

But I don't see a non-breaking way to get to either of these. So this does the next best thing (?) and passes a copy of these settings back to writeline.

I think I've set this up so that all users of the legacy interface should continue to work, but that's not actually tested.

This removes 80 invalidations, ref JuliaLang/julia#35915 This will be breaking so I've bumped the version in anticipation. We might want to merge this only when we're ready to release the next breaking version.

This eliminates camelCase, fixes a typo (which unfortunately was documented), adds a `const` and type info to the CONFIG dict, and does some renaming/consolidation. Finally, it eliminates a dangerous escape hatch from one of the tests.

The old `writeLine` forced the user to take the (undocumented) step of looking up the cursor character in the internal CONFIG Dict. We can use the name change as an opportunity to improve the API.

The only way `options` was being used was to count the number, so it seems a bit silly to force the user to pass a vector. This will make it easier to support dynamic menus.

For menus whose number of options might change as a consequence of user input (e.g., foldable menus), this prevents indexing errors.

This should enhance the support for dynamic menus that might change the number of options offered.

timholy · 2020-06-02T20:38:54Z

I think this has reached a satisfying point. Changes since the last update:

I've added an intermediate abstract type, ConfiguredMenu, that resolves the API "leak" I describe above. Subtypes store the configuration in the menu itself, rather than in a global variable.
The printing of the "cursor"-selected indicator is handled automatically, just like it already was for the "up arrow" and "down arrow" characters. This ends the odd asymmetry among these indicators.
I've made the printing algorithm more robust for dynamic menus that might change their number of options. It now passes a state variable that allows it to know how many lines it needs to back up to erase the previous menu.
I've added a more thorough test of navigation and menu-printing (the new dynamic_menu.jl test file)
I've added "legacy" tests that use the original Julia 1.0 AbstractMenu API, and verified that they still work.

With one important exception, this AbstractMenu API now preserves backward compatibility while developing a cleaner interface for the future. The exception, unfortunately, is in the exported RadioMenu and MultiSelectMenu types, which I've migrated over to the new API. That means that instead of configuring them via TerminalMenus.config, you configure them at construction time. If we want to preserve compatibility, we should export the OldRadioMenu and OldMultiSelectMenu that I added as test examples, and figure out some new name for the new implementations.

Thoughts?

Fixes #32349 Closes #32344

tomyun · 2020-06-03T16:42:42Z

Nice to see this moving forward! I was just wondering if we could use this chance for incorporating upstream changes made after split (merge?) circa early 2018. A notable change I can think of is single choice menu (nick-paul/TerminalMenus.jl#20) which was a sole reason prevented me from using the REPL version of this package.

timholy · 2020-06-03T16:45:27Z

That can be viewed as a bugfix, so it would be fine to put that in any time. That said, you might as well wait for this to merge?

KristofferC

Looks like a nice improvement to the API to me.

timholy · 2020-06-08T22:04:09Z

Thanks, @KristofferC!

timholy · 2020-06-08T22:19:21Z

Added NEWS, will merge if CI passes.

yakir12 · 2020-06-09T07:53:39Z

(about the news, if this indeed allows for adding default option/s, shouldn't that be mentioned in the news?)

timholy · 2020-06-09T08:17:18Z

Maybe. I'm not actually sure what you mean by "adding default options," can you clarify?

Note docs for TerminalMenus are at https://docs.julialang.org/en/v1.6-dev/stdlib/REPL/#TerminalMenus-1. Amazingly, they've already been updated for this PR.

yakir12 · 2020-06-09T09:07:34Z

I'm only asking because this #32349 closed by this PR. As stated in that issue, default options would allow for the prompt to have the cursor already set to some default option for the RadioMenu, and for some default options in MultiSelectMenu to be pre-checked. This is useful when you are near certain about what the user will choose and want to save the extra scrolling and clicking by allowing the user to simply press done.

timholy · 2020-06-09T10:20:07Z

Oh, got it. Yes, I suppose the individual API additions are NEWS-worthy. See if #36210 helps (and feel free to edit it).

yakir12 · 2020-06-09T10:47:29Z

Perfect! Thank you very much.

This overhaul of the `AbstractMenu` extension interface of `REPL.TerminalMenus` addresses several concerns: - Printing the "paging" navigation indicators was handled by TerminalMenus, but the subtype methods were expected to print the cursor indicator and, for multiple-selection menus, the selection status indicators in a manner that preserved alignment. There was no obvious reason for the inconsistency. - Printing these indicators relied on accessing a mutable private global variable in TerminalMenus. It was therefore not possible to use different settings simultaneously for two different menus (e.g. a main menu and its submenus). - The API required that subtype methods supply a list of strings for each menu-option when really it only needed to know how many options were available. This was particularly problematic for large, dynamic menus whose options might change and for which lists of options would have to be reallocated regularly but were then thrown away after checking their length. This deprecates the old interface in a backward-compatible manner, so is non-breaking.

GunnarFarneback · 2020-11-11T08:57:14Z

I'm late to discover the wonders of TerminalMenus and I was delighted to find this PR when faced with the limitations of the API in Julia 1.5. However, I still see one glaring omission, namely that the cursor position is a local variable in the request loop. For reading purposes you can catch it from the writeline calls but there is no way to control it after the initialization.

Things that would be possible with a controllable cursor include jumping to a related option after a custom key press or seamlessly moving into a submenu related to the option at the cursor.

Ideally the cursor should have been a mandatory field in the menu struct. That's not possible to introduce now with respect to backwards compatibility of course. One possibility is to use a cursor field from the struct if it exists and fall back to a local variable otherwise, but I don't immediately see how to implement that in an elegant way. A simpler but less complete approach would be to allow keypress to return an Int if you want to change the cursor from that call (which is probably where it's most interesting).

@timholy, any thoughts?

timholy · 2020-11-11T09:30:38Z

I agree that it would have been nice to allow cursor to be part of the menu. Indeed, the hardest part of this cleanup was doing it in a way that maintained backwards compatibility while still moving us towards a cleaner implementation.

As an alternative to supporting an arbitrary Int as a return value for keypress, what about supporting Union{Int,RefValue{Int}} for cursor? Then anyone who wants to observe the value from the outside can do so.

GunnarFarneback · 2020-11-11T09:42:18Z

Ah, a Ref in the call to request. Yes, that's a clever approach. I'll do something with that.

rfourquet approved these changes May 17, 2020

View reviewed changes

stdlib/REPL/src/TerminalMenus/util.jl Outdated Show resolved Hide resolved

stdlib/REPL/test/TerminalMenus/runtests.jl Show resolved Hide resolved

rfourquet added the REPL Julia's REPL (Read Eval Print Loop) label May 17, 2020

ararslan reviewed May 17, 2020

View reviewed changes

stdlib/REPL/src/TerminalMenus/TerminalMenus.jl Outdated Show resolved Hide resolved

timholy mentioned this pull request May 18, 2020

Move type-pirating convert methods from ImageCore JuliaGraphics/ColorTypes.jl#196

Closed

timholy mentioned this pull request May 26, 2020

What's needed to include ClearStacktrace.jl in Base? #36026

Closed

timholy added 15 commits June 2, 2020 14:47

Improve style in REPL.TerminalMenus

f602094

This eliminates camelCase, fixes a typo (which unfortunately was documented), adds a `const` and type info to the CONFIG dict, and does some renaming/consolidation. Finally, it eliminates a dangerous escape hatch from one of the tests.

Improve documentation of options

191433b

eliminate raw! altogether

4d896a5

Improve the writeline API to prevent leak of internals

5b86abc

The old `writeLine` forced the user to take the (undocumented) step of looking up the cursor character in the internal CONFIG Dict. We can use the name change as an opportunity to improve the API.

Support starting on a line different from 1 in request

3f369a6

Provide noptions as an alternative to options

8937a72

The only way `options` was being used was to count the number, so it seems a bit silly to force the user to pass a vector. This will make it easier to support dynamic menus.

Improve abstraction

a955ac0

Improve support for dynamic menus

5725a93

For menus whose number of options might change as a consequence of user input (e.g., foldable menus), this prevents indexing errors.

Handle state in menu-printing

af625a2

This should enhance the support for dynamic menus that might change the number of options offered.

reword

5807d79

more deprecate

291305c

Introduce ConfiguredMenu

c16b616

Rename cursor::Bool -> iscursor

2ccb78e

Add legacy tests

b86c7a3

Split movement commands for easier testing & add dynamic menu test

70a5f63

timholy force-pushed the teh/cleanup_terminalmenus branch from ca72b97 to 70a5f63 Compare June 2, 2020 19:48

laborg mentioned this pull request Jun 2, 2020

Fix doc: pressing ctrl-c doesn't return -1 in a RadioMenu [ci skip] #36114

Closed

timholy added 2 commits June 2, 2020 15:41

Update the docs

c64de7e

Wrap raw! in try/catch

2b24b82

timholy changed the title ~~Improve style in REPL.TerminalMenus~~ Improve the API of REPL.TerminalMenus Jun 3, 2020

Allow deprecation of the old RadioMenu/MultiSelectMenu API

2f4c68a

timholy requested a review from StefanKarpinski June 3, 2020 09:21

timholy added 2 commits June 3, 2020 04:46

Add preselection of items in MultiSetMenu

2a6d38b

Fixes #32349 Closes #32344

fix and enhance docs

648a06e

mbauman mentioned this pull request Jun 4, 2020

ProjectFile has a None option JuliaCI/PkgTemplates.jl#182

Open

This was referenced Jun 8, 2020

Second "using" is slow (slower than third, can even be slower than the first) #36005

Closed

avoid invalidations when the overlap is ambiguous #35877

Closed

KristofferC approved these changes Jun 8, 2020

View reviewed changes

Add NEWS

99eab4e

timholy merged commit 36270e9 into master Jun 9, 2020

timholy deleted the teh/cleanup_terminalmenus branch June 9, 2020 07:34

timholy mentioned this pull request Jun 11, 2020

Implement TreeMenu, an interactive REPL folding-tree menu JuliaCollections/FoldingTrees.jl#3

Merged

tomyun mentioned this pull request Jun 20, 2020

Allow single option with REPL.TerminalMenus #36369

Merged

nickrobinson251 mentioned this pull request Aug 26, 2020

Create templates from templates JuliaCI/PkgTemplates.jl#38

Open

GunnarFarneback mentioned this pull request Nov 11, 2020

Accept RefValue cursor as TerminalMenu request argument. #38393

Merged

timholy mentioned this pull request Jan 12, 2021

Implement basic pager using TerminalMenus #38956

Merged

timholy mentioned this pull request Aug 7, 2021

Consider reverting #37940 #41799

Closed

oxinabox mentioned this pull request May 8, 2024

deprecation warning in generate(): Warning: Legacy MultiSelectMenu interface is deprecated JuliaCI/PkgTemplates.jl#471

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the API of REPL.TerminalMenus #35915

Improve the API of REPL.TerminalMenus #35915

timholy commented May 17, 2020 •

edited

Loading

timholy commented May 17, 2020 •

edited

Loading

timholy commented Jun 2, 2020

tomyun commented Jun 3, 2020

timholy commented Jun 3, 2020

KristofferC left a comment

timholy commented Jun 8, 2020

timholy commented Jun 8, 2020

yakir12 commented Jun 9, 2020

timholy commented Jun 9, 2020

yakir12 commented Jun 9, 2020

timholy commented Jun 9, 2020

yakir12 commented Jun 9, 2020

GunnarFarneback commented Nov 11, 2020

timholy commented Nov 11, 2020 •

edited

Loading

GunnarFarneback commented Nov 11, 2020

Improve the API of REPL.TerminalMenus #35915

Improve the API of REPL.TerminalMenus #35915

Conversation

timholy commented May 17, 2020 • edited Loading

timholy commented May 17, 2020 • edited Loading

timholy commented Jun 2, 2020

tomyun commented Jun 3, 2020

timholy commented Jun 3, 2020

KristofferC left a comment

Choose a reason for hiding this comment

timholy commented Jun 8, 2020

timholy commented Jun 8, 2020

yakir12 commented Jun 9, 2020

timholy commented Jun 9, 2020

yakir12 commented Jun 9, 2020

timholy commented Jun 9, 2020

yakir12 commented Jun 9, 2020

GunnarFarneback commented Nov 11, 2020

timholy commented Nov 11, 2020 • edited Loading

GunnarFarneback commented Nov 11, 2020

timholy commented May 17, 2020 •

edited

Loading

timholy commented May 17, 2020 •

edited

Loading

timholy commented Nov 11, 2020 •

edited

Loading