bug#29599: 26.0; `dframe.el' binds keys unconditionally when loaded

bug#29599: 26.0; `dframe.el' binds keys unconditionally when loaded

[Drew Adams]

Well, not completely unconditionally.  But I see this:

(if (boundp 'special-event-map)
    (progn
      (define-key special-event-map [make-frame-visible]
	'dframe-handle-make-frame-visible)
      (define-key special-event-map [iconify-frame]
	'dframe-handle-iconify-frame)
      (define-key special-event-map [delete-frame]
	'dframe-handle-delete-frame))
  )

That condition that the map exists isn't, I think, sufficient to give it
license to bind keys in the map.

I must have done something after my init file was loaded that required
`dframe.elc' to be loaded, and that load overwrote bindings I made in my
init file.

I don't know what I did that caused dframe to be loaded, but that
shouldn't matter.  I don't think it should overwrite key bindings
just by being loaded.

In my init file I do this (after loading library `thumb-frm.el', which
defines `thumfr-thumbify-frame-upon-event'):

(when (and (eq system-type 'windows-nt)
           (fboundp 'thumfr-thumbify-frame-upon-event))
  (define-key special-event-map [iconify-frame]
              'thumfr-thumbify-frame-upon-event))

But dframe.el overwrites that binding when it gets loaded.

Shouldn't dframe.el bind its own commands to `special-event-map' keys
only if those keys are not already bound?

I don't know what it's doing or why, but this doesn't seem very polite
of it.  Why should it think that just by being loaded it should
(re-)bind keys?



In GNU Emacs 26.0.90 (build 3, x86_64-w64-mingw32)
 of 2017-10-13
Repository revision: 906224eba147bdfc0514090064e8e8f53160f1d4
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install 'CFLAGS=-O2 -static -g3''

bug#29599: 26.0; `dframe.el' binds keys unconditionally when loaded

[Noam Postavsky]

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

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

> I must have done something after my init file was loaded that required
> `dframe.elc' to be loaded, and that load overwrote bindings I made in my
> init file.
>
> I don't know what I did that caused dframe to be loaded, but that
> shouldn't matter.  I don't think it should overwrite key bindings
> just by being loaded.

Yeah (it's probably the loading-on-completion thing again).

> Shouldn't dframe.el bind its own commands to `special-event-map' keys
> only if those keys are not already bound?

> I don't know what it's doing or why, but this doesn't seem very polite
> of it.  Why should it think that just by being loaded it should
> (re-)bind keys?

The root problem is that there is no easy way to share the bindings.  I
think those keys should be bound to a function which calls runs a hook,
like focus-in and focus-out events are.  Actually, it sort of looks like
dframe is trying to install such a hook; the functions it puts don't do
anything except call `dframe-make-frame-{visible,iconify,delete}-function'.

Anyway, at a minimum, we can't have keybindings being modified by just a
load.  Here's a patch which moves the keybinding to dframe-frame-mode
activation instead.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1789 bytes --]

From e7965c6f4f9dcce7f38ab9a51cd2638d5feb5c66 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 18 Dec 2017 20:30:10 -0500
Subject: [PATCH] Don't bind dframe events on load (Bug#29599)

* lisp/dframe.el (dframe-set-special-events): New function, containing
previous top-level key binding code.
(dframe-frame-mode): Use it.
---
 lisp/dframe.el | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/lisp/dframe.el b/lisp/dframe.el
index 7f77d8991f..0766bd068d 100644
--- a/lisp/dframe.el
+++ b/lisp/dframe.el
@@ -288,6 +288,7 @@ dframe-frame-mode
 	(set frame-var nil))
     ;; Set this as our currently attached frame
     (setq dframe-attached-frame (selected-frame))
+    (dframe-set-special-events)
     (run-hooks popup-hook)
     ;; Updated the buffer passed in to contain all the hacks needed
     ;; to make it work well in a dedicated window.
@@ -543,16 +544,14 @@ dframe-detach
       )))
 
 ;;; Special frame event proxies
-;;
-(if (boundp 'special-event-map)
-    (progn
-      (define-key special-event-map [make-frame-visible]
-	'dframe-handle-make-frame-visible)
-      (define-key special-event-map [iconify-frame]
-	'dframe-handle-iconify-frame)
-      (define-key special-event-map [delete-frame]
-	'dframe-handle-delete-frame))
-  )
+(defun dframe-set-special-events ()
+  (when (boundp 'special-event-map)
+    (define-key special-event-map [make-frame-visible]
+      'dframe-handle-make-frame-visible)
+    (define-key special-event-map [iconify-frame]
+      'dframe-handle-iconify-frame)
+    (define-key special-event-map [delete-frame]
+      'dframe-handle-delete-frame)))
 
 (defvar dframe-make-frame-visible-function nil
   "Function used when a dframe controlled frame is de-iconified.
-- 
2.11.0

bug#29599: 26.0; `dframe.el' binds keys unconditionally when loaded

[Drew Adams]

> > I must have done something after my init file was loaded that required
> > `dframe.elc' to be loaded, and that load overwrote bindings I made in
> > my  init file.
> >
> > I don't know what I did that caused dframe to be loaded, but that
> > shouldn't matter.  I don't think it should overwrite key bindings
> > just by being loaded.
> 
> Yeah (it's probably the loading-on-completion thing again).

I don't think I know (or didn't know or at least don't
recall) anything about such a thing.  Is it something new?

> > Shouldn't dframe.el bind its own commands to `special-event-map' keys
> > only if those keys are not already bound?
> 
> > I don't know what it's doing or why, but this doesn't seem very polite
> > of it.  Why should it think that just by being loaded it should
> > (re-)bind keys?
> 
> The root problem is that there is no easy way to share the bindings.

Dunno what you mean by that ("share the bindings").
But maybe if I knew what "loading-on-completion" is
then I would understand "share the bindings" (?).

> I think those keys should be bound to a function which calls runs a hook,
> like focus-in and focus-out events are.

Maybe so, but then the question would be whether and why
dframe.el (or anything else) would initialize the hook
to have one or more functions on it.

In my case, I'm pretty sure I don't want anything other
than my replacement for iconification to be on the hook.
So I would probably empty the hook before using `add-hook'
for my function.  Maybe that's only out of ignorance of
what the deframe functions are for or do.

> Actually, it sort of looks like
> dframe is trying to install such a hook; the functions it puts don't do
> anything except call `dframe-make-frame-{visible,iconify,delete}-
> function'.

But (without looking at them), those sound like specific
replacements for the standard iconify etc.  If so, it's
great to provide such functions, but they shouldn't be
bound to special events by default (i.e., upon loading).

> Anyway, at a minimum, we can't have keybindings being modified by just a
> load.

Yes, thank you.

> Here's a patch which moves the keybinding to dframe-frame-mode
> activation instead.

I can't speak to the value of the patch (I know nothing
about this), but thanks for working on this.

bug#29599: 26.0; `dframe.el' binds keys unconditionally when loaded

[Noam Postavsky]

tags 29599 + patch
quit

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

>> Yeah (it's probably the loading-on-completion thing again).
>
> I don't think I know (or didn't know or at least don't
> recall) anything about such a thing.  Is it something new?

Yes, see Bug#28607.

>> Actually, it sort of looks like
>> dframe is trying to install such a hook; the functions it puts don't do
>> anything except call `dframe-make-frame-{visible,iconify,delete}-
>> function'.
>
> But (without looking at them), those sound like specific
> replacements for the standard iconify etc.  If so, it's
> great to provide such functions, but they shouldn't be
> bound to special events by default (i.e., upon loading).

As far as I can tell, there is no code in Emacs which sets those
functions to anything.  So presumably the idea is to allow the user to
run some code when a "dframe" is made visible/iconified/deleted.

>> Here's a patch which moves the keybinding to dframe-frame-mode
>> activation instead.
>
> I can't speak to the value of the patch (I know nothing
> about this), but thanks for working on this.

Okay, it should take care of this bug, and it should be perfectly safe,
since the functions do nothing before dframe-frame-mode is activated
anyway.  I'll push to emacs-26 in a few days if there are no objections.

bug#29599: 26.0; `dframe.el' binds keys unconditionally when loaded

[Drew Adams]

> tags 29599 + patch
> quit
> 
> Drew Adams <drew.adams@oracle.com> writes:
> 
> >> Yeah (it's probably the loading-on-completion thing again).
> >
> > I don't think I know (or didn't know or at least don't
> > recall) anything about such a thing.  Is it something new?
> 
> Yes, see Bug#28607.

That's quite a bug report - no report at all (?).

Perhaps it was because all of the actual report was
in another bug report that was closed, and that now
redirects to this (vacuous) report?

The only thing in #28607 is a link to an emacs-devel
thread.  No description of the problem (?).

Anyway, it's not clear to me how that bug relates to this one.

This one is about explicitly binding keys when the
source code loads.  I don't get the impression that
that one is about this at all.  (What am I missing?)

> > But (without looking at them), those sound like specific
> > replacements for the standard iconify etc.  If so, it's
> > great to provide such functions, but they shouldn't be
> > bound to special events by default (i.e., upon loading).
> 
> As far as I can tell, there is no code in Emacs which sets those
> functions to anything.  So presumably the idea is to allow the user to
> run some code when a "dframe" is made visible/iconified/deleted.

No doubt.  But (I think we agree?) that possibility
should be offered to users, to choose, and not imposed
just by loading the file.

> >> Here's a patch which moves the keybinding to dframe-frame-mode
> >> activation instead.
> >
> > I can't speak to the value of the patch (I know nothing
> > about this), but thanks for working on this.
> 
> Okay, it should take care of this bug, and it should be perfectly safe,
> since the functions do nothing before dframe-frame-mode is activated
> anyway.  I'll push to emacs-26 in a few days if there are no objections.

Thanks.

bug#29599: 26.0; `dframe.el' binds keys unconditionally when loaded

[Noam Postavsky]

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

> That's quite a bug report - no report at all (?).
>
> Perhaps it was because all of the actual report was
> in another bug report that was closed, and that now
> redirects to this (vacuous) report?
>
> The only thing in #28607 is a link to an emacs-devel
> thread.  No description of the problem (?).
>
> Anyway, it's not clear to me how that bug relates to this one.
>
> This one is about explicitly binding keys when the
> source code loads.  I don't get the impression that
> that one is about this at all.  (What am I missing?)

Oh, yeah, I probably should have pointed to the linked stuff.  Bug#28048
has more info.

Anyway, the relation is just that it probably explains how you got
dframe loaded (I have got dframe loaded in my session as well, even
though I don't use it).

bug#29599: 26.0; `dframe.el' binds keys unconditionally when loaded

[Drew Adams]

> the relation is just that it probably explains how you got
> dframe loaded (I have got dframe loaded in my session as well, even
> though I don't use it).

Got it.  Thanks.

bug#29599: 26.0; `dframe.el' binds keys unconditionally when loaded

[Noam Postavsky]

tags 29599 fixed
close 29599 26.1
quit

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> Okay, it should take care of this bug, and it should be perfectly safe,
> since the functions do nothing before dframe-frame-mode is activated
> anyway.  I'll push to emacs-26 in a few days if there are no objections.

Pushed to emacs-26, but I modified the patch a bit to avoid binding the
keys repeatedly.

[1: 43e2aafae3]: 2018-01-02 20:53:42 -0500
  Don't bind dframe events on load (Bug#29599)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=43e2aafae306d9f7a463cba301d0253db846e20d