Extend spacemacs/copy-file command with extra actions #14754

dalanicolai · 2021-05-08T21:39:10Z

This PR implements an alternative for PR #8974.

lebensterben

Also, replace error by user-error. These errors should not start the debugger.

lebensterben · 2021-05-09T04:12:16Z

layers/+spacemacs/spacemacs-defaults/funcs.el

+               (not (y-or-n-p (format "File %s exists. Overwrite?: " new-file-name))))
+      (error "Canceled"))
+    (pcase
+        (read-answer "New file action: "


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.

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?

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!

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

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

lebensterben · 2021-05-09T04:14:45Z

layers/+spacemacs/spacemacs-defaults/funcs.el

+    (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.")))


"non-visiting file" is not easy to understand

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).

Maybe "non-visiting-file buffers"?

this feels like romance language, not english

layers/+spacemacs/spacemacs-defaults/funcs.el

lebensterben · 2021-05-09T05:59:09Z

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.
So the user just quickly type SPC f c c to perform the default behavior.

I suggest

c Copy and open in new buffer
C Copy and open on current buffer
d Don't open the new file after copying
q Quit

dalanicolai · 2021-05-09T07:33:07Z

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 SPC f c the user first has to enter a new filename and a RET. So it is not very important here that the default action is under c.
Also, I always prefer downcase letters, so I would vote against using C.

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:
c for open in current buffer
o for open in other buffer
w for just write (or 's' for just save)

lebensterben · 2021-05-09T08:33:38Z

okay. c o w looks a good choice

dalanicolai · 2021-05-09T11:22:02Z

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 cow will also produce our milk...

lebensterben · 2021-05-26T01:22:27Z

layers/+spacemacs/spacemacs-defaults/funcs.el

+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


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))

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.

lebensterben · 2021-05-26T06:30:05Z

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))))

dalanicolai · 2021-05-26T07:02:56Z

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?)
Also it is not possible to select a "choice" with a single keystroke, but that is a matter of preference of course (I like this completing-read solution too).

Nice addition to include the 'write selected region' option!

lebensterben · 2021-05-26T07:28:23Z

@dalanicolai
That's a bug of helm.
If you want, (read-file-name "Save buffer as: " nil nil nil buffer-file-name)

lebensterben · 2021-05-26T07:30:22Z

I already found this issue. I just didn't bother to fix it.

lebensterben · 2021-05-26T07:50:18Z

it's possible to select with two keystroke. the initial and <enter>.

lebensterben · 2021-05-26T07:51:46Z

Also note that I've edited the code snippet. It's certainly not final.

lebensterben

I'm going to merge this. Just fix the docstring.
Also please update CHANGELOG.develop

lebensterben · 2021-08-24T01:59:18Z

layers/+spacemacs/spacemacs-defaults/funcs.el

-  (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


Please remove the extra space in the beginning of each line of docstring

dalanicolai · 2021-08-28T03:28:32Z

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.

lebensterben · 2021-08-28T03:37:21Z

https://github.com/syl20bnr/spacemacs/pull/14754/files
This is what the changes will be like when it's squashed and merged.

dalanicolai · 2021-08-28T05:33:41Z

You mean it is fine to have multiple commits because you will squash them when merging?

lebensterben · 2021-08-28T05:43:57Z

yes.
squash and merge is the default actually

This commit implements an alternative for PR syl20bnr#8974

lebensterben · 2021-09-15T18:58:22Z

Merged. Thanks for your contributions.

bcc32 · 2021-09-16T13:42:58Z

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...")

fixes #14754 (comment)

* 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 ...

fixes syl20bnr#14754 (comment)

dalanicolai force-pushed the extend-spacemacs/copy-file-command branch 2 times, most recently from ca6a3f5 to 2579a08 Compare May 8, 2021 21:50

lebensterben requested changes May 9, 2021

View reviewed changes

dalanicolai force-pushed the extend-spacemacs/copy-file-command branch 2 times, most recently from fc1dbcb to 76948b8 Compare May 9, 2021 05:34

dalanicolai force-pushed the extend-spacemacs/copy-file-command branch from 76948b8 to eab5a05 Compare May 9, 2021 11:21

dalanicolai requested a review from lebensterben May 25, 2021 17:22

lebensterben reviewed May 26, 2021

View reviewed changes

dalanicolai force-pushed the extend-spacemacs/copy-file-command branch from eab5a05 to 303e0ba Compare May 26, 2021 07:39

lebensterben requested changes Aug 24, 2021

View reviewed changes

dalanicolai force-pushed the extend-spacemacs/copy-file-command branch 3 times, most recently from 77c224c to e6ce398 Compare August 28, 2021 03:26

Extend spacemacs/copy-file command with extra actions

7492e1b

This commit implements an alternative for PR syl20bnr#8974

dalanicolai force-pushed the extend-spacemacs/copy-file-command branch from e6ce398 to 7492e1b Compare September 15, 2021 11:31

lebensterben approved these changes Sep 15, 2021

View reviewed changes

lebensterben merged commit cabe650 into syl20bnr:develop Sep 15, 2021

lebensterben added a commit that referenced this pull request Sep 16, 2021

Update keybinding

08fa117

fixes #14754 (comment)

2ynn pushed a commit to 2ynn/spacemacs that referenced this pull request Jun 22, 2022

Extend spacemacs/copy-file command with extra actions (syl20bnr#14754)

e2752df

2ynn pushed a commit to 2ynn/spacemacs that referenced this pull request Jun 22, 2022

Update keybinding

7f0fca4

fixes syl20bnr#14754 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend spacemacs/copy-file command with extra actions #14754

Extend spacemacs/copy-file command with extra actions #14754

dalanicolai commented May 8, 2021

lebensterben left a comment

lebensterben May 9, 2021

dalanicolai May 9, 2021 •

edited

dalanicolai May 9, 2021

lebensterben May 9, 2021

dalanicolai May 26, 2021 •

edited

lebensterben May 9, 2021

dalanicolai May 9, 2021

dalanicolai May 9, 2021

lebensterben May 26, 2021

lebensterben commented May 9, 2021

dalanicolai commented May 9, 2021 •

edited

lebensterben commented May 9, 2021

dalanicolai commented May 9, 2021

lebensterben May 26, 2021

dalanicolai May 26, 2021 •

edited

lebensterben commented May 26, 2021 •

edited

dalanicolai commented May 26, 2021 •

edited

lebensterben commented May 26, 2021

lebensterben commented May 26, 2021

lebensterben commented May 26, 2021 •

edited

lebensterben commented May 26, 2021

lebensterben left a comment

lebensterben Aug 24, 2021

dalanicolai Sep 15, 2021

dalanicolai commented Aug 28, 2021 •

edited

lebensterben commented Aug 28, 2021

dalanicolai commented Aug 28, 2021

lebensterben commented Aug 28, 2021 •

edited

lebensterben commented Sep 15, 2021

bcc32 commented Sep 16, 2021

Extend spacemacs/copy-file command with extra actions #14754

Extend spacemacs/copy-file command with extra actions #14754

Conversation

dalanicolai commented May 8, 2021

lebensterben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalanicolai May 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalanicolai May 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebensterben commented May 9, 2021

dalanicolai commented May 9, 2021 • edited

lebensterben commented May 9, 2021

dalanicolai commented May 9, 2021

Choose a reason for hiding this comment

dalanicolai May 26, 2021 • edited

Choose a reason for hiding this comment

lebensterben commented May 26, 2021 • edited

dalanicolai commented May 26, 2021 • edited

lebensterben commented May 26, 2021

lebensterben commented May 26, 2021

lebensterben commented May 26, 2021 • edited

lebensterben commented May 26, 2021

lebensterben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalanicolai commented Aug 28, 2021 • edited

lebensterben commented Aug 28, 2021

dalanicolai commented Aug 28, 2021

lebensterben commented Aug 28, 2021 • edited

lebensterben commented Sep 15, 2021

bcc32 commented Sep 16, 2021

dalanicolai May 9, 2021 •

edited

dalanicolai May 26, 2021 •

edited

dalanicolai commented May 9, 2021 •

edited

dalanicolai May 26, 2021 •

edited

lebensterben commented May 26, 2021 •

edited

dalanicolai commented May 26, 2021 •

edited

lebensterben commented May 26, 2021 •

edited

dalanicolai commented Aug 28, 2021 •

edited

lebensterben commented Aug 28, 2021 •

edited