[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#68765: 30.0.50; Adding window-tool-bar package.
From: |
Jared Finder |
Subject: |
bug#68765: 30.0.50; Adding window-tool-bar package. |
Date: |
Sat, 27 Apr 2024 21:44:32 -0700 |
On 2024-04-27 03:00, Philip Kaludercic wrote:
Eli Zaretskii <eliz@gnu.org> writes:
Ping! Ping! Philip and Jared, are there any issues left to resolve
here, or should this be installed?
Sorry for the delay, I'm going to comment below.
I addressed all feedback provided. Updated patch for just 0003 attached
since that's where all the feedback was. Let me know if this looks
good.
... partial patch elided ...
+(defun window-tool-bar-debug-show-memory-use ()
+ "Development-only command to show memory used by
`window-tool-bar-string'."
+ (interactive)
+ (require 'time-stamp)
+ (save-selected-window
+ (pop-to-buffer "*WTB Memory Report*")
+ (unless (eq major-mode 'special-mode)
You should explain what is going on here, and why you are checking
major-mode instead of using derived-mode-p.
I changed this to call derived-mode-p. The intent is to put the buffer
into special-mode and also avoid unnecessarily calling special-mode.
+ (special-mode))
+
... partial patch elided ...
+ (insert (format "Refresh count %d\n"
window-tool-bar--refresh-done-count)
+ (format "Refresh executed percent %.2f\n"
+ (/ window-tool-bar--refresh-done-count
+ (+ window-tool-bar--refresh-done-count
+ window-tool-bar--refresh-skipped-count)
+ 1.0))
I don't know if there is any significant difference between (/ a b 1.0)
and (/ a (float b)), but interesting they have the same number of
bytecode instructions and funcalls:
I changed this and other places where I was dividing by 1.0 to force
floating point division to instead do (float a) on the first parameter.
It sounds like that's more idiomatic.
+ "\n"))))
+
+(defun window-tool-bar--insert-memory-use (label avg-memory-use)
+ "Insert memory use into current buffer.
+
+LABEL: A prefix string to be in front of the data.
+AVG-MEMORY-USE: A list of averages, with the same meaning as
+ `memory-use-counts'."
The formatting is somewhat unconventional and can easily be broken
using M-q.
Addressed here and other places I used this convention.
+(defun window-tool-bar--ignore ()
+ "Do nothing. This command exists for isearch."
Can you elaborate? Why not just use the existing ignore? Or
defaliasing it?
There's a lot of detailed comments around usage of this command, see
comments around window-tool-bar--button-keymap. I also added a bit more
context to the docstring here. In brief, this command exists so that
isearch does not exit when you click on isearch tool bar buttons. This
is needed for window tool bar buttons since they are called by Emacs'
usual mouse event bindings, unlike toolkit tool bars.
I can't use ignore because it does not have the special registration
with isearch, specifically ignore is not a member of
isearch-menu-bar-commands. I did not feel safe changing all existing
uses of ignore.
+ (let ((type (event-basic-type last-command-event)))
... partial patch elided ...
+;;;###autoload
+(define-minor-mode window-tool-bar-mode
+ "Toggle display of the tool bar in the tab line of the current
buffer."
+ :lighter nil
There is no lighter by default, I prefer writing :global nil, to make
it
explicit to the reader that this is a local minor mode.
Thanks, changed.
Can you update define-minor-mode's docstring to guide others in the
right direction? I only passed :lighter nil because that was the
example given by define-minor-mode's docstring, "If you provide BODY,
then you must provide at least one keyword argument (e.g. `:lighter
nil')".
+ (let ((should-display (and window-tool-bar-mode
... partial patch elided ...
+;;; window-tool-bar.el ends here
Hope this was of use.
Thank you for the thorough review!
-- MJF
0003-Adding-window-tool-bar-package.patch
Description: Text Data