bug#24103: 25.1.50; Add white space separator when appending string

bug#24103: 25.1.50; Add white space separator when appending string

[Tino Calancha]

I)
emacs -Q ~
M-: (goto-char (dired-subdir-min)) RET
M-: (dired-goto-next-file) RET
w
;; copied first file name into kill-ring
M-: (dired-next-line 1) RET
C-M-w w
;; This append second file name to the kill-ring.
;; Note that there is no separator between the first/second file name.

II)
emacs -Q ~
M-: (goto-char (dired-subdir-min)) RET
w
;; dir name copied into kill-ring
M-: (goto-char (dired-subdir-max)) RET
w
;; copied '' into kill-ring

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From 80e9eb3fc790148a70dbf1da5d12e22681180bee Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Fri, 29 Jul 2016 18:53:38 +0900
Subject: [PATCH] Add white space separator when appending string

* lisp/dired.el (dired-copy-filename-as-kill):
Add white space before string when command is appending.
Copy just non-empty strings (Bug#24103).
---
  lisp/dired.el | 9 +++++----
  1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lisp/dired.el b/lisp/dired.el
index 5d14291..095ab14 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2467,10 +2467,11 @@ dired-copy-filename-as-kill
  				    'no-dir (prefix-numeric-value arg))))
                            (dired-get-marked-files 'no-dir))
                          " "))))
-    (if (eq last-command 'kill-region)
-	(kill-append string nil)
-      (kill-new string))
-    (message "%s" string)))
+    (unless (string= string "")
+      (if (eq last-command 'kill-region)
+          (kill-append (concat " " string) nil)
+        (kill-new string))
+      (message "%s" string))))


  ;; Keeping Dired buffers in sync with the filesystem and with each other
-- 
2.8.1


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;


GNU Emacs 25.1.50 (x86_64-pc-linux-gnu, GTK+ Version 3.20.6)
  of 2016-07-28
Repository revision: 4a5b6e621c68172bb69d60fe8a76932f7c779f81

bug#24103: 25.1.50; Add white space separator when appending string

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Fri, 29 Jul 2016 18:57:41 +0900 (JST)
> 
> 
> I)
> emacs -Q ~
> M-: (goto-char (dired-subdir-min)) RET
> M-: (dired-goto-next-file) RET
> w
> ;; copied first file name into kill-ring
> M-: (dired-next-line 1) RET
> C-M-w w
> ;; This append second file name to the kill-ring.
> ;; Note that there is no separator between the first/second file name.
> 
> II)
> emacs -Q ~
> M-: (goto-char (dired-subdir-min)) RET
> w
> ;; dir name copied into kill-ring
> M-: (goto-char (dired-subdir-max)) RET
> w
> ;; copied '' into kill-ring
> 
> ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
> >From 80e9eb3fc790148a70dbf1da5d12e22681180bee Mon Sep 17 00:00:00 2001
> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Fri, 29 Jul 2016 18:53:38 +0900
> Subject: [PATCH] Add white space separator when appending string
> 
> * lisp/dired.el (dired-copy-filename-as-kill):
> Add white space before string when command is appending.
> Copy just non-empty strings (Bug#24103).

Isn't this second-guessing what the user means?  What if the kill ring
has "/foo/bar/" and the user wants to append a file name to that, thus
creating a valid absolute file name?

Thanks.

bug#24103: 25.1.50; Add white space separator when appending string

[Tino Calancha]

On Fri, 29 Jul 2016, Tino Calancha wrote:
>> emacs -Q ~
Please, require dired-aux to do my example, as follows:
emacs -Q ~ --eval="(require 'dired-aux)"
;; for 'dired-subdir-min'

On Fri, 29 Jul 2016, Eli Zaretskii wrote:
> Isn't this second-guessing what the user means?  What if the kill ring
> has "/foo/bar/" and the user wants to append a file name to that, thus
> creating a valid absolute file name?
Maybe, but not so standard way; we can skip that part of the fix, though.
The command provide a nice way to do that as mentioned in the doc string:
C-u 0 w

bug#24103: 25.1.50; Add white space separator when appending string

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Fri, 29 Jul 2016 19:27:30 +0900 (JST)
> cc: Tino Calancha <tino.calancha@gmail.com>, 24103@debbugs.gnu.org
> 
> On Fri, 29 Jul 2016, Eli Zaretskii wrote:
> > Isn't this second-guessing what the user means?  What if the kill ring
> > has "/foo/bar/" and the user wants to append a file name to that, thus
> > creating a valid absolute file name?
> Maybe, but not so standard way; we can skip that part of the fix, though.

Sorry, I don't understand: which part of the fix you suggest to skip,
and what will be left then?

> The command provide a nice way to do that as mentioned in the doc string:
> C-u 0 w

No, that produces the absolute names of files in the directory shown
by Dired, whereas what I meant is constructing a name of a file in
another directory by concatenating a directory killed in another kill
command and the file name killed by 'w'.

bug#24103: 25.1.50; Add white space separator when appending string

[Tino Calancha]

On Fri, 29 Jul 2016, Eli Zaretskii wrote:

>> The command provide a nice way to do that as mentioned in the doc 
string:
>> C-u 0 w
>
> No, that produces the absolute names of files in the directory shown
> by Dired, whereas what I meant is constructing a name of a file in
> another directory by concatenating a directory killed in another kill
> command and the file name killed by 'w'.
Agreed.  It might be useful.

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Date: Fri, 29 Jul 2016 19:27:30 +0900 (JST)
>> cc: Tino Calancha <tino.calancha@gmail.com>, 24103@debbugs.gnu.org

> Sorry, I don't understand: which part of the fix you suggest to skip,
> and what will be left then?
So, the patch now fix the II) in the report: it prevent to change the kill
ring when the string is empty.
The command name and the doc string suggest the usage is about copying
file names.
I would prefer if this command have effect just when point is:

1) on a line showing a subdir name
for instance:
(dired-subdir-min)

2) on a line showing a file from the dired listing
on a line displaying a file or subddir name
for instance,
(progn (goto-char (dired-subdir-min)) (dired-goto-next-file) (point))

Currently the command is enabled whatever is the point in the Dired 
buffer.  As a consequence, the command put a "" in the kill ring if
the command is called with the point in one position other than 1) 2)
for instance:
3)
(progn (goto-char (dired-subdir-min)) (forward-line 1) (point))

I would propose in order of preference:

A) Do not change the kill ring when the command is called with point
    in a position like 3) (See patch below).
B) Update the doc string alerting that the command is responsive even
    if there is no filename/subdir accesible: the kill ring will get "" in
    that cases.

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
From c7c1312e4acf942742e387a474d368266b48dc99 Mon Sep 17 00:00:00 2001
From: Tino Calancha <tino.calancha@gmail.com>
Date: Fri, 29 Jul 2016 23:06:16 +0900
Subject: [PATCH] Copy just non-empty strings to kill-ring

* lisp/dired.el (dired-copy-filename-as-kill):
Do not change the kill ring when the string is empty. (Bug#24103).
---
  lisp/dired.el | 9 +++++----
  1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lisp/dired.el b/lisp/dired.el
index 5d14291..7ead087 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2467,10 +2467,11 @@ dired-copy-filename-as-kill
  				    'no-dir (prefix-numeric-value arg))))
                            (dired-get-marked-files 'no-dir))
                          " "))))
-    (if (eq last-command 'kill-region)
-	(kill-append string nil)
-      (kill-new string))
-    (message "%s" string)))
+    (unless (string= string "")
+      (if (eq last-command 'kill-region)
+          (kill-append string nil)
+        (kill-new string))
+      (message "%s" string))))


  ;; Keeping Dired buffers in sync with the filesystem and with each other
-- 
2.8.1


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

GNU Emacs 25.1.50 (x86_64-pc-linux-gnu, GTK+ Version 3.20.6)
  of 2016-07-28
Repository revision: 4a5b6e621c68172bb69d60fe8a76932f7c779f81

bug#24103: 25.1.50; Add white space separator when appending string

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Fri, 29 Jul 2016 23:29:50 +0900 (JST)
> cc: Tino Calancha <tino.calancha@gmail.com>, 24103@debbugs.gnu.org
> 
> So, the patch now fix the II) in the report: it prevent to change the kill
> ring when the string is empty.

That should be fine, thanks.

bug#24103: (no subject)

[Tino Calancha]

Fixed in master branch

bug#24103: (no subject)

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Sat, 30 Jul 2016 17:29:42 +0900 (JST)
> 
> Fixed in master branch

Thanks.  Two minor comments:

 . When you close a bug, please reply to the last message related to
   that bug, so that the Subject line is meaningful (as opposed to the
   above).

 . Please try making the summary line of the commit log message be as
   specific as possible, within the limitations of 64 characters.  The
   one you used here:

      Copy just non-empty strings to kill-ring

   is too general: it fails to mention Dired.  I would suggest
   something like this:

      Avoid copying empty strings with Dired's 'w' command

Thanks.

bug#24103: (no subject)

[Tino Calancha]

On Sat, 30 Jul 2016, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Date: Sat, 30 Jul 2016 17:29:42 +0900 (JST)
>>
>> Fixed in master branch
>
> Thanks.  Two minor comments:
>
> . When you close a bug, please reply to the last message related to
>   that bug, so that the Subject line is meaningful (as opposed to the
>   above).
>
> . Please try making the summary line of the commit log message be as
>   specific as possible, within the limitations of 64 characters.  The
>   one you used here:
>
>      Copy just non-empty strings to kill-ring
>
>   is too general: it fails to mention Dired.  I would suggest
>   something like this:
>
>      Avoid copying empty strings with Dired's 'w' command

Thanks for the tips Eli.
I will follow then in future commits.
Tino