-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Extend spacemacs/copy-file command with extra actions #14754
Extend spacemacs/copy-file command with extra actions #14754
Conversation
ca6a3f5
to
2579a08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, replace error by user-error. These errors should not start the debugger.
(not (y-or-n-p (format "File %s exists. Overwrite?: " new-file-name)))) | ||
(error "Canceled")) | ||
(pcase | ||
(read-answer "New file action: " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general avoid string comparison. This is inefficient.
You can just match the character code.
If you really want, match symbol, e.g. 'only-write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this info, handy to be aware of. But in this case the code only runs once only when this command is called, so I guess the efficiency is not so important in this case.
So otherwise you would suggest to compare only the first (or other relevant) characters, or alternatively compare by converting to symbol with intern
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I did not know about user-error
. Indeed I used a catch-throw before, but I thought error
looks nicer (and then accept the "wrong" behavior). Very useful info!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So otherwise you would suggest to compare only the first (or other relevant) characters, or alternatively compare by converting to symbol with
intern
?
See https://www.gnu.org/software/emacs/manual/html_node/elisp/Multiple-Queries.html
- wrap this in a let form to set read-answer-short to t.
- Change the long answer to a single character string. the long answer is not shown but is returned. Since it has length 1, it's fast to compare.
- Since read-answer-short is t, user only need to type one key to select a choice, instead of being required to type a sentence/phrase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I was aware of this, but then I would probably have wrapped it in read-answer-short 'auto
(and indeed make the long answers slightly shorter, but still a little more informative)... somehow I assumed the 'auto was the default behavior...
(when (directory-name-p new-file-name) | ||
(if buffer-file-name | ||
(setq new-file-name (concat new-file-name (buffer-name))) | ||
(error "Copying of non-visiting file buffers requires to specify new file name."))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"non-visiting file" is not easy to understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree somewhat, although I think it is hard to misunderstand this (of course it is "non-visiting file buffer").
I would be happy if you could suggest a better term (English is not my native language).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "non-visiting-file buffers"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels like romance language, not english
fc1dbcb
to
76948b8
Compare
key-bindings for the multiple choice can be improved. If this command is invoked via SPC f c, we can put the most possible choice under the key c. I suggest
|
I agree with you @lebensterben that generally, it is nice to put the default bindings on the quickest way to type. However, in this case after typing On the other hand, I do agree that my suggestions (r, n and o) might not be the best choices. But I would suggest we choose either the most sensible mnemonic or most consistent with other command's keybindings (which should generally most sensible mnemonic also). So another main candidate for me was: |
okay. c o w looks a good choice |
76948b8
to
eab5a05
Compare
Updated! The reason why I chose "replace" over "current-buffer" is that the former "more explicitly" implies that the old-file buffer is getting killed (or well... replaced). But I guess this |
This command prompts for a new filename then prompts for | ||
selecting an action to perform on the new file. | ||
|
||
If prefixed with the universal ARG \\[universal-argument] the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remind me why we need this prefix argument?
(read-file-name "Save file as: " (if arg buffer-file-name default-directory))
Can we just write
(read-file-name "Save file as: " (or buffer-file-name default-directory))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This argument is to include the filename in the prompt for new filename like in the original proposal #8974 (comment) (but then inverted). The or
wouldn't work here, at least not for how I meant it. On the other hand (buffer-name)
would be even better than buffer-file-name.
Use this (defun spacemacs/save-as (filename &optional visit)
"Save current buffer or active region as specified file.
When called interactively, it first prompts for FILENAME, and then asks
whether to VISIT it, and if so, whether to show it in current window or
another window.
FILENAME a non-empty string as the name of the saved file.
VISIT When it's `:current', open FILENAME in current window. When it's
`:other', open FILENAME in another window. When it's nil, only
save to FILENAME but does not visit it. (Default to `:current'
when called from a LISP program.)
When FILENAME already exists, it also asks the user whether to overwrite it."
(interactive (let* ((filename (expand-file-name (read-file-name "Save buffer as: ")))
(choices '("Current window"
"Other window"
"Don't open"))
(actions '(:current :other nil))
(visit (let ((completion-ignore-case t))
(nth (cl-position
(completing-read "Do you want to open the file? "
choices nil t)
choices
:test #'equal)
actions))))
(list filename visit)))
(unless (called-interactively-p 'any)
(cl-assert (and (stringp filename)
(not (string-empty-p filename))
(not (directory-name-p filename)))
t "Expect a non-empty filepath, found: %s")
(setq filename (expand-file-name filename)
visit (or visit :other))
(let ((choices '(:current :other nil)))
(cl-assert (memq visit choices)
t "Found %s, expect one of %s")))
(let ((dir (file-name-directory filename)))
(unless (file-directory-p dir)
(make-directory dir t)))
(if (use-region-p)
(write-region (region-beginning) (region-end) filename nil nil nil t)
(write-region nil nil filename nil nil nil t))
(pcase visit
(:current (find-file filename))
(:other (find-file-other-window filename)))) |
Haha! That's funny... it is a full rewrite. Anyway, that is fine with me. However, this doesn't yet include an option to include the (current) filename in the prompt. (Which was one of the main "improvements" of the original proposal #8974 (comment). Or you don't like that?) Nice addition to include the 'write selected region' option! |
@dalanicolai |
I already found this issue. I just didn't bother to fix it. |
eab5a05
to
303e0ba
Compare
it's possible to select with two keystroke. the initial and <enter>. |
Also note that I've edited the code snippet. It's certainly not final. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to merge this. Just fix the docstring.
Also please update CHANGELOG.develop
(call-interactively 'write-file)) | ||
(defun spacemacs/save-as (filename &optional visit) | ||
"Save current buffer or active region as specified file. | ||
When called interactively, it first prompts for FILENAME, and then asks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the extra space in the beginning of each line of docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
77c224c
to
e6ce398
Compare
I got a little confused about 5d213ce. It caused this PR to consist of 2 commits, and I think it was not meant like that. Anyway, I have reverted to a single commit, implemented the requested changes, and rebased the PR/branch on develop. |
https://github.com/syl20bnr/spacemacs/pull/14754/files |
You mean it is fine to have multiple commits because you will squash them when merging? |
yes. |
This commit implements an alternative for PR syl20bnr#8974
e6ce398
to
7492e1b
Compare
Merged. Thanks for your contributions. |
I think this key binding needs to be updated, since the name of the function was changed: ;; file -----------------------------------------------------------------------
(spacemacs|spacebind
"Files manipulation."
:global
(("f" "Files"
("A" spacemacs/find-file-and-replace-buffer "Set another file for buffer...")
("c" spacemacs/copy-file "Copy file to new file...") |
* checkversion/develop: (55 commits) [doc] use org-mode example block for example Added COPYRIGHT file [ivy] Fix new src block in docs [ivy] counsel search commands use spacemacs--ivy-file-actions [core] Support packing elisp to *.elc and *.el.gz files [version-control] Switch default version control diff tool to git-gutter [python] Remove comments and fix new commands [bot] "built_in_updates" Wed Sep 29 19:18:45 UTC 2021 Add pydoc to python layer for better python doc functionality [python] Make ipython version detection more robust Revise some smaller layers [bot] "documentation_updates" Fri Sep 17 19:24:14 UTC 2021 Fix (activate) company-emoji completion [bot] "documentation_updates" Fri Sep 17 18:59:01 UTC 2021 Add documentation about evilifying 'special-mode buffers' (+fix eww) Fix and update PR syl20bnr#14992: update eaf layer Update keybinding Extend spacemacs/copy-file command with extra actions (syl20bnr#14754) Add simple workaround to ebib kill buffer issue (syl20bnr#15048) [bot] "documentation_updates" Sat Sep 11 22:58:39 UTC 2021 ...
This PR implements an alternative for PR #8974.