Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closing window by C-g doesn't run post-command-hook #52

Open
iRi-E opened this issue Feb 1, 2012 · 12 comments
Open

Closing window by C-g doesn't run post-command-hook #52

iRi-E opened this issue Feb 1, 2012 · 12 comments

Comments

@iRi-E
Copy link
Collaborator

iRi-E commented Feb 1, 2012

Programs which invoke some function after selecting another window may not work correctly if they use post-command-hook. The problem actually happens to ibus.el because the change of input focus cannot be detected.

I think popwin:close-popup-window-if-necessary() should run post-command-hook when closing the popup window and selecting another one.

@m2ym
Copy link
Contributor

m2ym commented Feb 1, 2012

How about if there is a hook something like popwin:after-close-hook?

@iRi-E iRi-E closed this as completed Feb 2, 2012
@iRi-E iRi-E reopened this Feb 2, 2012
@iRi-E
Copy link
Collaborator Author

iRi-E commented Feb 2, 2012

Sorry, I have accidentally closed this issue... (´・ω・`)
Now reopened. (`・ω・´)

I guess most of all people using both ibus.el and popwin.el encounter the same problem.

Normally, moving to another window should happen only in interactive commands, otherwise users are confused by unexpected behavior, so AFAIK most programs (including ibus.el) don't assume that window selection is changed by a timer.

Currently I'm avoiding this problem by the following code in my .emacs file:

(defadvice popwin:close-popup-window-timer
  (around popwin:run-post-command-hook () activate)
  (let ((buffer (window-buffer)))
    ad-do-it
    (unless (eq (window-buffer) buffer)
      (run-hooks 'post-command-hook))))

ibus.el already includes several workarounds like this for the other programs, but I no longer want to add more dirty workarounds to ibus.el.

I dislike popwin:after-close-hook because all users of ibus.el still need workaround such as:

(add-hook 'popwin:after-close-hook (lambda () (run-hooks 'post-command-hook)))

or

(add-hook 'popwin:after-close-hook 'ibus-check-current-buffer)

@m2ym
Copy link
Contributor

m2ym commented Feb 2, 2012

How about this patch? https://gist.github.com/1721707

This enables you to detect popup windows closed via pre-command-hook/post-command-hook with this-command being popwin:popup-window-closed.

@iRi-E
Copy link
Collaborator Author

iRi-E commented Feb 2, 2012

Thanks, but unfortunately your patch doesn't work because post-command-hook runs before closing the popup window. post-command-hook must run at the last!

And I think your implementation is too complicated. Probably using call-interactive() makes no sense.

The behavior you intend can be done by enqueuing a fake event like 'popwin:closed' to unread-command-events to invoke the fake command, but this way might fail if keymap is overridden by some other programs.

@m2ym
Copy link
Contributor

m2ym commented Feb 2, 2012

As you may know, there is no robust way to fake a command execution. To me, defining keymap for command-loop to translate fake events to actual commands seems worse rather than calling post-command-hook explicitly. Anyway, I'll fix the patch.

@m2ym
Copy link
Contributor

m2ym commented Feb 2, 2012

Updated the patch. Please check it out again.

@iRi-E
Copy link
Collaborator Author

iRi-E commented Feb 2, 2012

I tested the new patch. So far it works fine.

@m2ym
Copy link
Contributor

m2ym commented Feb 2, 2012

Merged. Thanks for reporting.

@m2ym m2ym closed this as completed Feb 2, 2012
@iRi-E
Copy link
Collaborator Author

iRi-E commented Feb 5, 2012

Currently popwin:close-popup-window-if-necessary() runs post-command-hook directly to fake an interactive command execution for all situations listed below:

  1. `C-g' has been pressed.
  2. The popup buffer has been killed.
  3. The popup buffer has been buried.
  4. The popup buffer has been changed if the popup window is dedicated to the buffer.
  5. Another window has been selected.

However, I confirmed this code causes some problems if an actual command or post-command-hook is already running and is waiting for outputs from external processes, because accept-process-output() cannot inhibit any timers during its waiting, so the fake post-command-hook might run unexpectedly in such function.

Furthermore, each of the conditions 2-5 runs post-command-hook by itself.

And the following code must run post-command-hook twice:

(run-hooks 'post-command-hook)
(execute-kbd-macro [])

because execute-kbd-macro() also runs post-command-hook at the start of its fake command loop.

I think popwin:close-popup-window-if-necessary() should be modified as follows:

  • Check if C-g was pressed
  • Check if actual command or hook is not running
  • Remove "(execute-kbd-macro [])"

Please check my patch 46994b4.

@m2ym
Copy link
Contributor

m2ym commented Feb 6, 2012

Merged!

@iRi-E
Copy link
Collaborator Author

iRi-E commented Feb 8, 2012

Hmm... The problems still happen to some programs that also use timer.

So I tried to implementing another solution using minor mode. So far this solution works fine for me.
Can you test my patch ba7a1a0 ?

@m2ym m2ym reopened this Feb 8, 2012
@iRi-E
Copy link
Collaborator Author

iRi-E commented Feb 8, 2012

I found that the previous patch 46994b4 still cannot run post-command-hook in some cases.

Steps to reproduce:

  • Type M-x, and then the selection moves to the minibuffer window
  • Select another window by C-x o (Don't finish the input)
  • Open the popup window, and type C-g to exit it

In this case, this-command doesn't become nil even if the timer's function is called from command loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants