Skip to content

Use upstream replxx#30143

Merged
kitaisreal merged 3 commits intoClickHouse:masterfrom
amosbird:useupstreamreplxx
Oct 19, 2021
Merged

Use upstream replxx#30143
kitaisreal merged 3 commits intoClickHouse:masterfrom
amosbird:useupstreamreplxx

Conversation

@amosbird
Copy link
Copy Markdown
Collaborator

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Now clickhouse-client supports native multi-line editing.

Switch to upstream replxx because it now contains all our patches plus more features such as multi-line editing. Use -n alone and paste multiple lines of text to try it out.

Detailed description / Documentation draft:
.

@robot-clickhouse robot-clickhouse added the pr-improvement Pull request with some product improvements label Oct 13, 2021
@robot-ch-test-poll2 robot-ch-test-poll2 added the submodule changed At least one submodule changed in this PR. label Oct 13, 2021
@kitaisreal kitaisreal self-assigned this Oct 13, 2021
@azat
Copy link
Copy Markdown
Member

azat commented Oct 13, 2021

Great!

I've looked at the replxx from this PR and here a few bits:

  • CTRL-J does not works as ENTER, this can be fixed by adding a custom bind in
    rx.bind_key(Replxx::KEY::control('N'), [this](char32_t code) { return rx.invoke(Replxx::ACTION::HISTORY_NEXT, code); });
  • it does not reads correctly multiline comments from the existing history files (I remember that you've mentioned something about multiline format changes), I guess it is ok, but maybe if it is simple enough than it can be added to
    void convertHistoryFile(const std::string & path, replxx::Replxx & rx)
    although I'm not sure

@amosbird
Copy link
Copy Markdown
Collaborator Author

it does not reads correctly multiline comments from the existing history files (I remember that you've mentioned something about multiline format changes), I guess it is ok,

I think we should use the upstream's behavior. Add the conversion will be expensive for large history files (we will need to check the file everytime)

@kitaisreal kitaisreal merged commit 0b39269 into ClickHouse:master Oct 19, 2021
azat added a commit to azat/ClickHouse that referenced this pull request Nov 22, 2021
Before this patch, replxx history_next/history_previous (which were
mapped to UP/DOWN) navigates through history lines first (in multi-line
mode, and after through history.

But it is not convenient, since in case of query contains lots of line
it can take awhile.

So now, after this update, replxx will change the meaning of
history_next/history_previous (which will be binded to M-UP/M-DOWN),
those actions will always navigates through history lines.

And new actions, line_next/line_previous (binded to UP/DOWN) will
navigate through lines of current item first, and when it reaches
begin/end it will go the previous/next history item.

Also note, that ClickHouse rebind of C-P/C-N is fine, since it is binded to
history_next/history_previous, and this is desired (like in readline).

Follow-up for: ClickHouse#30143 (cc: @amosbird)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-improvement Pull request with some product improvements submodule changed At least one submodule changed in this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants