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

Fix moving up and down in REPL #350

Merged
merged 3 commits into from
Jun 29, 2024
Merged

Fix moving up and down in REPL #350

merged 3 commits into from
Jun 29, 2024

Conversation

huangyxi
Copy link
Contributor

src/repl.jl Outdated
Comment on lines 288 to 289
php_idx = VERSION >= v"1.11.0-rc1" ? 6 : 5
p = mirepl.interface.modes[php_idx]::LineEdit.PrefixHistoryPrompt
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems brittle. Better to find first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I have improved the implementation by using findfirst. (8b6dc6f)

src/repl.jl Outdated
mirepl = isdefined(repl,:mistate) ? repl.mistate : repl
interface_modes = mirepl.interface.modes
main_mode = interface_modes[1]
php_idx = findfirst(isa.(interface_modes, LineEdit.PrefixHistoryPrompt))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
php_idx = findfirst(isa.(interface_modes, LineEdit.PrefixHistoryPrompt))
php_idx = findfirst(isa(LineEdit.PrefixHistoryPrompt), interface_modes)

Avoid allocating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, isa requires two arguments explicitly. Here is a workaround:

findfirst(Base.Fix2(isa, LineEdit.PrefixHistoryPrompt), interface_modes)

However, I'm uncertain about using Base.Fix2 since it hasn't been exported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. It's fine to use Fix2, it's public, just not exported https://docs.julialang.org/en/v1/base/base/#Base.Fix2

Copy link
Contributor

@IanButterworth IanButterworth left a comment

Choose a reason for hiding this comment

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

Seems good to me. Assuming the main mode is first is probably ok

@daviehh
Copy link
Contributor

daviehh commented Jun 28, 2024

Thanks for the quick fix! If you do not wish to hard code by assuming main mode is first entry, one can replace the main_mode = interface_modes[1] line w/ something like

get_prompt_str = m -> LineEdit.prompt_string(isdefined(m, :prompt) ? m.prompt : "")
mm_idx_ = findfirst(m -> get_prompt_str(m) == REPL.JULIA_PROMPT, interface_modes) # main mode index
mm_idx = isnothing(mm_idx_) ? 1 : mm_idx_
main_mode = interface_modes[mm_idx]

@KristofferC KristofferC merged commit 81d45af into KristofferC:master Jun 29, 2024
7 of 10 checks passed
@huangyxi huangyxi deleted the patch-keybinding branch July 3, 2024 09:27
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.

Fail to press UP or DOWN keys in REPL after v1.11.0-rc1
5 participants