bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Drew Adams]

`dired-mark-if' marks, unmarks, or flags Dired lines that satisfy its
PREDICATE argument.

But it should do nothing if a given line is already so marked, unmarked,
or flagged.  More importantly, it should not count that line as having
been marked, unmarked, or flagged.  The message that echoes the count of
changes should not take such non-changes into account.

(Also, trivially, the message should not end in a period (.).)

Yes, this is a change in behavior (the message) observed by users.  But
it is TRT, IMO.

Here is a fixed version of the macro.  The diff is trivial.

(defmacro dired-mark-if (predicate msg)
  "Mark files for PREDICATE, according to `dired-marker-char'.
PREDICATE is evaluated on each line, with point at beginning of line.
MSG is a noun phrase for the type of files being marked.
It should end with a noun that can be pluralized by adding `s'.
Return value is the number of files marked, or nil if none were marked."
  `(let ((inhibit-read-only  t)
         count)
    (save-excursion
      (setq count  0)
      (when ,msg (message "%s %ss%s..."
                          (cond ((eq dired-marker-char ?\040)            "Unmarking")
                                ((eq dired-del-marker dired-marker-char) "Flagging")
                                (t                                       "Marking"))
                          ,msg
                          (if (eq dired-del-marker dired-marker-char) " for deletion" "")))
      (goto-char (point-min))
      (while (not (eobp))
        (when ,predicate
          (unless (looking-at (char-to-string dired-marker-char))
            (delete-char 1) (insert dired-marker-char) (setq count  (1+ count))))
        (forward-line 1))
      (when ,msg (message "%s %s%s %s%s" count ,msg
                          (dired-plural-s count)
                          (if (eq dired-marker-char ?\040) "un" "")
                          (if (eq dired-marker-char dired-del-marker) "flagged" "marked"))))
    (and (> count 0)  count)))


In GNU Emacs 24.5.1 (i686-pc-mingw32)
 of 2015-04-11 on LEG570
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/usr --host=i686-pc-mingw32'

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

> `dired-mark-if' marks, unmarks, or flags Dired lines that satisfy its
> PREDICATE argument.
>
> But it should do nothing if a given line is already so marked, unmarked,
> or flagged.  More importantly, it should not count that line as having
> been marked, unmarked, or flagged.  The message that echoes the count of
> changes should not take such non-changes into account.

That sounds like a sensible change, and a cursory look at the code seems
to say that nothing uses the return value, anyway.  (So it shouldn't
break anything.)  (But my look was very cursory; this should be
confirmed by looking at the code closer.)

> Here is a fixed version of the macro.  The diff is trivial.

No diff was included, which, as usual, is a shame, because the macro has
changed slightly in the meantime.

>                           (cond ((eq dired-marker-char ?\040)            "Unmarking")

?\040 isn't the recommended way of saying "space".

>                                 ((eq dired-del-marker dired-marker-char) "Flagging")
>                                 (t                                       "Marking"))

Your lines are also too wide; they should be less than 80 characters
wide.

Could you re-spin your change and submit a patch?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Drew Adams]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

> > Here is a fixed version of the macro.  The diff is trivial.
> 
> No diff was included, which, as usual, is a shame, because the macro has
> changed slightly in the meantime.

That makes no difference. ;-)  And the diff is still
trivial. ("The macro has changed slightly" == ?\040
became ?\s.  Nothing more)

> Your lines are also too wide; they should be less than 80 characters
> wide.

Oh, lines 92 chars are too wide for dired.el?
Then why does it have lots of lines wider than
80 chars, including lines up to 103 chars wide?

> Could you re-spin your change and submit a patch?

diff -u dired.el dired-2019-06-25a-patched.el
--- dired.el	2019-06-25 07:57:35.886354900 -0700
+++ dired-2019-06-25a-patched.el	2019-06-25 08:09:23.058466400 -0700
@@ -538,7 +538,7 @@
 ;;; Macros must be defined before they are used, for the byte compiler.
 
 (defmacro dired-mark-if (predicate msg)
-  "Mark all files for which PREDICATE evals to non-nil.
+  "Mark files for PREDICATE, according to `dired-marker-char'.
 PREDICATE is evaluated on each line, with point at beginning of line.
 MSG is a noun phrase for the type of files being marked.
 It should end with a noun that can be pluralized by adding `s'.
@@ -558,13 +558,11 @@
 		   "")))
       (goto-char (point-min))
       (while (not (eobp))
-        (if ,predicate
-            (progn
-              (delete-char 1)
-              (insert dired-marker-char)
-              (setq count (1+ count))))
+        (when ,predicate
+          (unless (looking-at-p (char-to-string dired-marker-char))
+            (delete-char 1) (insert dired-marker-char) (setq count (1+ count))))
         (forward-line 1))
-      (if ,msg (message "%s %s%s %s%s."
+      (when ,msg (message "%s %s%s %s%s"
                         count
                         ,msg
                         (dired-plural-s count)

----

Actually, the _attached_ definition is much better than
this - it's what I use in `dired+.el'.  The value returned
and the message indicate both the number matched and the
number changed.  And it has optional arg PLURAL, for
irregular plurals (e.g. "directories").

But if you want this one you'll have to do your own line
shortening, `diff', and adjustment of any callers you
might to want to pass an irregular PLURAL form.

[-- Attachment #2: throw-dired-mark-if.el --]
[-- Type: application/octet-stream, Size: 2330 bytes --]

;; 1. Value returned and message indicate both the number matched and the number changed.
;; 2. Added optional arg PLURAL, for irregular plurals (e.g. "directories").
;;
(defmacro dired-mark-if (predicate singular &optional plural)
  "Mark files for PREDICATE, according to `dired-marker-char'.
PREDICATE is evaluated on each line, with point at beginning of line.
SINGULAR is a singular noun phrase for the type of files being marked.
Optional arg PLURAL is a plural noun phrase for the type of files
 being marked.
If PLURAL is nil then SINGULAR should end with a noun that can be
pluralized by adding `s'.

Return nil if no files matched PREDICATE.
Otherwise return a cons (CHANGED . MATCHED), where:
 CHANGED is the number of markings that were changed by the operation.
 MATCHED is the number of files that matched PREDICATE."
  `(let ((inhibit-read-only  t)
         changed matched)
     (save-excursion
       (setq matched  0
             changed  0)
       (when ,singular (message "%s %s%s..."
                                (cond ((eq dired-marker-char ?\040)            "Unmarking")
                                      ((eq dired-del-marker dired-marker-char) "Flagging")
                                      (t                                       "Marking"))
                                (or ,plural  (concat ,singular "s"))
                                (if (eq dired-del-marker dired-marker-char) " for deletion" "")))
       (goto-char (point-min))
       (while (not (eobp))
         (when ,predicate
           (setq matched  (1+ matched))
           (unless (diredp-looking-at-p (char-to-string dired-marker-char))
             (delete-char 1) (insert dired-marker-char) (setq changed  (1+ changed))))
         (forward-line 1))
       (when ,singular (message "%s %s%s%s newly %s%s"
                                matched
                                (if (= matched 1) ,singular (or ,plural  (concat ,singular "s")))
                                (if (not (= matched changed)) " matched, " "")
                                (if (not (= matched changed)) changed "")
                                (if (eq dired-marker-char ?\040) "un" "")
                                (if (eq dired-marker-char dired-del-marker) "flagged" "marked"))))
     (and (> matched 0)  (cons changed matched))))

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

>> Could you re-spin your change and submit a patch?
>
> diff -u dired.el dired-2019-06-25a-patched.el
> --- dired.el	2019-06-25 07:57:35.886354900 -0700
> +++ dired-2019-06-25a-patched.el	2019-06-25 08:09:23.058466400 -0700

Thanks; applied to the Emacs trunk.

As this is a user-visible change (but a good one, I think), it might be
controversial.  The change is basically what I put in NEWS:

--
*** The marking commands now report how many files were marked by the
command itself, not how many files are marked in total.
--

Perhaps somebody will want to weigh in here; feel free to revert the
commit if this is felt to be a confusing change.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> --
> *** The marking commands now report how many files were marked by the
> command itself, not how many files are marked in total.
> --

I don't think this exactly describes what the patch changed: it is not
about new vs total marks, but about marks of files that were already
marked before, right?

> Perhaps somebody will want to weigh in here; feel free to revert the
> commit if this is felt to be a confusing change.

IIUC: Maybe we could add to the summary message something like
"(N already were marked)" or so if a command finds existent marks on files
to be marked?  That might be useful information, even independent from
this change, because it might also be a hint for a pilot error.


Michael.

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> --
>> *** The marking commands now report how many files were marked by the
>> command itself, not how many files are marked in total.
>> --
>
> I don't think this exactly describes what the patch changed: it is not
> about new vs total marks, but about marks of files that were already
> marked before, right?

It's this code:

          (unless (looking-at-p (char-to-string dired-marker-char))
            (delete-char 1)
            (insert dired-marker-char)
            (setq count (1+ count))))

So now it says how many lines that are marked this time.  Previously it
gave you a total tally of all marked files.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> > I don't think this exactly describes what the patch changed: it is not
> > about new vs total marks, but about marks of files that were already
> > marked before, right?
>
> It's this code:
>
>           (unless (looking-at-p (char-to-string dired-marker-char))
>             (delete-char 1)
>             (insert dired-marker-char)
>             (setq count (1+ count))))
>
> So now it says how many lines that are marked this time.  Previously it
> gave you a total tally of all marked files.

Yes, I think I understand.  My problem is that "[...] not how many files
are marked in total" sounds like as if marked files that were not even
touched by the command (that are not matched by the predicate) were
previously included.  You mean it relative to the files matched, right?

Oh, btw, this patch line

  (looking-at-p (char-to-string dired-marker-char))

is not good if dired-marker-char is a regexp special like . or ?, I
guess (regexp-quote missing)?

Michael

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Yes, I think I understand.  My problem is that "[...] not how many files
> are marked in total" sounds like as if marked files that were not even
> touched by the command (that are not matched by the predicate) were
> previously included.  You mean it relative to the files matched, right?

Well... this is the patch:

-        (if ,predicate
-            (progn
-              (delete-char 1)
-              (insert dired-marker-char)
-              (setq count (1+ count))))
+        (when ,predicate
+          (unless (looking-at-p (char-to-string dired-marker-char))
+            (delete-char 1)
+            (insert dired-marker-char)
+            (setq count (1+ count))))

So if the predicate matches, if there was a mark, it would delete the
mark, and then add it back, and then count it.

But you're right, files where the predicate doesn't say non-nil are not
counted, so it doesn't report the total number of marked files...

> Oh, btw, this patch line
>
>   (looking-at-p (char-to-string dired-marker-char))
>
> is not good if dired-marker-char is a regexp special like . or ?, I
> guess (regexp-quote missing)?

Indeed.  I'll fix it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Drew Adams]

> Oh, btw, this patch line
> 
>   (looking-at-p (char-to-string dired-marker-char))
> 
> is not good if dired-marker-char is a regexp special like . or ?, I
> guess (regexp-quote missing)?

It's a good point; it would be clearer to wrap the
arg to `looking-at-p' with `regexp-quote'.

It works as coded, however, because Emacs regexp
matching treats a special char such as `*' as
non-special in such a context.

I'm not saying it's good to rely on that (documented)
behavior - it's better to use `regexp-quote'.

See (elisp)`Syntax of Regexps':

  *Please note:* For historical compatibility, special characters are
  treated as ordinary ones if they are in contexts where their special
  meanings make no sense.  For example, '*foo' treats '*' as ordinary
  since there is no preceding expression on which the '*' can act.  It is
  poor practice to depend on this behavior; quote the special character
  anyway, regardless of where it appears.

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Michael Heerdegen]

Drew Adams <drew.adams@oracle.com> writes:

> It works as coded, however, because Emacs regexp matching treats a
> special char such as `*' as non-special in such a context.

Oh, indeed.  But not for `.' I think.

Michael.

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Drew Adams]

> > It works as coded, however, because Emacs regexp matching treats a
> > special char such as `*' as non-special in such a context.
> 
> Oh, indeed.  But not for `.' I think.

Right again.  (And yes, one can use `.' as a marker char.)

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Andreas Schwab]

On Jun 25 2019, Drew Adams <drew.adams@oracle.com> wrote:

>> Oh, btw, this patch line
>> 
>>   (looking-at-p (char-to-string dired-marker-char))
>> 
>> is not good if dired-marker-char is a regexp special like . or ?, I
>> guess (regexp-quote missing)?
>
> It's a good point; it would be clearer to wrap the
> arg to `looking-at-p' with `regexp-quote'.

I think it's better to say (eq (char-after) dired-marker-char).

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Drew Adams]

> I think it's better to say (eq (char-after) dired-marker-char).

Yes, good.  Not a big deal either way, but
that's simpler and clearer.

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Drew Adams]

> > *** The marking commands now report how many files were marked by the
> > command itself, not how many files are marked in total.
> > --
> 
> I don't think this exactly describes what the patch changed: it is not
> about new vs total marks, but about marks of files that were already
> marked before, right?

It's about making the reported count refer to
the number of marks that were changed or added.

The old way counted all of the matches.
The new way counts only matches that are not
already marked the same way.

> > Perhaps somebody will want to weigh in here; feel free to revert the
> > commit if this is felt to be a confusing change.
> 
> IIUC: Maybe we could add to the summary message something like
> "(N already were marked)" or so if a command finds existent marks on files
> to be marked?  That might be useful information, even independent from
> this change, because it might also be a hint for a pilot error.

That's what the "better" version does that I sent
as an _attachment_.

If some were already marked as directed it says:

"42 foobars matched, 35 newly marked"    or
"42 foobars matched, 35 newly unmarked"  or
"42 foobars matched, 35 newly flagged"   or
"42 foobars matched, 35 newly unflagged"

(If all that were matched were already marked
as directed then instead of "35" it says "0".)

Otherwise (no matches were already marked as
directed) it says:

"42 foobars newly marked"    or
"42 foobars newly unmarked"  or
"42 foobars newly flagged"   or
"42 foobars newly unflagged"

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Michael Heerdegen]

Drew Adams <drew.adams@oracle.com> writes:

> That's what the "better" version does that I sent
> as an _attachment_.

Yes, something like that.

Michael.

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Lars Ingebrigtsen]

And why is dired-mark-if a macro in the first place?  It's pretty long
to be a macro; it could be rewritten as a function that take a
predicate...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#22457: 24.5; [PATCH] `dired-mark-if' should not count non-changes

[Drew Adams]

> And why is dired-mark-if a macro in the first place?  It's pretty long
> to be a macro; it could be rewritten as a function that take a
> predicate...

(Length of a macro's code makes no difference, if
the code using it is compiled.)

I would say leave this sleeping dog lie...

---

The PREDICATE arg is really code (a sexp), not
a predicate (function).

See also the use of `dired-mark-if' in command
`dired-mark-sexp' (in `dired-x.el').  See the
doc string of that command.

There too the PREDICATE arg is code (a sexp),
not a predicate.  And there you can use a set
of predefined variables in the "PREDICATE"
sexp that gets evaluated and then passed to
`dired-mark-if'.

See also the uses of `dired-mark-if' in
`dired-aux.el', where an anonymous function
is constructed using the macro.

This is the way things have been since about
Day One of Emacs.

Could things have been coded differently for
this?  Likely.  Should they be?  Doubtful.