From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Morgan Willcock Newsgroups: gmane.emacs.bugs Subject: bug#73533: [PATCH] Rewrite speedbar expansion for all descendants Date: Sun, 29 Sep 2024 10:52:07 +0100 Message-ID: <87v7yeeqig.fsf@ice9.digital> References: <87y13blhu1.fsf@ice9.digital> <86a5fr5apb.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="5809"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 73533@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Sep 29 11:53:54 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1suqck-0001NE-2c for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 29 Sep 2024 11:53:54 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1suqcR-0004BN-6p; Sun, 29 Sep 2024 05:53:35 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1suqcN-0004AV-HW for bug-gnu-emacs@gnu.org; Sun, 29 Sep 2024 05:53:31 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1suqcN-0006P1-96 for bug-gnu-emacs@gnu.org; Sun, 29 Sep 2024 05:53:31 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debbugs.gnu.org; s=debbugs-gnu-org; h=MIME-Version:Date:References:In-Reply-To:From:To:Subject; bh=sQruOnqH9CM49bTgvXCsLGFFI3VJlTZaz+Z6NXMWBP0=; b=WhV2Oha+/A7zF6AG5SFkUD9+kr8UyFJNy24OlP1lVpPKdxcLQ8neEccxlq+TOuhuYfg6OFw8zSXfmZtxMw7V3MDBMp3/sbzO4KE5YO4gyX/4vTBoyOCewgS0N979oGrksR+k40OrvEgPPNX92cZ9iLhDPJGDMCAlfTmVReyJyTgclpQZ4PDViBTxJdARqv1lCebwiU8EUlYtmOr5VNyk1B10VAwu/GcG8d/dIc3mERC6SgFsS6fF1YLiz0E5cthL8F8qJ5W78EBeR18MipaeiElhor6yZuo7agZa/wKHwSalg/b+v6INBs3ZzeftySd7HjnewfCCXsNr0ULh/VJR/w==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1suqcs-0003lw-6s for bug-gnu-emacs@gnu.org; Sun, 29 Sep 2024 05:54:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Morgan Willcock Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 29 Sep 2024 09:54:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 73533 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 73533-submit@debbugs.gnu.org id=B73533.172760360114138 (code B ref 73533); Sun, 29 Sep 2024 09:54:02 +0000 Original-Received: (at 73533) by debbugs.gnu.org; 29 Sep 2024 09:53:21 +0000 Original-Received: from localhost ([127.0.0.1]:40123 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1suqcC-0003fx-Gf for submit@debbugs.gnu.org; Sun, 29 Sep 2024 05:53:21 -0400 Original-Received: from relay6-d.mail.gandi.net ([217.70.183.198]:47767) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1suqby-0003cn-Ez for 73533@debbugs.gnu.org; Sun, 29 Sep 2024 05:53:18 -0400 Original-Received: by mail.gandi.net (Postfix) with ESMTPSA id D0A77C0002; Sun, 29 Sep 2024 09:52:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ice9.digital; s=gm1; t=1727603529; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=sQruOnqH9CM49bTgvXCsLGFFI3VJlTZaz+Z6NXMWBP0=; b=PwZWxWJgub0b1PFdhB5XkDw/Z7xwCPA2CFv9KSD3ntsWvJvMrkJtTs74gWZa+v2aC1OOdo 1N/jUJ+k7O1MKK33ELyZ9IV68UuGwNeBSvmxPMDoa+KwzH57CTX9a4litO+oj4Kfj0UAs+ 9q4nR51066hj4o5acg66xC6NeT610pSgYnjNdZZTfVjV1Lt8Mx08p/229iLP2zQ4rlnLfe y8ZUabyoADZDUGGgr+H+zMNJxYUO54EOVcrhXSxVVyXI9wGE5UpCpmgdFi9FLZHEOs6erQ c9sO+sd1Zx3+UTUzn2Z101UDTzU5rKgCpV66SMzJZ5YVy8E1Pu1SgBeVzOkaKA== In-Reply-To: <86a5fr5apb.fsf@gnu.org> (Eli Zaretskii's message of "Sun, 29 Sep 2024 07:46:08 +0300") X-GND-Sasl: morgan@ice9.digital X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:292624 Archived-At: Eli Zaretskii writes: > Thanks. Unfortunately, speedbar doesn't have a test suite, so I would > like to ask how you tested this rewrite, and whether we could have > some tests for this added to the test suite. I've been testing by expanding all descendent items interactively, either with test files which generate a lot of items or within the Emacs source tree. I wouldn't be against adding tests but it doesn't look like there are many ways to query the speedbar state, which is probably why the original code is not testing whether nodes are expandable before trying to expand them. Both the original and the new code and fundamentally doing the same thing by assuming that each new line may be expanded and trying to expand it - moving "into" a sub-list is just moving forward because if expansion was possible it has occurred. I could probably craft a test which would demonstrate the old code hitting a recursion limit, but fundamentally the problem was the speed and excessive message output. > Regardless, it would be good to have both old and the new code > annotated with explanations of what each non-trivial line does, to > allow independent verification of the correctness of the rewrite by > people who are not familiar with speedbar code and don't immediately > understand the effects of a call to speedbar-naxt or > speedbar-restricted-move. speedbar-next = forward-line + display message for item + reposition cursor speedbar-restricted-move = move to the next item in the list at the current level or signal an error if the end of the list is reached speedbar-restricted-next = speedbar-restricted-move + display message for item Here is the original code my comments instead of the original comments: (defun speedbar-expand-line-descendants (&optional arg) "Expand the line under the cursor and all descendants. Optional argument ARG indicates that any cache should be flushed." (interactive "P") (save-restriction ;; The recursion of the original code means that sibling items below ;; the first item were also being accidentally expanded. Narrow the ;; buffer around the current item to prevent opening all sibling ;; items. bug#35014 (narrow-to-region (line-beginning-position) (line-beginning-position 2)) ;; Assume that the current line can be expanded. (speedbar-expand-line arg) (save-excursion ;; Move forward. Where current line did not expand this ;; effectively moves to the next sibling or the end of the buffer ;; restriction. Display a description of the newly moved to item ;; as a message. Readjust the column position of point. (speedbar-next 1) ;; Loop across all siblings until an error is signalled. (let ((err nil)) (while (not err) (condition-case nil (progn ;; Assume that the item is expandable and recursively ;; expand it. (speedbar-expand-line-descendants arg) ;; Move to the next item at the current level or signal ;; an error. Display a description of the newly moved ;; to item as a message. (speedbar-restricted-next 1)) (error (setq err t)))))))) To stop the continual stream of messages and cursor repositioning of calling speedbar-next, it can just be replaced with forward-line because that is all speedbar-next does to move. To stop the continual stream of messages from speedbar-restricted-next it can just be replaced with speedbar-restricted-move which is exactly the same thing but without the message output. So the majority of the speedup is just done by those two replacements, and all that should have changed is that now no messages are being produced. To make it non-recursive and add the status message to indicate that something is happening, what I've actually done is change the fix which was applied for bug#35014. Instead of narrowing recursively to avoid accidentally running into siblings, the narrowing is setup once by using speedbar-restricted-move to find the next sibling. The logic for expansion is effectively still the same as a depth first traversal but without the messages being generated: (defun speedbar-expand-line-descendants (&optional arg) "Expand the line under the cursor and all descendants. Optional argument ARG indicates that any cache should be flushed." (interactive "P") (dframe-message "Expanding all descendants...") (save-excursion (with-restriction ;; Narrow around the top-level item to ensure that later sibling ;; items will not be entered. (line-beginning-position) (condition-case nil (save-excursion (speedbar-restricted-move 1) ;; The line beginning position of the next sibling item. (line-beginning-position)) ;; This was the last sibling item so just apply the ;; restriction around the current item, in the same way that ;; the previous fix for bug#35014 did. (error (line-beginning-position 2))) ;; Expand every line until the end of the restriction. (while (zerop (progn ;; Assume that the line will expand and try to ;; expand it. (speedbar-expand-line arg) ;; Moving forwards will be moving into the ;; expanded lists if one opened, or moving to a ;; sibling or end of restriction if there was no ;; expansion. (forward-line 1)))))) (dframe-message "Expanding all descendants...done") ;; Reposition point to match the previous behavior. (speedbar-position-cursor-on-line)) > Also, this comment in the old code bothers me: > >> - ;; Now, inside the area expanded here, expand all subnodes of >> - ;; the same descendant type. > > What does it mean by "the same descendant type", and how does the old > and the new code make sure they expand only those descendants? I don't know what it means by "type" because the code only deals with siblings and children. I think it means depth-first expansion of all siblings. The old code pre-bug#35014 did not ensure that it only expanded descendants and just ran until the end of the buffer. The old code with the fix for bug#35014 narrowed each item recursively with the narrowing around the top level preventing a top-level sibling being reached. The new code just sets up the narrowing once around the top-level item and then effectively runs the same expansion logic but without the item look and message output. The messages were being generated by calling speedbar-item-info, and I cannot see how calling that repeatedly during expansion is required when someone can just click the nodes open with the mouse and also skip those calls. I can re-send the patch with additional comments (more similar to the above) if that helps. Thanks, Morgan -- Morgan Willcock