GNU bug report logs - #68765
30.0.50; Adding window-tool-bar package.

Previous Next

Full log

Message #43 received at 68765 <at> debbugs.gnu.org (full text, mbox):

From: Jared Finder <jared <at> finder.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 68765 <at> debbugs.gnu.org
Subject: Re: bug#68765: 30.0.50; Adding window-tool-bar package.
Date: Sat, 27 Apr 2024 21:44:32 -0700

[Message part 1 (text/plain, inline)]

On 2024-04-27 03:00, Philip Kaludercic wrote:
> Eli Zaretskii <eliz <at> 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 (text/x-diff, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.