bug#35564: 27.0.50; [PATCH] Tweak dired-do-shell-command warning about "wildcard" characters

classic Classic list List threaded Threaded
34 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#35564: 27.0.50; [PATCH] Tweak dired-do-shell-command warning about "wildcard" characters

Kévin Le Gouguec
Hello,

The function dired-do-shell-command checks the user's command for any
star or question mark not surrounded by whitespace or backquotes,
asking whether they are deliberate, since the character will then be
sent as-is to the shell, instead of being replaced with the marked
file(s).

A silly example:

- Open a Dired buffer
- M-! echo "Foobar." > foo RET
- g
- with point on foo:
    - ! sed 's/\./?/' RET

The way the question is phrased bothers me:

> Confirm--do you mean to use `?' as a wildcard?

The first time I met this prompt was when I included a quoted '?' in
my command as in the example above, so I definitely did *not* mean to
use '?' as a shell wildcard.

Even now, knowing what the question really means, it still trips my
brain that I must answer "yes" (as in, "yes, I know Dired will not
substitute the marked files") when I mean "no" (as in, "no, I don't
mean to use '?' as a wildcard, what is this even ab- oh wait no right
I meant yes! Yes! 🤦").

I can think of a few ways to solve this:

1. Rephrase the question to be more general, specifically without
   calling the characters "wildcards"; for example:

> Confirm--do you mean to send `?' to the shell without substitution?

2. Parse the command to find out whether the shell will actually use
   these characters as wildcards.
    - not sure how portable this would be across different shells
    - AFAICT the aim of this prompt is simply to warn the user that
      Dired will not expand these characters; whether the shell will
      process them as wildcards is irrelevant

3. Add an option to skip this question (more of a workaround than a
   solution).

Favoring option #1, I tried to find alternative questions, but none of
the ones I came up with sounded satisfying (most of them included some
form of double-negation, which is not the kind of puzzle I want to
solve when I'm about to run a hastily-put-together Bash oneliner).

I played around with the idea of actually *showing* the
"unsubstituted" characters to the user in order to be able to say
something like…

> Confirm--the highlighted characters will not be substituted.
> Proceed?

… and ended up with the attached patch, which I am not entirely
satisfied with (for one, it replaces `y-or-n-p' with `yes-or-no-p'
merely because the former seems to strip my prompt's text attributes
somehow[1]).




WDYT?  Assuming that Dired calling unsubstituted characters
"wildcards" is indeed a problem,

- can someone come up with a better phrasing?
- is the highlighting, as implemented in this patch, helpful?
- does anybody know why `y-or-n-p' prompts lose their face property?


Thank you for your time.

Kévin


[1] Compare:

(let ((prompt "foobar "))
  (add-face-text-property 3 6 'warning nil prompt)
  (yes-or-no-p prompt))

With:

(let ((prompt "foobar "))
  (add-face-text-property 3 6 'warning nil prompt)
  (y-or-n-p prompt))


In GNU Emacs 27.0.50 (build 2, i686-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2019-05-02 built on nc10-laptop
Repository revision: 17a722982cca4e8e643c7a9102903e820e784cc6
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description: BunsenLabs GNU/Linux 9.8 (Helium)

Recent messages:
Mark saved where search started
Mark set
Mark saved where search started
Mark set
Making completion list...
Quit [3 times]
Mark set
Quit [2 times]
Type "q" in help window to restore its previous buffer, C-M-v to scroll help.
Quit
Quit
Configured using:
 'configure --with-xwidgets'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS GLIB
NOTIFY INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF
XFT ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM THREADS XWIDGETS JSON
PDUMPER LCMS2 GMP

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  global-magit-file-mode: t
  magit-file-mode: t
  magit-auto-revert-mode: t
  auto-revert-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  shell-dirtrack-mode: t
  show-paren-mode: t
  minibuffer-depth-indicate-mode: t
  icomplete-mode: t
  global-page-break-lines-mode: t
  page-break-lines-mode: t
  electric-pair-mode: t
  diff-hl-flydiff-mode: t
  global-diff-hl-mode: t
  diff-hl-mode: t
  delete-selection-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort emacsbug sendmail nndoc gnus-dup mm-archive url-cache
debbugs-gnu debbugs soap-client url-http url-auth url-gw url url-proxy
url-privacy url-expand url-methods url-history url-cookie url-domsuf
url-util warnings rng-xsd rng-dt rng-util xsd-regexp xml tabify man
mail-extr ffap pulse diff-hl-dired magit-patch flyspell ispell dired-aux
dired-x magit-extras hi-lock cc-mode cc-fonts cc-guess cc-menus cc-cmds
cc-styles cc-align cc-engine cc-vars cc-defs cus-edit whitespace
find-dired xref magit-submodule magit-obsolete magit-blame magit-stash
magit-reflog magit-bisect magit-push magit-pull magit-fetch magit-clone
magit-remote magit-commit magit-sequence magit-notes magit-worktree
magit-tag magit-merge magit-branch magit-reset magit-files magit-refs
magit-status magit magit-repos magit-apply magit-wip magit-log
which-func imenu magit-diff smerge-mode magit-core magit-autorevert
autorevert filenotify magit-margin magit-transient magit-process
magit-mode transient git-commit magit-git magit-section log-edit
pcvs-util add-log with-editor async-bytecomp async server face-remap
eieio-opt speedbar sb-image ezimage dframe magit-utils crm dash shell
pcomplete ert pp gnus-async qp gnus-ml nndraft nnmh nnfolder utf-7
epa-file gnutls network-stream nsm gnus-agent gnus-srvr gnus-score
score-mode nnvirtual gnus-msg gnus-art mm-uu mml2015 mm-view mml-smime
smime dig mailcap nntp gnus-cache gnus-sum gnus-group gnus-undo
gnus-start gnus-cloud nnimap nnmail mail-source utf7 netrc nnoo
parse-time gnus-spec gnus-int gnus-range message rmc puny dired
dired-loaddefs format-spec rfc822 mml mml-sec epa derived epg mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
gnus-win gnus nnheader gnus-util rmail rmail-loaddefs rfc2047 rfc2045
ietf-drums text-property-search time-date mail-utils mm-util mail-prsvr
wid-edit markdown-mode rx color noutline outline vc-mtn vc-hg jka-compr
cl-print debug backtrace find-func thingatpt help-fns radix-tree
executable misearch multi-isearch vc-git vc-bzr vc-src vc-sccs vc-svn
vc-cvs vc-rcs project delight advice eighters-theme quail cl-extra
help-mode rg rg-ibuffer rg-result wgrep-rg wgrep s rg-history rg-header
rg-compat ibuf-ext ibuffer ibuffer-loaddefs grep compile comint
ansi-color ring edmacro kmacro disp-table paren mb-depth icomplete
page-break-lines elec-pair diff-hl-flydiff diff diff-hl vc-dir ewoc vc
vc-dispatcher diff-mode easy-mmode delsel cus-start cus-load mule-util
tex-site info package easymenu epg-config url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json subr-x map url-vars seq byte-opt gv bytecomp
byte-compile cconv cl-loaddefs cl-lib tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting xwidget-internal move-toolbar
gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 8 708714 111581)
 (symbols 24 31862 1)
 (strings 16 136515 44484)
 (string-bytes 1 4039230)
 (vectors 8 60958)
 (vector-slots 4 1347636 61628)
 (floats 8 3359 1168)
 (intervals 28 69005 689)
 (buffers 564 56))

0001-Make-dired-do-shell-command-highlight-unsubstituted-.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#35564: 27.0.50; [PATCH] Tweak dired-do-shell-command warning about "wildcard" characters

martin rudalics
 > … and ended up with the attached patch, which I am not entirely
 > satisfied with (for one, it replaces `y-or-n-p' with `yes-or-no-p'
 > merely because the former seems to strip my prompt's text attributes
 > somehow[1]).

[...]

 > [1] Compare:
 >
 > (let ((prompt "foobar "))
 >    (add-face-text-property 3 6 'warning nil prompt)
 >    (yes-or-no-p prompt))
 >
 > With:
 >
 > (let ((prompt "foobar "))
 >    (add-face-text-property 3 6 'warning nil prompt)
 >    (y-or-n-p prompt))

'y-or-n-p' propertizes the prompt rigidly as

                     (read-key (propertize (if (memq answer scroll-actions)
                                               prompt
                                             (concat "Please answer y or n.  "
                                                     prompt))
                                           'face 'minibuffer-prompt)))))

while 'yes-or-no-p' carefully applies 'minibuffer-prompt-properties'
to any text properties provided with PROMPT.

martin




Reply | Threaded
Open this post in threaded view
|

bug#35564: 27.0.50; [PATCH] Tweak dired-do-shell-command warning about "wildcard" characters

Kévin Le Gouguec
martin rudalics <[hidden email]> writes:

>> [1] Compare:
>>
>> (let ((prompt "foobar "))
>>    (add-face-text-property 3 6 'warning nil prompt)
>>    (yes-or-no-p prompt))
>>
>> With:
>>
>> (let ((prompt "foobar "))
>>    (add-face-text-property 3 6 'warning nil prompt)
>>    (y-or-n-p prompt))
>
> 'y-or-n-p' propertizes the prompt rigidly as
>
>                     (read-key (propertize (if (memq answer scroll-actions)
>                                               prompt
>                                             (concat "Please answer y or n.  "
>                                                     prompt))
>                                           'face 'minibuffer-prompt)))))
>
> while 'yes-or-no-p' carefully applies 'minibuffer-prompt-properties'
> to any text properties provided with PROMPT.
Well, that's interesting.

I dug into yes-or-no-p until I came across `read_minibuf()'; is this the
code you are referring to?

    if (PT > BEG)
      {
        Fput_text_property (make_fixnum (BEG), make_fixnum (PT),
                            Qfront_sticky, Qt, Qnil);
        Fput_text_property (make_fixnum (BEG), make_fixnum (PT),
                            Qrear_nonsticky, Qt, Qnil);
        Fput_text_property (make_fixnum (BEG), make_fixnum (PT),
                            Qfield, Qt, Qnil);
        if (CONSP (Vminibuffer_prompt_properties))
          {
            /* We want to apply all properties from
               `minibuffer-prompt-properties' to the region normally,
               but if the `face' property is present, add that
               property to the end of the face properties to avoid
               overwriting faces. */
            Lisp_Object list = Vminibuffer_prompt_properties;
            while (CONSP (list))
              {
                Lisp_Object key = XCAR (list);
                list = XCDR (list);
                if (CONSP (list))
                  {
                    Lisp_Object val = XCAR (list);
                    list = XCDR (list);
                    if (EQ (key, Qface))
                      Fadd_face_text_property (make_fixnum (BEG),
                                               make_fixnum (PT), val, Qt, Qnil);
                    else
                      Fput_text_property (make_fixnum (BEG), make_fixnum (PT),
                                          key, val, Qnil);
                  }
              }
          }
      }

If one were to fix the issue of y-or-n-p hardcoding the face property,
what would be the best way to go?

1. Make a C DEFUN out of this snippet, and have it called by
   `read_minibuf()' and `y-or-n-p'.

2. Re-implement this snippet as an Elisp defun, and have it called by
   `read_minibuf()' and `y-or-n-p'.

3. (Re-implement this snippet within `y-or-n-p'.)

(FWIW, the attached patch seems to work as a workaround, but I assume
solutions 1 or 2 would be better, by virtue of reusing code)




Thanks for your help!

Kévin

0001-Make-y-or-no-p-keep-the-supplied-prompt-s-face.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#35564: 27.0.50; [PATCH] Tweak dired-do-shell-command warning about "wildcard" characters

martin rudalics
 > I dug into yes-or-no-p until I came across `read_minibuf()'; is this the
 > code you are referring to?

It is.

 > If one were to fix the issue of y-or-n-p hardcoding the face property,
 > what would be the best way to go?
 >
 > 1. Make a C DEFUN out of this snippet, and have it called by
 >     `read_minibuf()' and `y-or-n-p'.
 >
 > 2. Re-implement this snippet as an Elisp defun, and have it called by
 >     `read_minibuf()' and `y-or-n-p'.
 >
 > 3. (Re-implement this snippet within `y-or-n-p'.)
 >
 > (FWIW, the attached patch seems to work as a workaround, but I assume
 > solutions 1 or 2 would be better, by virtue of reusing code)

By design, 'yes-or-no-p' and 'y-or-n-p' are kept apart to avoid that
people use the latter for more "crucial decisions".  Part of this
distinction is that 'y-or-n-p' is asked in the echo area, so applying
'minibuffer-prompt-properties' would be conceptually inappropriate.
Obviously, applying 'minibuffer-prompt' is just as inappropriate (that
face is part of 'minibuffer-prompt-properties') but that's a decision
that has been made long ago.

So although I'd vote for a solution like the one you propose in your
patch, any decision in this area is subtle and should be approved by
others first.  Also because we'd then have to decide what to do with
other clients of the 'minibuffer-prompt' face like 'read-char-choice'
or the ones in isearch.el.

martin



Reply | Threaded
Open this post in threaded view
|

bug#35564: 27.0.50; [PATCH] Tweak dired-do-shell-command warning about "wildcard" characters

Drew Adams
> By design, 'yes-or-no-p' and 'y-or-n-p' are kept apart to avoid that
> people use the latter for more "crucial decisions".  Part of this
> distinction is that 'y-or-n-p' is asked in the echo area, so applying
> 'minibuffer-prompt-properties' would be conceptually inappropriate.
> Obviously, applying 'minibuffer-prompt' is just as inappropriate (that
> face is part of 'minibuffer-prompt-properties') but that's a decision
> that has been made long ago.
>
> So although I'd vote for a solution like the one you propose in your
> patch, any decision in this area is subtle and should be approved by
> others first.  Also because we'd then have to decide what to do with
> other clients of the 'minibuffer-prompt' face like 'read-char-choice'
> or the ones in isearch.el.

I don't see any good reason why face `minibuffer-prompt'
should be used, especially by default (users can do
whatever they like) for situations where there is no
active minibuffer, i.e., for prompting situations
generally.  It should instead serve as a useful clue
that the minibuffer is being used.  (Just one opinion.)



Reply | Threaded
Open this post in threaded view
|

bug#35564: 27.0.50; [PATCH] Tweak dired-do-shell-command warning about "wildcard" characters

Kévin Le Gouguec
Drew Adams <[hidden email]> writes:

> I don't see any good reason why face `minibuffer-prompt'
> should be used, especially by default (users can do
> whatever they like) for situations where there is no
> active minibuffer, i.e., for prompting situations
> generally.  It should instead serve as a useful clue
> that the minibuffer is being used.  (Just one opinion.)

I'd hazard a guess[1] that this was done to make y-or-n-p and
yes-or-no-p similar from a UI point of view: they both prompt the user
for a binary answer, therefore the prompt might as well look the same in
both situations.

From what I can tell the use of 'minibuffer-prompt' in minibuffer-less
situations has enough precedents (the "other clients" Martin mentions)
that the "minibuffer-" prefix might be considered a historical accident
by now…

(Perhaps those clients could be migrated to a new face,
e.g. 'message-prompt', which would inherit 'minibuffer-prompt' by
default?)


martin rudalics <[hidden email]> writes:

> So although I'd vote for a solution like the one you propose in your
> patch, any decision in this area is subtle and should be approved by
> others first.  Also because we'd then have to decide what to do with
> other clients of the 'minibuffer-prompt' face like 'read-char-choice'
> or the ones in isearch.el.

Fair enough.  Should I raise the issue on emacs-devel, or create a new
bug report?  Just to make sure I am not omitting something, is this how
you would sum up the issue?

- In the context of bug#35564, I would like to add text properties to
  the y-or-n-p prompt (although I'm open to other, simpler solutions
  e.g. simply changing Dired's message).

- While this can be patched within y-or-n-p and we can call it a day,
  the minibuffer-prompt-face-adding code could be factored out of
  'read_minibuf'.

- This raises two questions:

    1. Do we actually want to use the 'minibuffer-prompt' face in this
       context, since the minibuffer is not involved?
       
    2. What do we do with other clients of 'minibuffer-prompt', which
       use the same (propertize prompt 'face 'minibuffer-prompt) idiom?


Thank you both for your thoughts on this.


[1] AFAICT, the commit that added this face (927be33, back when y-or-n-p
    was still a C function) does not say why this was thought to be a
    good idea.



Reply | Threaded
Open this post in threaded view
|

bug#35564: 27.0.50; [PATCH] Tweak dired-do-shell-command warning about "wildcard" characters

Drew Adams
> Drew Adams <[hidden email]> writes:
>
> > I don't see any good reason why face `minibuffer-prompt'
> > should be used, especially by default (users can do
> > whatever they like) for situations where there is no
> > active minibuffer, i.e., for prompting situations
> > generally.  It should instead serve as a useful clue
> > that the minibuffer is being used.  (Just one opinion.)
>
> I'd hazard a guess[1] that this was done to make y-or-n-p and
> yes-or-no-p similar from a UI point of view: they both prompt the user
> for a binary answer, therefore the prompt might as well look the same in
> both situations.
>
> From what I can tell the use of 'minibuffer-prompt' in minibuffer-less
> situations has enough precedents (the "other clients" Martin mentions)
> that the "minibuffer-" prefix might be considered a historical accident
> by now…

Not an accident.  The "precedents" are themselves bugs,
IMO.  The minibuffer is the minibuffer.  It's important
that users be aware when it is active.  That may not
matter much for `yes-or-no-p', as it insists on "yes"
or "no".  But it is important for the minibuffer in
general.  The minibuffer is an editing buffer; you can
do lots of things in it and with it.

> (Perhaps those clients could be migrated to a new face,
> e.g. 'message-prompt', which would inherit 'minibuffer-prompt' by
> default?)

That would be fine by me.  Or no face.  Is it really
important that the prompt be distinguished by a face?

(To be clear, I have no objection to our using faces
for prompts.)

> - In the context of bug#35564, I would like to add text properties to
>   the y-or-n-p prompt (although I'm open to other, simpler solutions
>   e.g. simply changing Dired's message).

I wouldn't mention bug #35564.  And that bug should not,
itself, also deal with prompt faces.  It should be only
about `dired-do-shell-command' warning about wildcard
characters.

Any question (for this command or in general) about
prompt faces is independent of this bug report about
the `dired-do-shell-command' treatment of wildcards.

(Just one opinion.)



Reply | Threaded
Open this post in threaded view
|

bug#35564: 27.0.50; [PATCH] Tweak dired-do-shell-command warning about "wildcard" characters

martin rudalics
In reply to this post by Kévin Le Gouguec
 > (Perhaps those clients could be migrated to a new face,
 > e.g. 'message-prompt', which would inherit 'minibuffer-prompt' by
 > default?)

Simply 'prompt' would be more intuitive since most messages do not show
prompts.  But I'm afraid we'll have to stick to what we have now.

 > Fair enough.  Should I raise the issue on emacs-devel, or create a new
 > bug report?  Just to make sure I am not omitting something, is this how
 > you would sum up the issue?
 >
 > - In the context of bug#35564, I would like to add text properties to
 >    the y-or-n-p prompt (although I'm open to other, simpler solutions
 >    e.g. simply changing Dired's message).
 >
 > - While this can be patched within y-or-n-p and we can call it a day,
 >    the minibuffer-prompt-face-adding code could be factored out of
 >    'read_minibuf'.
 >
 > - This raises two questions:
 >
 >      1. Do we actually want to use the 'minibuffer-prompt' face in this
 >         context, since the minibuffer is not involved?
 >
 >      2. What do we do with other clients of 'minibuffer-prompt', which
 >         use the same (propertize prompt 'face 'minibuffer-prompt) idiom?

That would sum it up (but since I don't use Dired I can't comment on
that).

Note that this isssue also touches one Drew raised elsewhere - whether
'tooltip-show' should retain face properties of the original text or
show text uniformly with the 'tooltip' face.  Maybe we should
introduce an option like 'prompts-retain-text-properites' so users
have the choice.

 > [1] AFAICT, the commit that added this face (927be33, back when y-or-n-p
 >      was still a C function) does not say why this was thought to be a
 >      good idea.

It's probably part of a concept to have prompts appear uniformly and
integrate them into the framework of themes.  Note that Emacs usually
cannot control the appearance of dialog boxes or system tooltips.

martin



Reply | Threaded
Open this post in threaded view
|

bug#35564: 27.0.50; [PATCH] Tweak dired-do-shell-command warning about "wildcard" characters

Drew Adams
>  > (Perhaps those clients could be migrated to a new face,
>  > e.g. 'message-prompt', which would inherit 'minibuffer-prompt' by
>  > default?)
>
> Simply 'prompt' would be more intuitive since most messages do not show
> prompts.

Sounds OK to me.

> But I'm afraid we'll have to stick to what we have now.

Why?  Not sure what you mean.

> Note that this isssue also touches one Drew raised elsewhere - whether
> 'tooltip-show' should retain face properties of the original text or
> show text uniformly with the 'tooltip' face.  Maybe we should
> introduce an option like 'prompts-retain-text-properites' so users
> have the choice.

I would prefer that the two be separated.  Tooltip text
is quite different from prompts in use cases and behavior.

Wrt tooltips, I also don't see why we need an option, or
even a defvar.  Tooltips should just accept and respect
propertized strings.

When you use `x-show-tip' there is no such problem - you
can apply properties as usual.  It is only `help-echo'
tooltips that do not respect properties (AFAIK).

Another, simpler possibility, for dealing with face
`tooltip':

It is not possible to simply _bind_ a face for the
duration (or lex scope) of a function.  But if we
use a face variable for this, e.g. `tooltip-face',
then it should be simple to do so.

IOW, work around the limitation that you cannot bind
a face by binding a face variable and using that for
`help-echo'.  That should make it simple for any code
to control the appearance of the tooltip text.



Reply | Threaded
Open this post in threaded view
|

bug#35564: 27.0.50; [PATCH] Tweak dired-do-shell-command warning about "wildcard" characters

martin rudalics
 >> But I'm afraid we'll have to stick to what we have now.
 >
 > Why?  Not sure what you mean.

I mean that it won't be easy to convince others that we need a new
face for propertizing prompts.

 > I would prefer that the two be separated.  Tooltip text
 > is quite different from prompts in use cases and behavior.

They are similar in the following aspect: Both prompts and tooltips
are often displayed using toolkit functions.  GTK tooltips are by
default not propertized because the system doesn't accept any face
properties for them.  If Emacs used balloon tooltips on Windows,
propertizing them would not be possible either.  And both 'y-or-n-p'
and 'yes-or-no-p', when implemented via dialog popups, don't adopt our
text properties either.

The question is now whether an application should accept the uniform
appearance of such objects as prescribed by the toolkit used and as
such obey the toolkit's look-and-feel or insist to use its own
implementations.  Emacs leaves that choice to its users.  Which means
that users are told things like "if you want this mode to behave as
intended, you have to customize variables like 'use-dialog-box' or
'x-gtk-use-system-tooltips'".  Nothing bad with that, but some users
might be uncertain whether they should agree.  In particular when such
an option affects all sorts of tooltips or prompts.

martin



Reply | Threaded
Open this post in threaded view
|

bug#35564: 27.0.50; [PATCH] Tweak dired-do-shell-command warning about "wildcard" characters

Drew Adams
>  >> But I'm afraid we'll have to stick to what we have now.
>  >
>  > Why?  Not sure what you mean.
>
> I mean that it won't be easy to convince others that we need a new
> face for propertizing prompts.

Why assume that?  Who needs to be convinced?

If we can't have a new face for this then I'd like
to see non-minibuffer prompting have no face at all,
by default.

>  > I would prefer that the two be separated.  Tooltip text
>  > is quite different from prompts in use cases and behavior.
>
> They are similar in the following aspect: Both prompts and tooltips
> are often displayed using toolkit functions.

That sounds like an implementation thing, not a user-level thing.

And as I said, `x-show-tooltip' has no problem with
showing propertized text.

> GTK tooltips are by
> default not propertized because the system doesn't accept any face
> properties for them.  If Emacs used balloon tooltips on Windows,
> propertizing them would not be possible either.  And both 'y-or-n-p'
> and 'yes-or-no-p', when implemented via dialog popups, don't adopt our
> text properties either.

Oh, so `x-show-tooltip' only supports properties on
some platforms (e.g. MS Windows)?  If so, that's
unfortunate.

Yes, I don't expect window-dialogs to respect
propertized text.  Again, unfortunate - but livable.

> The question is now whether an application should accept the uniform
> appearance of such objects as prescribed by the toolkit used and as
> such obey the toolkit's look-and-feel or insist to use its own
> implementations.  Emacs leaves that choice to its users.  Which means
> that users are told things like "if you want this mode to behave as
> intended, you have to customize variables like 'use-dialog-box' or
> 'x-gtk-use-system-tooltips'".  Nothing bad with that, but some users
> might be uncertain whether they should agree.  In particular when such
> an option affects all sorts of tooltips or prompts.

Got it; thx.

I would like to see Emacs allow, when possible, Emacsy
things as much as possible.  But I can understand that
there can be some tradeoffs.



Reply | Threaded
Open this post in threaded view
|

bug#35564: [PATCH v2] Tweak dired warning about "wildcard" characters

Kévin Le Gouguec
In reply to this post by Kévin Le Gouguec
Hello,

Here is my second attempt at solving this issue.

To recap: dired-do-shell-command warns the user about non-isolated '*'
and '?' characters since the function will not substitute them.  It
refers to these characters as "wildcards", which can be incorrect: they
may be quoted or backslash-escaped, in which case the shell will not
interpret them as wildcards.

My main motivation to change this warning is that it trips my brain to
have to answer "yes" ("yes, I want to use wildcards") when no wildcards
are involved.

I could not come up with a simple, self-sufficient rephrasing for the
warning, so I decided to display the command itself as part of the
warning prompt, highlighting the non-isolated characters.

The first patch adjusts y-or-n-p so that it preserves the prompt's text
properties.  The second patch changes dired-do-shell-command's prompt.



Sample screenshot:



I am not sure these patches should be applied as-is.  Some things I
wonder about:

1. About read--propertize-prompt…

    1. Should the function return a copy of its argument instead of
       propertizing it directly?

    2. Is it properly named?  Does it fit in subr.el?  I placed it there
       because I figured other users of read-char in subr.el could use
       it, e.g. read-char-choice.

2. dired-aux.el already contains some logic to detect isolated
   characters; I could not think of a way to re-use it, so I added my
   own functions to find *non*-isolated characters.  I added unit tests
   for these new functions; still, there may be some redundancy there.

WDYT?


PS1: I am still absolutely open to simply rephrasing the prompt…  I just
cannot come up with good alternatives.

PS2: CC'ing Stefan to follow up on the discussion on emacs-devel.
<https://lists.gnu.org/archive/html/emacs-devel/2019-05/msg00339.html>

0001-Preserve-text-properties-in-y-or-n-p-prompts.patch (2K) Download Attachment
0002-Tweak-dired-warning-about-wildcard-characters.patch (5K) Download Attachment
dired-warning-highlight.png (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#35564: [PATCH v2] Tweak dired warning about "wildcard" characters

Noam Postavsky
Kévin Le Gouguec <[hidden email]> writes:

> +(defun dired--isolated-char-p (command pos)
> +  "Assert whether the character at POS is isolated within COMMAND.
> +A character is isolated if:
> +- it is surrounded by whitespace, the start of the command, or
> +  the end of the command,
> +- it is surrounded by `\\=`' characters."
> +  (let ((n (length command))
> +        (whitespace '(?\s ?\t)))
> +    (or (= n 1)
> +        (and (= pos 0)
> +             (memq (elt command 1) whitespace))
> +        (and (= pos (1- n))
> +             (memq (elt command (1- pos)) whitespace))
> +        (and
> +         (> pos 0)
> +         (< pos (1- n))
> +         (let ((prev (elt command (1- pos)))
> +               (next (elt command (1+ pos))))
> +           (or (and (memq prev whitespace)
> +                    (memq next whitespace))
> +               (and (= prev ?`)
> +                    (= next ?`))))))))

I think this might be better expressed in regexp:

  (and (string-match
        (rx-to-string '(seq (or bos (in blank ?`))
                            (group (eval (string (aref command pos))))
                            (or eos (in blank ?`))))
        command (max 0 (1- pos)))
       (= pos (match-beginning 1)))

Although this gives different results for things like:

    (dired--isolated-char-p "?`foo`" 0)

Do we care about that?  (if yes, then that's a missing test case)

> +(defun dired--highlight-nosubst-char (command char)
> +  "Highlight occurences of CHAR that are not isolated in COMMAND.
> +These occurences will not be substituted; they will be sent as-is
> +to the shell, which may interpret them as wildcards."
> +  (save-match-data
> +    (let ((highlighted (substring-no-properties command))
> +          (pos 0))
> +      (while (string-match (regexp-quote char) command pos)
> +        (let ((start (match-beginning 0))
> +              (end (match-end 0)))
> +          (unless (dired--isolated-char-p command start)
> +            (add-face-text-property start end 'warning nil highlighted))
> +          (setq pos end)))
> +      highlighted)))

And perhaps the regexp above (if it's correct) can be integrated into
this search?  (maybe not though, since negation isn't straightforward in
regexps)



Reply | Threaded
Open this post in threaded view
|

bug#35564: [PATCH v2] Tweak dired warning about "wildcard" characters

Stefan Monnier
>         (rx-to-string '(seq (or bos (in blank ?`))
>                             (group (eval (string (aref command pos))))
>                             (or eos (in blank ?`))))

Can't we use `(... ,.. ..) instead of `eval`?


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#35564: [PATCH v2] Tweak dired warning about "wildcard" characters

Kévin Le Gouguec
In reply to this post by Noam Postavsky
Noam Postavsky <[hidden email]> writes:

> I think this might be better expressed in regexp:

I guess it is; I just tend to get superstitious about messing with match
data.

> Although this gives different results for things like:
>
>     (dired--isolated-char-p "?`foo`" 0)
>
> Do we care about that?  (if yes, then that's a missing test case)

I think we do care; if I look at what the existing code says,

    (dired--star-or-qmark-p "?`foo`" "?")
    ;; => nil

Off the top of my head, this is the best I can come up with to satisfy
this edge case:

    (defun dired--isolated-char-p (command pos)
      "Assert whether the character at POS is isolated within COMMAND.
    A character is isolated if:
    - it is surrounded by whitespace, the start of the command, or
      the end of the command,
    - it is surrounded by `\\=`' characters."
      (let ((start (max 0 (1- pos)))
            (char (string (aref command pos))))
        (and (or (string-match
                  (rx-to-string '(seq (or bos blank)
                                      (group char)
                                      (or eos blank)))
                  command start)
                 (string-match
                  (rx-to-string '(seq ?`
                                      (group char)
                                      ?`))
                  command start))
             (= pos (match-beginning 1)))))

> And perhaps the regexp above (if it's correct) can be integrated into
> this search?  (maybe not though, since negation isn't straightforward in
> regexps)

I will look into that.


Stefan Monnier <[hidden email]> writes:

> Can't we use `(... ,.. ..) instead of `eval`?
>
>
>         Stefan

That works too from what I tested, although it's not necessary anymore
with the new version above.


Thank you both for the review!  I will come back with an updated patch
(with the new test case) Some Time Later™.



Reply | Threaded
Open this post in threaded view
|

bug#35564: [PATCH v2] Tweak dired warning about "wildcard" characters

Stefan Monnier
> I guess it is; I just tend to get superstitious about messing with match
> data.

Instead, you should get superstitious about using the match data too
far from the corresponding match.


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#35564: [PATCH v2] Tweak dired warning about "wildcard" characters

Noam Postavsky
In reply to this post by Kévin Le Gouguec
>
> I think we do care; if I look at what the existing code says,
>
>     (dired--star-or-qmark-p "?`foo`" "?")
>     ;; => nil

>      (let ((start (max 0 (1- pos)))
>            (char (string (aref command pos))))

>                   (rx-to-string '(seq (or bos blank)
>                                       (group char)

`char' in this context translates to the "." regexp (i.e., any
character).  Yeah it's a bit weird.  I have a patch in mind to remove
the need for eval or rx-to-string.  I'll send in a few days (to a new
bug, it's getting off-topic here).

Meanwhile, I suggest:

  (let ((start (max 0 (1- pos)))
        (char (aref command pos)))
    (and (string-match
          (rx-to-string `(or (seq (or bos blank)
                                  (group-n 1 ,char)
                                  (or eos blank))
                             (seq ?` (group-n 1 ,char) ?`)))
          command start)
         (= pos (match-beginning 1))))



Reply | Threaded
Open this post in threaded view
|

bug#35564: [PATCH v2] Tweak dired warning about "wildcard" characters

Kévin Le Gouguec
[hidden email] writes:

>           (rx-to-string `(or (seq (or bos blank)
>                                   (group-n 1 ,char)
>                                   (or eos blank))
>                              (seq ?` (group-n 1 ,char) ?`)))

Ah!  Thanks for teaching me about \(?NUM: ... \), I did not know this
Elisp extension.  This makes the whole thing much more readable.

For a minute I thought that maybe this patch should also add (require
'rx) somewhere in dired-aux.el, but AFAICT this is not necessary since
rx-to-string is autoloaded…  Do I understand correctly?

Thanks again for the review, and for your efforts with bug#36237.


(And thank you Stefan for your advice on being superstitious about the
right things :) )



Reply | Threaded
Open this post in threaded view
|

bug#35564: [PATCH v2] Tweak dired warning about "wildcard" characters

Noam Postavsky
Kévin Le Gouguec <[hidden email]> writes:

> For a minute I thought that maybe this patch should also add (require
> 'rx) somewhere in dired-aux.el, but AFAICT this is not necessary since
> rx-to-string is autoloaded…  Do I understand correctly?

Yes, that's correct.



Reply | Threaded
Open this post in threaded view
|

bug#35564: [PATCH v3] Tweak dired warning about "wildcard" characters

Kévin Le Gouguec
In reply to this post by Kévin Le Gouguec
And here is the third set of patches.



The first patch is unchanged (it adjusts y-or-n-p so that the prompt's
text properties are preserved), the second patch uses the new
(literal …) feature from rx and adds a test case for
dired--isolated-char-p[1].

Quoting my previous email:

> Some things I wonder about:
>
> 1. About read--propertize-prompt…
>
>     1. Should the function return a copy of its argument instead of
>        propertizing it directly?
>
>     2. Is it properly named?  Does it fit in subr.el?  I placed it there
>        because I figured other users of read-char in subr.el could use
>        it, e.g. read-char-choice.
>
> 2. dired-aux.el already contains some logic to detect isolated
>    characters; I could not think of a way to re-use it, so I added my
>    own functions to find *non*-isolated characters.  I added unit tests
>    for these new functions; still, there may be some redundancy there.
Thank you for your time.


[1] (should-not (dired--isolated-char-p "foo `bar`?" 9))


0001-Preserve-text-properties-in-y-or-n-p-prompts.patch (2K) Download Attachment
0002-Tweak-dired-warning-about-wildcard-characters.patch (5K) Download Attachment
12