bug#61613: 30.0.50; ffap-other-window ignores ffap-require-prefix

bug#61613: 30.0.50; ffap-other-window ignores ffap-require-prefix

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi:

I just found that ffap-other-window ignores when ffap-require-prefix is
non-nil.

Looking at the code it seems to be that find-file-at-point has:

(and (called-interactively-p 'interactive)
     (if ffap-require-prefix (not current-prefix-arg)
	 current-prefix-arg))

but ffap-other-window calls find-file-at-point no interactively. Same
for ffap-other-frame, ffap-other-tab and so on...

I am not sure if the (called-interactively-p 'interactive) test is
actually needed in the first place, but I could be wrong. Is this
intended?

Best,
Ergus



In GNU Emacs 30.0.50 (build 14, x86_64-pc-linux-gnu, GTK+ Version
 3.24.36, cairo version 1.17.8) of 2023-02-19 built on Ergus
Repository revision: 750bc57cbb8d081566e671e8fc3e27a82588c197
Repository branch: master
System Description: Arch Linux

Configured using:
 'configure --prefix=/home/ergo/.local/ --with-mailutils --with-json
 --with-x-toolkit=gtk3 --with-xft --with-modules --with-cairo
 --with-harfbuzz --with-native-compilation
 '--program-transform-name=s/^ctags$/ctags.emacs/''

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER
PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS
TREE_SITTER WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

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

Major mode: ELisp/l

Minor modes in effect:
  windmove-mode: t
  global-auto-revert-mode: t
  fido-vertical-mode: t
  icomplete-vertical-mode: t
  icomplete-mode: t
  fido-mode: t
  electric-pair-mode: t
  flyspell-mode: t
  company-mode: t
  flycheck-mode: t
  diff-hl-margin-local-mode: t
  diff-hl-margin-mode: t
  diff-hl-mode: t
  gtags-mode: t
  repeat-mode: t
  xterm-mouse-mode: t
  xclip-mode: t
  override-global-mode: t
  winner-mode: t
  save-place-mode: t
  delete-selection-mode: t
  savehist-mode: t
  global-display-fill-column-indicator-mode: t
  display-fill-column-indicator-mode: t
  global-display-line-numbers-mode: t
  display-line-numbers-mode: t
  which-key-mode: t
  eldoc-mode: t
  show-paren-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  context-menu-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  size-indication-mode: t
  column-number-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/mnt/casa/gits/emacs_clones/gtags-mode/gtags-mode hides /home/ergo/.config/emacs/elpa/gtags-mode-1.0/gtags-mode
/home/ergo/.config/emacs/elpa/transient-20230214.1525/transient hides /home/ergo/.local/share/emacs/30.0.50/lisp/transient

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny rfc822
mml mml-sec epa derived epg rfc6068 epg-config gnus-util mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils pulse
color shortdoc time-date dabbrev cape-keyword move-dup misearch
multi-isearch company-elisp vc-git bookmark text-property-search pp
windmove jka-compr cl-print help-fns radix-tree autorevert filenotify
ffap thingatpt url-parse auth-source password-cache url-vars cus-start
cus-load icomplete elec-pair flyspell-correct flyspell ispell
company-semantic company-template company-capf company flycheck
ansi-color json map find-func dash pcase diff-hl-margin diff-hl-dired
dired-x dired dired-loaddefs diff-hl log-view pcvs-util vc-dir ewoc vc
vc-dispatcher diff-mode cape compat comp comp-cstr warnings icons rx
gtags-mode subr-x files-x xref project term/tmux term/xterm xterm init
repeat xt-mouse xclip edmacro kmacro use-package-bind-key bind-key
simple-16-theme winner ring saveplace delsel savehist easy-mmode
display-fill-column-indicator display-line-numbers diminish which-key
cl-extra help-mode use-package-diminish use-package-core disp-table info
dumb-jump-autoloads highlight-indent-guides-autoloads
company-lua-autoloads vundo-autoloads sudo-edit-autoloads
cuda-mode-autoloads nginx-mode-autoloads crdt-autoloads magit-autoloads
company-auctex-autoloads groovy-mode-autoloads flycheck-rust-autoloads
string-inflection-autoloads company-c-headers-autoloads
protobuf-mode-autoloads markdown-mode-autoloads tree-sitter-autoloads
lice-autoloads lorem-ipsum-autoloads julia-mode-autoloads
nasm-mode-autoloads deadgrep-autoloads popup-autoloads
company-nginx-autoloads csv-mode-autoloads d-mode-autoloads
tsc-autoloads i3wm-config-mode-autoloads magit-section-autoloads
ssh-config-mode-autoloads move-dup-autoloads clang-format-autoloads
esup-autoloads gnuplot-autoloads web-completion-data-autoloads
phi-search-autoloads better-shell-autoloads fancy-compilation-autoloads
arduino-cli-mode-autoloads flycheck-julia-autoloads cape-autoloads
auctex-autoloads tex-site which-key-autoloads slime-autoloads
flycheck-autoloads multiple-cursors-autoloads ibuffer-sidebar-autoloads
systemd-autoloads pkgbuild-mode-autoloads neotree-autoloads
modern-cpp-font-lock-autoloads company-reftex-autoloads
google-c-style-autoloads flymake-nasm-autoloads request-autoloads
caml-autoloads arduino-mode-autoloads ede/auto eieio-base cl-seq eieio
byte-opt bytecomp byte-compile eieio-core cl-macs gv cl-loaddefs cl-lib
sphinx-mode-autoloads f-autoloads diff-hl-autoloads lua-mode-autoloads
gtags-mode-autoloads mutt-mode-autoloads xclip-autoloads
diminish-autoloads imenu-list-autoloads paradox-autoloads
spinner-autoloads avy-zap-autoloads nftables-mode-autoloads s-autoloads
ibuffer-vc-autoloads objed-autoloads iedit-autoloads
languagetool-autoloads vterm-toggle-autoloads vterm-autoloads
avy-autoloads git-timemachine-autoloads emamux-autoloads
flymake-quickdef-autoloads yasnippet-snippets-autoloads
ibuffer-project-autoloads haskell-mode-autoloads
shell-command+-autoloads notmuch-autoloads e2ansi-autoloads
face-explorer-autoloads pkg-info-autoloads flx-autoloads
opencl-mode-autoloads ptemplate-templates-autoloads ptemplate-autoloads
yasnippet-autoloads git-modes-autoloads evil-collection-autoloads
annalist-autoloads ibuffer-tramp-autoloads debbugs-autoloads
evil-autoloads goto-chg-autoloads cobol-mode-autoloads
dired-sidebar-autoloads dired-subtree-autoloads
dired-hacks-utils-autoloads git-commit-autoloads with-editor-autoloads
transient-autoloads compat-autoloads macrostep-autoloads
flyspell-correct-autoloads company-autoloads dash-autoloads
epl-autoloads vdiff-autoloads hydra-autoloads lv-autoloads early-init
rmc iso-transl tooltip cconv eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode 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 lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
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 emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo move-toolbar gtk x-toolkit xinput2 x multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 281356 45353)
 (symbols 48 17929 0)
 (strings 32 63749 4451)
 (string-bytes 1 2119985)
 (vectors 16 33013)
 (vector-slots 8 925310 53948)
 (floats 8 340 2501)
 (intervals 56 11485 692)
 (buffers 984 18))

bug#61613: 30.0.50; ffap-other-window ignores ffap-require-prefix

[Eli Zaretskii]

> Date: Sun, 19 Feb 2023 03:45:30 +0100
> From:  Ergus via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I just found that ffap-other-window ignores when ffap-require-prefix is
> non-nil.
> 
> Looking at the code it seems to be that find-file-at-point has:
> 
> (and (called-interactively-p 'interactive)
>      (if ffap-require-prefix (not current-prefix-arg)
> 	 current-prefix-arg))
> 
> but ffap-other-window calls find-file-at-point no interactively. Same
> for ffap-other-frame, ffap-other-tab and so on...

What would be the point of supporting ffap-require-prefix when you
call ffap-other-window?  That variable controls whether to invoke
ffap-file-finder, which is not suitable for -other-window commands, I
think?  That is why, AFAIU, when not called interactively,
find-file-at-point doesn't call ffap-file-finder at all, and therefore
there's no point in heeding ffap-require-prefix.  Right?

bug#61613: 30.0.50; ffap-other-window ignores ffap-require-prefix

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On February 19, 2023 10:21:02 AM GMT+01:00, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Sun, 19 Feb 2023 03:45:30 +0100
>> From:  Ergus via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> I just found that ffap-other-window ignores when ffap-require-prefix is
>> non-nil.
>> 
>> Looking at the code it seems to be that find-file-at-point has:
>> 
>> (and (called-interactively-p 'interactive)
>>      (if ffap-require-prefix (not current-prefix-arg)
>> 	 current-prefix-arg))
>> 
>> but ffap-other-window calls find-file-at-point no interactively. Same
>> for ffap-other-frame, ffap-other-tab and so on...
>
>What would be the point of supporting ffap-require-prefix when you
>call ffap-other-window?  That variable controls whether to invoke
>ffap-file-finder, which is not suitable for -other-window commands, I
>think?  That is why, AFAIU, when not called interactively,
>find-file-at-point doesn't call ffap-file-finder at all, and therefore
>there's no point in heeding ffap-require-prefix.  Right?

Hi Eli:

Yes, that's what I expect, but it is actually offering the completion when using the other-window alternative.


Just:

emacs -Q
M-x ffap-bindings
M-: (setq ffap-require-prefix t)
Write /somedir

C-x C-f  (you see: ~/)

But 

C-x 4 f (you see /somedir)

This seems to be related with the ffap-prompter called before the switch then?

Best,







-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

bug#61613: 30.0.50; ffap-other-window ignores ffap-require-prefix

[Eli Zaretskii]

> Date: Sun, 19 Feb 2023 11:09:04 +0100
> From: Ergus <spacibba@aol.com>
> CC: 61613@debbugs.gnu.org
> 
> emacs -Q
> M-x ffap-bindings
> M-: (setq ffap-require-prefix t)
> Write /somedir
> 
> C-x C-f  (you see: ~/)
> 
> But 
> 
> C-x 4 f (you see /somedir)
> 
> This seems to be related with the ffap-prompter called before the switch then?

Yes, I think so.

bug#61613: 30.0.50; ffap-other-window ignores ffap-require-prefix

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Sun, Feb 19, 2023 at 12:15:49PM +0200, Eli Zaretskii wrote:
>> Date: Sun, 19 Feb 2023 11:09:04 +0100
>> From: Ergus <spacibba@aol.com>
>> CC: 61613@debbugs.gnu.org
>>
>> emacs -Q
>> M-x ffap-bindings
>> M-: (setq ffap-require-prefix t)
>> Write /somedir
>>
>> C-x C-f  (you see: ~/)
>>
>> But
>>
>> C-x 4 f (you see /somedir)
>>
>> This seems to be related with the ffap-prompter called before the switch then?
>
>Yes, I think so.

Hi Eli:

So, what do you think? The fix may be to put the ffap-require-prefix
condition in ffap-read-file-or-url directly or in ffap-prompter??

bug#61613: 30.0.50; ffap-other-window ignores ffap-require-prefix

[Eli Zaretskii]

> Date: Sun, 19 Feb 2023 22:32:55 +0100
> From: Ergus <spacibba@aol.com>
> Cc: 61613@debbugs.gnu.org
> 
> On Sun, Feb 19, 2023 at 12:15:49PM +0200, Eli Zaretskii wrote:
> >> Date: Sun, 19 Feb 2023 11:09:04 +0100
> >> From: Ergus <spacibba@aol.com>
> >> CC: 61613@debbugs.gnu.org
> >>
> >> emacs -Q
> >> M-x ffap-bindings
> >> M-: (setq ffap-require-prefix t)
> >> Write /somedir
> >>
> >> C-x C-f  (you see: ~/)
> >>
> >> But
> >>
> >> C-x 4 f (you see /somedir)
> >>
> >> This seems to be related with the ffap-prompter called before the switch then?
> >
> >Yes, I think so.
> 
> Hi Eli:
> 
> So, what do you think? The fix may be to put the ffap-require-prefix
> condition in ffap-read-file-or-url directly or in ffap-prompter??

I don't understand what you want to achieve by that, so please
elaborate.  ffap-file-finder's default value is a command that doesn't
display the file in another window, so how will ffap-require-prefix
help here?

bug#61613: 30.0.50; ffap-other-window ignores ffap-require-prefix

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Mon, Feb 20, 2023 at 02:50:18PM +0200, Eli Zaretskii wrote:
>> Date: Sun, 19 Feb 2023 22:32:55 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: 61613@debbugs.gnu.org
>>
>> On Sun, Feb 19, 2023 at 12:15:49PM +0200, Eli Zaretskii wrote:
>> >> Date: Sun, 19 Feb 2023 11:09:04 +0100
>> >> From: Ergus <spacibba@aol.com>
>> >> CC: 61613@debbugs.gnu.org
>> >>
>> >> emacs -Q
>> >> M-x ffap-bindings
>> >> M-: (setq ffap-require-prefix t)
>> >> Write /somedir
>> >>
>> >> C-x C-f  (you see: ~/)
>> >>
>> >> But
>> >>
>> >> C-x 4 f (you see /somedir)
>> >>
>> >> This seems to be related with the ffap-prompter called before the switch then?
>> >
>> >Yes, I think so.
>>
>> Hi Eli:
>>
>> So, what do you think? The fix may be to put the ffap-require-prefix
>> condition in ffap-read-file-or-url directly or in ffap-prompter??
>
>I don't understand what you want to achieve by that, so please
>elaborate.  ffap-file-finder's default value is a command that doesn't
>display the file in another window, so how will ffap-require-prefix
>help here?

 From the example above I expect that after:

(setq ffap-require-prefix t)
C-x 4 f

You see ~/ instead of /somedir. Like when we do C-x C-f.

As I said before, ffap-prompter is causing this because it is called
before switching window.

So ffap-file-finder is actually called at point before going to another
window. And it does not have any ffap-require-prefix condition. My
question is actually that.

We need to put the ffap-require-prefix condition somewhere in
ffap-read-file-or-url or ffap-prompter. Because this problem happens
with the other-frame and other-tab variants as well.

bug#61613: 30.0.50; ffap-other-window ignores ffap-require-prefix

[Eli Zaretskii]

> Date: Mon, 20 Feb 2023 16:49:15 +0100
> From: Ergus <spacibba@aol.com>
> Cc: 61613@debbugs.gnu.org
> 
> >> So, what do you think? The fix may be to put the ffap-require-prefix
> >> condition in ffap-read-file-or-url directly or in ffap-prompter??
> >
> >I don't understand what you want to achieve by that, so please
> >elaborate.  ffap-file-finder's default value is a command that doesn't
> >display the file in another window, so how will ffap-require-prefix
> >help here?
> 
>  From the example above I expect that after:
> 
> (setq ffap-require-prefix t)
> C-x 4 f
> 
> You see ~/ instead of /somedir. Like when we do C-x C-f.

That's not what ffap-require-prefix does:

  (defcustom ffap-require-prefix nil
    "If set, reverses the prefix argument to `find-file-at-point'.
  This is nil so neophytes notice ffap.  Experts may prefer to disable
  ffap most of the time."

> As I said before, ffap-prompter is causing this because it is called
> before switching window.

So what we need is fix how the prompt is calculated, not what is the
meaning of the prefix arg.

> So ffap-file-finder is actually called at point before going to another
> window. And it does not have any ffap-require-prefix condition. My
> question is actually that.

Once again, calling ffap-file-finder in this case is WRONG!  We need a
function that visits the file in another window, and ffap-file-finder
doesn't.

IOW, you are talking about a very different change to
ffap-other-window.

> We need to put the ffap-require-prefix condition somewhere in
> ffap-read-file-or-url or ffap-prompter. Because this problem happens
> with the other-frame and other-tab variants as well.

I think we need to rewrite the interactive spec of those to do what
you want.  ffap-require-prefix is not related to this at all.

bug#61613: 30.0.50; ffap-other-window ignores ffap-require-prefix

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

BTW:

Looking at the ffap code I think some minor update/cleanup will be
beneficial any-way.

Nothing very drastic, for example, isn't more consistent to make
ffap-bindings a minor mode to allow load and unload? Or use the
"interactive" in find-file-at-point to simplify the initial conditions
there and remove the optional? Or update the global-set-key to use the
new keymap-set thing? Or add autoload decorators to the interactive
functions?

WDYT?




On Sun, Feb 19, 2023 at 12:15:49PM +0200, Eli Zaretskii wrote:
>> Date: Sun, 19 Feb 2023 11:09:04 +0100
>> From: Ergus <spacibba@aol.com>
>> CC: 61613@debbugs.gnu.org
>>
>> emacs -Q
>> M-x ffap-bindings
>> M-: (setq ffap-require-prefix t)
>> Write /somedir
>>
>> C-x C-f  (you see: ~/)
>>
>> But
>>
>> C-x 4 f (you see /somedir)
>>
>> This seems to be related with the ffap-prompter called before the switch then?
>
>Yes, I think so.

bug#61613: 30.0.50; ffap-other-window ignores ffap-require-prefix

[Eli Zaretskii]

> Date: Sun, 19 Feb 2023 22:43:26 +0100
> From: Ergus <spacibba@aol.com>
> Cc: 61613@debbugs.gnu.org
> 
> Looking at the ffap code I think some minor update/cleanup will be
> beneficial any-way.
> 
> Nothing very drastic, for example, isn't more consistent to make
> ffap-bindings a minor mode to allow load and unload? Or use the
> "interactive" in find-file-at-point to simplify the initial conditions
> there and remove the optional? Or update the global-set-key to use the
> new keymap-set thing? Or add autoload decorators to the interactive
> functions?
> 
> WDYT?

It's hard to say without seeing the code you have in mind.  Please
post a patch, and let's take it from there.