Honor ignored prefixes when clearing overlay (fixes #434) #435
Conversation
Eask
Outdated
| (package-file "copilot.el") | ||
|
|
||
| (script "test" "echo \"Error: no test specified\" && exit 1") | ||
| (script "test" "emacs -Q --batch -L . -l ert -l test/copilot-overlay-tests.el -f ert-run-tests-batch-and-exit") |
There was a problem hiding this comment.
Probably we should make this generic and run all test files. (even if there's only one today)
There was a problem hiding this comment.
Copy, will update later
copilot.el
Outdated
| (or | ||
| (string-prefix-p "copilot-" (symbol-name this-command)) | ||
| (member this-command copilot-clear-overlay-ignore-commands) | ||
| (let ((key-command (key-binding (this-command-keys-vector) t))) |
There was a problem hiding this comment.
Might be a good idea to add some comment to this bit of code, as I assume its purpose won't be obvious to many people.
test/copilot-overlay-tests.el
Outdated
| (require 'cl-lib) | ||
| (require 'ert) | ||
|
|
||
| (unless (require 'f nil 'noerror) |
There was a problem hiding this comment.
What's the point of those conditional requires/provides? To be able to run the tests without the deps present or something else?
There was a problem hiding this comment.
The point here was just to make the tests work when run directly via Emacs -Q in batch mode. Open to suggestions here
* copilot.el (copilot--post-command): Consider 'this-original-command' when checking whether not to clear the overlay. Fixes copilot-emacs#434: prefix commands like C-u were clearing the Copilot overlay even when their command symbols (e.g., 'universal-argument) were listed in 'copilot-clear-overlay-ignore-commands'.
ultronozm
force-pushed
the
fix/overlay-ignore
branch
from
9b841c3 to
5c3211e
Compare
|
I pushed an update to the PR that fixes the issue in a slightly more direct way. I also deleted the tests from the PR -- they were testing the mechanism used to implement the fix rather than the symptom itself, and so did not really serve their intended purpose. |
|
The new fix looks nicer indeed! Thanks! I agree we're probably better off without the tests. |
|
An addendum: I noticed that the README states, regarding
Perhaps - to keep the README accurate without requiring users to customize anything -- it would be worth making that the default value, or hard-coding those commands to be ignored (e.g., as a separate |
|
That would be fine by me. |
2 participants
Prefix commands like C-u were clearing the Copilot overlay even when
their command symbols (e.g., 'universal-argument) were listed in
'copilot-clear-overlay-ignore-commands'.
copilot.el (copilot--post-command): Check both 'this-command and the
command bound to the current key sequence.
test/copilot-overlay-tests.el
(copilot-post-command-honors-key-binding-ignore)
(copilot-post-command-clears-when-not-ignored): New tests.
Eask: Add script entry to run the tests.