search

magicant / yash

Yet another shell
http://magicant.github.io/yash/
GNU General Public License v2.0
346 stars 30 forks source link

Implement emacs-search-{forward,backward}-current. #37

Closed vext01 closed 1 year ago

vext01 commented 1 year ago

Here's a very quick attempt at the feature we discussed over here

A variant of emacs-search-backward which initialises the search buffer with the contents of the line edit buffer.

This seems to work, but since I'm not very familiar with the codebase, there may be errors.

Does this look like it's going in the right direction?

Questions:

If you try that with:

bindkey -e '\U' emacs-search-backward-current
bindkey -e '\U' srch-continue-backward

Then you don't get what you expected. What confuses me is that by default \^R is bound to different actions depending on the mode. Is it possible?

Thanks

vext01 commented 1 year ago

Just noticed a typo (fixed).

Would we want a emacs-search-forward-current for parity?

magicant commented 1 year ago

Thank you for pull-requesting!

  • Would we want a emacs-search-forward-current for parity? Admittedly, I don't see why you'd want to start searching the history forwards, but since emacs-search-forward exists, I guess there must be some reason.

emacs-search-forward is just an imitation of emacs and bash that perform forward search with <ctrl-s>. Maybe somebody find this useful when they happened to perform emacs-search-backward too many times? I think it's nice to have the emacs-search-*-current commands for both directions, which will make them more consistent with other search commands.

  • To be able to test the utility of this properly, I'd have to bind the keys how I'm used to them from fish. I'd like:

    • emacs-search-backward-current if you are not already in emacs search mode.
    • srch-continue-backward if you are already in emacs search mode.

If you try that with:

bindkey -e '\U' emacs-search-backward-current
bindkey -e '\U' srch-continue-backward

Then you don't get what you expected. What confuses me is that by default \^R is bound to different actions depending on the mode. Is it possible?

We have different modes before and after entering the search. bindkey -e affects the binding used before entering the search, and there is no bindkey option to modify the binding for the mode after entering the search. The latter is not implemented because allowing arbitrary bindings during the search would corrupt the editor state.

vext01 commented 1 year ago

I think it's nice to have the emacs-search-*-current commands for both directions, which will make them more consistent with other search commands.

OK, I will implement the forward search function shortly.

The latter is not implemented because allowing arbitrary bindings during the search would corrupt the editor state.

That's a shame.

Can you elaborate on the state-corruption a bit?

Thanks

magicant commented 1 year ago

The behavior of the search has been tested only with the hard-coded key bindings, so I don't know what would happen if any different bindings are used.

vext01 commented 1 year ago

I've pushed commits to:

Question: I had to edit lineedit/commands.in to make it build, but I note that this file is in .gitignore. Is this supposed to be generated? What should I do?

magicant commented 1 year ago

Question: I had to edit lineedit/commands.in to make it build, but I note that this file is in .gitignore. Is this supposed to be generated? What should I do?

It is auto-generated from lineedit/editing.h. Note the /*C*/ markers in that file. Makefile is expected to do the necessary regeneration as long as lineedit/editing.h is updated correctly.

vext01 commented 1 year ago

Thanks. I'll update that later.

I also noticed that the manual page is generated differently when I build yash manually (outside of the OpenBSD ports tree):

--- /usr/local/man/man1/yash.1  Tue Nov 14 09:27:46 2023
+++ /home/edd/source/yash/doc/yash.1    Thu Nov 23 10:09:51 2023
@@ -1,7237 +1,4876 @@
-'\" t
-.\"     Title: yash
-.\"    Author: Yuki Watanabe <magicant@users.osdn.me>
-.\" Generator: DocBook XSL Stylesheets vsnapshot <http://docbook.sf.net/>
-.\"      Date: 2023-08-20
-.\"    Manual: \ \&
-.\"    Source: \ \& 2.55
-.\"  Language: English
-.\"
-.TH "YASH" "1" "2023\-08\-20" "\ \& 2\&.55" "\ \&"
-.\" -----------------------------------------------------------------
-.\" * Define some portability stuff
-.\" -----------------------------------------------------------------
-.\" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-.\" http://bugs.debian.org/507673
-.\" http://lists.gnu.org/archive/html/groff/2009-02/msg00013.html
-.\" ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-.ie \n(.g .ds Aq \(aq
-.el       .ds Aq '
-.\" -----------------------------------------------------------------
-.\" * set default formatting
-.\" -----------------------------------------------------------------
-.\" disable hyphenation
-.nh
-.\" disable justification (adjust text to left margin only)
-.ad l
-.\" -----------------------------------------------------------------
-.\" * MAIN CONTENT STARTS HERE *
-.\" -----------------------------------------------------------------
-.SH "NAME"
-yash \- a POSIX\-compliant command line shell
+.\"Generated by db2man.xsl. Don't modify this, modify the source.
+.de Sh \" Subsection
+.br
+.if t .Sp
+.ne 5
+.PP
+\fB\\$1\fR
+.PP
+..
+.de Sp \" Vertical space (when we can't use .PP)
+.if t .sp .5v
+.if n .sp
+..
+.de Ip \" List item
+.br
+.ie \\n(.$>=3 .ne \\$3
+.el .ne 3
+.IP "\\$1" \\$2
+..
+.TH "YASH" 1 "2023-11-23" "" "YASH(1)"
+.SH NAME
+yash \- a POSIX-compliant command line shell
 .SH "SYNOPSIS"
-.sp
-\fByash [options\&...] [\-\-] [operands\&...]\fR
+
+
+\fIyash [options...] [\-\-] [operands...]\fR
+
...

As a result, the manual page doesn't look right when put through mandoc when yash was built manually.

Any idea why this could be?

(unrelated to this change, but makes it hard for me to check if my man changes are right)

magicant commented 1 year ago

Seems we're using different backends for generating the file, but I don't know anything about such internals of asciidoc...

vext01 commented 1 year ago

Ah, I see. The release tarball has a pre-generated manual.

vext01 commented 1 year ago

On Debian I get a manpage like yours. On OpenBSD I get a broken manpage.

Debain: asciidoc 10.2.0 OpenBSD: asciidoc 9.0.4

It's probably to do with that. I may try updating asciidoc in OpenBSD.

vext01 commented 1 year ago

It is auto-generated from lineedit/editing.h

I've already updated that file, so no changes required for that.

Should I write tests for the new functions?

vext01 commented 1 year ago

And in the manual page, my new commands are formatted like this:

           emacs-search-forward-current
               The same as “emacs-search-forward”, but the search is initialised with the current contents of the line-edit buffer.

Are those the right quotes to use? Not sure.

magicant commented 1 year ago

Should I write tests for the new functions?

Unfortunately, there have been very few tests about line-editing. The behavior of line-editing is too complex to test with the current script-based testing. But I think there is something that needs to be updated in tests/bindkey-y.tst to integrate the new command. Try make check to see if it works.

And in the manual page, my new commands are formatted like this: Are those the right quotes to use? Not sure.

Great. No quotation marks would be consistent with other existing parts of the manual, but the double quotes would be ok.

vext01 commented 1 year ago

Done. Just needs Japanese manual updates and then some user-testing now I think.

magicant commented 1 year ago

Okay, when you think you're ready please remove the draft flag from the pull request.

vext01 commented 1 year ago

I've just done some testing. At first I wasn't sure how emacs-search-forward-current should work, but now I think I see.

It appears that when you do a search (in either direction) and escape out of the search, yash remembers where you searched up until and that is used for the starting point of subsequent searches.

So if my search history is (oldest first):

ls /1
ls /2
ls /3

I can:

If that sounds right, then I think we are all good.

This work needs squashing. Shall I do that now, or will you do it after adding the japanese docs?

vext01 commented 1 year ago

Probably I'd like to use the squash option when I push the merge button on this pull request

If it's OK with you, I'd like to do the squashing, so I can sign the resulting commit with my SSH key. Is that OK with you?

magicant commented 1 year ago

If it's OK with you, I'd like to do the squashing, so I can sign the resulting commit with my SSH key. Is that OK with you?

Sure, no problem!

vext01 commented 1 year ago

Squashed and signed :)

magicant commented 1 year ago

Thank you!!