Re: [PATCH] (CEDET development) Improve the bovinate output buffer

[Fermin <fmfs@posteo.net>]

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

Thanks for the feedback!

> AFAICT this is better served by `lisp-data-mode` than `emacs-lisp-mode`, no?
> Also, try to make the first line of docstrings be self-contained.
> Maybe you could just use `lisp-data-mode` without bothering to
> define a new major mode for it.

I couldn't find the lisp-data-mod because I'm developing in the Emacs 
27.1 , I did have to change my configuration
so now I can use the build from master.

The idea of the major mode is that I want to add some interactive 
behavior, e.g improve the tag navigation with buttons
, so a new major mode can help in that task.

I did change the function from defun to cl-defun, so I can set the 
default value of display to true, the idea of non displaying the
source code file is for testing mainly.

> This would better be folded into the previous patch.
Done, I think I address most of the issues, sorry for the inconvenience, 
I'm not use to work with mailing list development.

I attach a patch with the changes.

Regards


On 02/04/2021 00:02, Stefan Monnier wrote:
>>  From f8c7ee1791ec6eee1993a9e5449112579e193415 Mon Sep 17 00:00:00 2001
>> From: Fermin <fmfs@posteo.net>
>> Date: Thu, 1 Apr 2021 21:31:56 +0200
>> Subject: [PATCH] Add bovinate-mode as a major mode and improve the bovinate
>>   command
>>
>> ---
> Could you expand this commit message a bit and clarify on the first line
> that this regards some part of semantic?  E.g.:
>
>      Subject: [PATCH] semantic.el (bovinate): Prettify output
>
>      * lisp/cedet/semantic.el (bovinate-mode): New major mode.
>      (bovinate): ???.
>
> where the `???` should say what kind of improvement is brought by the
> patch (like "Give a more meaningful name to the buffer"?).
>
>> +(define-derived-mode bovinate-mode emacs-lisp-mode "Bovinate"
>> +  "Bovinate buffer major-mode, giving that the list is generated with
>> +`pp-to-string', the syntax is very similar to a list of Emacs Lisp.
>> +This major mode helps with the syntax highlight of some of the symbols.")
> AFAICT this is better served by `lisp-data-mode` than `emacs-lisp-mode`, no?
> Also, try to make the first line of docstrings be self-contained.
> Maybe you could just use `lisp-data-mode` without bothering to
> define a new major mode for it.
>
>>   (defun bovinate (&optional clear)
>>     "Parse the current buffer.  Show output in a temp buffer.
>>   Optional argument CLEAR will clear the cache before parsing.
>>   If CLEAR is negative, it will do a full reparse, and also display
>>   the output buffer."
>>     (interactive "P")
>> -  (if clear (semantic-clear-toplevel-cache))
>> -  (if (eq clear '-) (setq clear -1))
>> +  (when clear (semantic-clear-toplevel-cache))
>>     (let* ((start (current-time))
>> -	 (out (semantic-fetch-tags)))
>> -    (message "Retrieving tags took %.2f seconds."
>> -	     (semantic-elapsed-time start nil))
>> -    (when (or (null clear) (not (listp clear))
>> -	      (and (numberp clear) (< 0 clear)))
>> -      (pop-to-buffer "*Parser Output*")
>> -      (require 'pp)
>> +	 (tags (semantic-fetch-tags))
>> +         (time-elapse (semantic-elapsed-time start nil))
>> +         (bovinate-buffer
>> +          (format "*Parser Output from %s buffer*" (buffer-name))))
>> +    (message "Retrieving tags took %.2f seconds." time-elapse)
>> +    (unless (get-buffer bovinate-buffer)
>> +      (setq bovinate-buffer (get-buffer-create bovinate-buffer)))
>> +    (with-current-buffer bovinate-buffer
> I think you can simplify the last three lines to
>
>         (with-current-buffer (get-buffer-create bovinate-buffer)
>
>>         (erase-buffer)
>> -      (insert (pp-to-string out))
>> +      (insert (pp-to-string tags))
>> +      (bovinate-mode)
>>         (goto-char (point-min)))))
> I don't see where the new code implements the "negative CLEAR" behavior.
> Also I don't see in the new code where/when the buffer is displayed.
> [ Read on before replying ;-)  ]
>
>>  From 1c1673283186780e6db007f79d4f8aa864660f79 Mon Sep 17 00:00:00 2001
>> From: Fermin <fmfs@posteo.net>
>> Date: Thu, 1 Apr 2021 21:53:16 +0200
>> Subject: [PATCH] Add display as a optional parameter for displaying the buffer
>>
>> ---
> Similar comment abut the commit message.
>
>>   (define-derived-mode bovinate-mode emacs-lisp-mode "Bovinate"
>>     "Bovinate buffer major-mode, giving that the list is generated with
>> -`pp-to-string', the syntax is very similar to a list of Emacs Lisp.
>> +`pp-to-string', the syntax is very similar to a list in Emacs Lisp.
>>   This major mode helps with the syntax highlight of some of the symbols.")
> This would better be folded into the previous patch.
>
>> -(defun bovinate (&optional clear)
>> -  "Parse the current buffer.  Show output in a temp buffer.
>> +(defun bovinate (&optional clear display)
>> +  "Parse the current buffer.  Show output in a `bovinate-mode' buffer.
>>   Optional argument CLEAR will clear the cache before parsing.
>> -If CLEAR is negative, it will do a full reparse, and also display
>> -the output buffer."
>> +If non-nil DISPLAY will display the buffer `pop-to-buffer'."
>>     (interactive "P")
> This means that when used interactively DISPLAY will always be nil and
> hence the buffer will never be "popped".  This contradicts the
> first line's "Show output in a `bovinate-mode' buffer" and I also wonder
> what is the benefit (is it often useful/necessary to do
> `M-x bovinate` without wanting to see the resulting tags)?
>
>>     (when clear (semantic-clear-toplevel-cache))
>>     (let* ((start (current-time))
>>   	 (tags (semantic-fetch-tags))
>>            (time-elapse (semantic-elapsed-time start nil))
>>            (bovinate-buffer
>> -          (format "*Parser Output from %s buffer*" (buffer-name))))
>> +          (format "*Parser Output from %s*" (buffer-name))))
> This should also be folded into the previous patch.
>
>>       (message "Retrieving tags took %.2f seconds." time-elapse)
>>       (unless (get-buffer bovinate-buffer)
>>         (setq bovinate-buffer (get-buffer-create bovinate-buffer)))
>> @@ -359,7 +358,8 @@ the output buffer."
>>         (erase-buffer)
>>         (insert (pp-to-string tags))
>>         (bovinate-mode)
>> -      (goto-char (point-min)))))
>> +      (goto-char (point-min)))
>> +    (when display (pop-to-buffer bovinate-buffer))))
> Hmm... overall, I think the two patches should be squashed together.
>
>
>          Stefan
>

[-- Attachment #2: bovinate.patch --]
[-- Type: text/x-patch, Size: 1857 bytes --]

diff --git a/lisp/cedet/semantic.el b/lisp/cedet/semantic.el
index fb443fa4a3..a017caa7ff 100644
--- a/lisp/cedet/semantic.el
+++ b/lisp/cedet/semantic.el
@@ -335,25 +335,28 @@ semantic-elapsed-time
 Arguments START and END bound the time being calculated."
   (float-time (time-subtract end start)))
 
-(defun bovinate (&optional clear)
+(define-derived-mode bovinate-mode lisp-data-mode "Bovinate"
+  "Bovinate buffer major-mode.")
+
+(cl-defun bovinate (&optional clear (display t))
   "Parse the current buffer.  Show output in a temp buffer.
-Optional argument CLEAR will clear the cache before parsing.
-If CLEAR is negative, it will do a full reparse, and also display
-the output buffer."
+If CLEAR is non-nil, it will do a full reparse.
+If DISPLAY is nil, it doesn't show the parser buffer."
   (interactive "P")
-  (if clear (semantic-clear-toplevel-cache))
-  (if (eq clear '-) (setq clear -1))
+  (when clear (semantic-clear-toplevel-cache))
   (let* ((start (current-time))
-	 (out (semantic-fetch-tags)))
-    (message "Retrieving tags took %.2f seconds."
-	     (semantic-elapsed-time start nil))
-    (when (or (null clear) (not (listp clear))
-	      (and (numberp clear) (< 0 clear)))
-      (pop-to-buffer "*Parser Output*")
-      (require 'pp)
+	 (tags (semantic-fetch-tags))
+         (time-elapse (semantic-elapsed-time start nil))
+         (bovinate-buffer
+          (format "*Parser Output from %s*" (buffer-name))))
+    (message "Retrieving tags took %.2f seconds." time-elapse)
+    (with-current-buffer (get-buffer-create bovinate-buffer)
       (erase-buffer)
-      (insert (pp-to-string out))
-      (goto-char (point-min)))))
+      (insert (pp-to-string tags))
+      (bovinate-mode)
+      (goto-char (point-min)))
+    (when display (pop-to-buffer bovinate-buffer))))
+
 \f
 ;;; Functions of the parser plug-in API
 ;;