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

Parser.parse()'s "cursor" argument is not the cursor position for ACCEPT_LINE #84

Closed
scgray opened this issue Jan 3, 2017 · 7 comments

Comments

@scgray
Copy link

scgray commented Jan 3, 2017

The definition of the method is:

ParsedLine parse(String line, int cursor, ParseContext context) throws SyntaxError;

default ParsedLine parse(String line, int cursor) throws SyntaxError {
    return parse(line, cursor, ParseContext.UNSPECIFIED);
}

but when called for ACCEPT_LINE the "cursor" argument is actually always line.length():

try {
    parsedLine = parser.parse(str, str.length(), ParseContext.ACCEPT_LINE);
} catch (EOFError e) {

(this is from LineReaderImpl.acceptLine().

For my purposes I really need to know the context in which the user hit accept line (presumably ENTER) -- that is, I need the "cursor" variable to mean what it's name indicates it should mean. For example, in my application a semicolon at the end of a line usually means "run it", but in the case of:

prompt> select * _from x;

if _ is the cursor position and the user hits enter, it would make logical sense for it to do:

prompt> select *
prompt> _from;
@gnodet
Copy link
Member

gnodet commented Jan 4, 2017

I don't mind passing the cursor position of course, though one problem will be when an history event has been expanded, in which case the cursor position will be lost.
Another option would be to bind the <enter> key to a custom widget which in turn would either insert a newline or call the accept widget.

@scgray
Copy link
Author

scgray commented Jan 4, 2017

Thanks Guillaume - I don't see a lot of documentation or examples of widgets right now (as an aside, I am planning on forking and writing some javadocs as I learn things). I'm not quite sure I see how a widget would have access to enough state information to be able to decipher the current cursor position and buffer contents -- at least not without assuming direct access to LineReaderImpl.

As for the history events, my application doesn't use them so that wouldn't be a problem for me.

As I write this, though, I'm having my doubts as to my idea of the behavior for this issue...I think users might find the behavior strange when sometimes, means newline and in other cases means "run". Is it possible to bind CTRL-ENTER to ACCEPT_LINE and have just ENTER be newline?

gnodet added a commit that referenced this issue Jan 4, 2017
@gnodet
Copy link
Member

gnodet commented Jan 4, 2017

Have a look at the 2 tests in the file below, which demonstrates the use of a custom widget and your second suggestion.
https://github.com/jline/jline3/blob/master/reader/src/test/java/org/jline/reader/impl/WidgetTest.java

gnodet added a commit that referenced this issue Jan 4, 2017
@gnodet
Copy link
Member

gnodet commented Jan 4, 2017

Fwiw, there's no simple way to find out a Ctrl+<enter>, that's because the <enter> key is already in the first 32 ascii codes, so it's already mapped to Ctrl+J.

@scgray
Copy link
Author

scgray commented Jan 5, 2017

Looking over your examples (thanks for that, I'm trying it now), I notice what appears to be a breaking of abstraction. You have interfaces for LineReader and Buffer, but I notice that some of what you are showing requires using methods on the *Impl classes. For example, LineReader.getBuffer() returns the actual BufferImpl in the definition of the method and reader.getKeyMap() is only defined in the Impl.

Maybe either those methods should be promoted to the interface, or maybe you shouldn't have the interface at all and just make them direct classes?

Also, one of the down sides of the widget approach is that it depends upon the keymap name and with vi your keymap name switches depending on mode (insert vs. command)

@gnodet
Copy link
Member

gnodet commented Jan 5, 2017

Yes, I'm aware of the BufferImpl problem. It will have to be fixed as part of #70 I think.
I'll fix the getKeyMap() one asap.
I've raised #86 for the API enhancements.

@scgray
Copy link
Author

scgray commented Jan 5, 2017

Ok, I got the widget to work, but I discovered that the enter key is actually CTRL-M. The other issue that I can now is that I need to review each key that can cause ACCEPT_LINE to be called for each keymap :-(. I think I would like to request if you could attempt to make "cursor" accurate when possible, that would make what I'm trying to do less error prone, particularly since I don't use history expansion.

@gnodet gnodet closed this as completed in 6f7a2e5 Jan 5, 2017
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

No branches or pull requests

2 participants