bug#14925: 24.3.50; `image-dired.el' code (minor)

bug#14925: 24.3.50; `image-dired.el' code (minor)

[Drew Adams]

Generally, it would be better if `image-dired.el' followed the
conventions used in dired.el, dired-aux.el, and dired-x.el wrt (a)
naming and (b) parameter ARG (prefix argument).  That helps users by
providing relatively consistent command names and UI behavior.

Also a minor coding remark (#1).

1. `image-dired-dired-file-marked-p' uses regexp "^ .*$", which I think
is the same as "^ " (which is simpler).  I also wonder why it does not
just use `dired-re-mark'.

2. Commands that act on the marked files should perhaps follow the
naming convention used in dired.el, dired-aux.el, and dired-x.el:
`*-do-*'.  Thus perhaps rename `image-dired-dired-comment-files',
`image-dired-tag-files', `image-dired-delete-tag',
`image-dired-display-thumbs', `image-dired-copy-with-exif-file-name',
`image-dired-dired-edit-comment-and-tags', and
`image-dired-create-thumbs'.

The equivalent Dired command that acts on the current file only, when
available, is named similarly, but without the `*-do-*' part.

3. Those commands do not treat parameter ARG the same way as do the
Dired `*-do-*' commands.  For the image-dired commands, ARG is
essentially boolean (and should thus have a name that reflects that).
Wouldn't it be better for it to be the `prefix-numeric-value' and let
you act on the next ARG files?  That would give users more flexibility
and provide a more consistent UI.  Is there a downside I am missing?


In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2013-07-14 on ODIEONE
Bzr revision: 113423 lekktu@gmail.com-20130715004922-i67tg2ois14h3fpm
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/Devel/emacs/binary --enable-checking=yes,glyphs
 CFLAGS='-O0 -g3' CPPFLAGS='-Ic:/Devel/emacs/include'
 LDFLAGS='-Lc:/Devel/emacs/lib''

bug#14925: 24.3.50; `image-dired.el' code (minor)

[Drew Adams]

Forgot to mention: "selected" should be "marked" in the doc string
of `image-dired-delete-tag'.

bug#14925: 24.3.50; `image-dired.el' code (minor)

[Stefan Monnier]

> 1. `image-dired-dired-file-marked-p' uses regexp "^ .*$", which I think
> is the same as "^ " (which is simpler).

It's similar, except for (match-end 0) and the position of point after
re-search-forward.  I haven't looked at the code to see if the
difference matters.

> I also wonder why it does not just use `dired-re-mark'.

No idea.

> 2. Commands that act on the marked files should perhaps follow the
> naming convention used in dired.el, dired-aux.el, and dired-x.el:
> `*-do-*'.  Thus perhaps rename `image-dired-dired-comment-files',
> `image-dired-tag-files', `image-dired-delete-tag',
> `image-dired-display-thumbs', `image-dired-copy-with-exif-file-name',
> `image-dired-dired-edit-comment-and-tags', and
> `image-dired-create-thumbs'.

> The equivalent Dired command that acts on the current file only, when
> available, is named similarly, but without the `*-do-*' part.

> 3. Those commands do not treat parameter ARG the same way as do the
> Dired `*-do-*' commands.  For the image-dired commands, ARG is
> essentially boolean (and should thus have a name that reflects that).
> Wouldn't it be better for it to be the `prefix-numeric-value' and let
> you act on the next ARG files?  That would give users more flexibility
> and provide a more consistent UI.  Is there a downside I am missing?

2 and 3 sound like good ideas.


        Stefan

bug#14925: 24.3.50; `image-dired.el' code (minor)

[Drew Adams]

> > 1. `image-dired-dired-file-marked-p' uses regexp "^ .*$", which I think
> > is the same as "^ " (which is simpler).
> 
> It's similar, except for (match-end 0) and the position of point after
> re-search-forward.  I haven't looked at the code to see if the
> difference matters.

It does not; the match data and the position of point are not used.
(The `-p' suffix provides a clue.)  There is currently only one occurrence
of `image-dired-dired-file-marked-p':

(if (image-dired-dired-file-marked-p)
    (dired-unmark 1)
  (dired-mark 1))

bug#14925: 24.3.50; `image-dired.el' code (minor)

[Drew Adams]

> > > 1. `image-dired-dired-file-marked-p' uses regexp "^ .*$", which I think
> > > is the same as "^ " (which is simpler).
> >
> > It's similar, except for (match-end 0) and the position of point after
> > re-search-forward.  I haven't looked at the code to see if the
> > difference matters.
> 
> It does not; the match data and the position of point are not used.
> (The `-p' suffix provides a clue.)  There is currently only one occurrence
> of `image-dired-dired-file-marked-p':
> 
> (if (image-dired-dired-file-marked-p)
>     (dired-unmark 1)
>   (dired-mark 1))

It also uses `save-excursion' and `looking-at', not `re-search-forward'.

However, as it is in principle just a predicate it should probably also
use `looking-at-p', not `looking-at'.

bug#14925: 24.3.50; `image-dired.el' code (minor)

[Lars Ingebrigtsen]

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

> 1. `image-dired-dired-file-marked-p' uses regexp "^ .*$", which I think
> is the same as "^ " (which is simpler).  I also wonder why it does not
> just use `dired-re-mark'.

I've now changed this function to use `dired-re-mark' instead in Emacs
28.

As for the other two points -- changing the names of all these dired
commands -- I don't think the gain we get from more regular function
names in this area outweighs the pain we'd inflict on users that may be
using the old names.  So I'm closing this bug report.

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

bug#14925: [External] : Re: bug#14925: 24.3.50; `image-dired.el' code (minor)

[Drew Adams]

> As for the other two points -- changing the names of all these dired
> commands -- I don't think the gain we get from more regular function
> names in this area outweighs the pain we'd inflict on users that may be
> using the old names.  So I'm closing this bug report.

Renaming and deprecation of the old names is the
right approach, IMO.  There's no reason that
something that's deprecated can't continue to be
supported forever, if you like.

Dired has very consistent command and argument
behaviors, especially when it comes to commands
that act on or are affected by marks.  It's a
pity to keep this wart - in the name of what
exactly?

It's trivial to make the renaming 100% painless
to all users of the old names - they'd just
continue to work, as long as you like. 

And what about changing the behavior of the ARG
argument?  Or if you don't change its behavior
to fit Dired, what about changing its name to
reflect what it does?