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

Adds vi-mode ex command :read. #1223

Merged
merged 2 commits into from
Jan 3, 2024
Merged

Conversation

mychris
Copy link
Contributor

@mychris mychris commented Jan 2, 2024

Only a partial implementation.
Executing a command and inserting its output is not yet supported.

The range currently does not distinguish between :0 and :1 so it is not possible to insert at the beginning of the file.

https://vimdoc.sourceforge.net/htmldoc/insert.html#:r

Copy link
Member

@cxxxr cxxxr left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
I have made a few comments and would appreciate it if you could check them out.

;; This makes it impossible to insert at the beginning of the file
(cond
((string= "" filename)
(lem:message "No file name"))
Copy link
Member

Choose a reason for hiding this comment

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

How about using lem:editor-error?
Errors in command execution are handled in the command loop and messages are displayed when using lem:editor-error.

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, fixed.

(lem:line-start (lem:current-point))
(unless (lem:line-offset (lem:current-point) 1)
(lem:line-end (lem:current-point))
(lem:newline))
Copy link
Member

Choose a reason for hiding this comment

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

newline is a command, and other operators (lem:move-point, lem:line-start, lem:line-end, etc...) and the layers are different and upscale.
(lem:insert-character point #\newline) is recommended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it.
Does lem has the notion of different line-endings per buffer yet? I didn't check it and expected it.

E.g. if I edit a file on Linux which has Windows line endings, I would like the editor to insert Windows line endings. and work with those properly. That's why I used NEWLINE, since I thought it might do something like this.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the character #\newline abstract line breaks?
For example, CRLF in windows is represented by (#\return #\linefeed), but LF in linux is #\linefeed.

However, I have not grasped whether they behave as the example you have given, so this may be a point for improvement in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, #\newline abstracts line breaks, but for the current OS, meaning that Lem on Windows will insert Windows newlines and Lem on Linux will insert Linux newlines.

My concern was about files, which have Windows newlines, but Lem is executed on Linux and the file is worked on on Linux. Now, newly added newline characters will be those of Linux and the file has mixed newline characters.

Emacs discovers the line endings of the file when it loads the file and configures the file encoding in such a way, that newline characters stay the same. But maybe this should be done by INSERT-CHARACTER. Or some other facility which takes the file encoding into account.

But maybe I am also mistaken and all of this is already handled somewhere in the lisp abstraction. I didn't try it and didn't look into it yet.

@mychris mychris force-pushed the patch/vi-mode-ex-read branch from df8f8b1 to 61e504c Compare January 3, 2024 08:13
@cxxxr cxxxr merged commit a626165 into lem-project:main Jan 3, 2024
1 check passed
@cxxxr
Copy link
Member

cxxxr commented Jan 3, 2024

I merged it, thanks!

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.

2 participants