emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [PATCH] Fixing refile cache use for org-goto in indirect buffers.
@ 2022-09-19  5:16 Yuchen Pei
  2022-09-19 15:48 ` Max Nikulin
  0 siblings, 1 reply; 5+ messages in thread
From: Yuchen Pei @ 2022-09-19  5:16 UTC (permalink / raw)
  To: emacs-orgmode mailing list

To reprod:
- make sure the org-refile-targets generates a big enough list where
  the refile cache makes a difference
- visit an org file in org-refile-targets
- M-x clone-indirect-buffer-other-window
- C-0 C-c C-w to clear cache
- M-: (org-refile-get-targets)
- org-goto in the original buffer takes no effort
- but, org-goto in the indirect buffer takes time, which is unexpected.

---
 lisp/org-refile.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/org-refile.el b/lisp/org-refile.el
index 16cff25bd..7189ef595 100644
--- a/lisp/org-refile.el
+++ b/lisp/org-refile.el
@@ -306,7 +306,10 @@ converted to a headline before refiling."
 	(dolist (f files)
 	  (with-current-buffer (if (bufferp f) f (org-get-agenda-file-buffer f))
 	    (or
-	     (setq tgs (org-refile-cache-get (buffer-file-name) descre))
+	     (setq tgs (org-refile-cache-get
+                        (buffer-file-name (when (bufferp f)
+                                            (buffer-base-buffer f)))
+                        descre))
 	     (progn
 	       (when (bufferp f)
 		 (setq f (buffer-file-name (buffer-base-buffer f))))
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fixing refile cache use for org-goto in indirect buffers.
  2022-09-19  5:16 [PATCH] Fixing refile cache use for org-goto in indirect buffers Yuchen Pei
@ 2022-09-19 15:48 ` Max Nikulin
  2022-09-20 12:44   ` Yuchen Pei
  0 siblings, 1 reply; 5+ messages in thread
From: Max Nikulin @ 2022-09-19 15:48 UTC (permalink / raw)
  To: Yuchen Pei, emacs-orgmode mailing list

On 19/09/2022 12:16, Yuchen Pei wrote:
> To reprod:
> - make sure the org-refile-targets generates a big enough list where
>    the refile cache makes a difference
> - visit an org file in org-refile-targets
> - M-x clone-indirect-buffer-other-window
> - C-0 C-c C-w to clear cache
> - M-: (org-refile-get-targets)

Have you tried to execute this command in the indirect buffer?

> - org-goto in the original buffer takes no effort
> - but, org-goto in the indirect buffer takes time, which is unexpected.

> diff --git a/lisp/org-refile.el b/lisp/org-refile.el
> index 16cff25bd..7189ef595 100644
> --- a/lisp/org-refile.el
> +++ b/lisp/org-refile.el
> @@ -306,7 +306,10 @@ converted to a headline before refiling."
>   	(dolist (f files)
>   	  (with-current-buffer (if (bufferp f) f (org-get-agenda-file-buffer f))
>   	    (or
> -	     (setq tgs (org-refile-cache-get (buffer-file-name) descre))
> +	     (setq tgs (org-refile-cache-get
> +                        (buffer-file-name (when (bufferp f)
> +                                            (buffer-base-buffer f)))
> +                        descre))

Thank you for the attempt to improve handling of indirect buffers.

I am afraid, more serious refactoring is required to reuse result of 
`buffer-base-buffer', for the previous attempt to avoid issues with 
`buffer-file-name' see
satotake to emacs-orgmode… [PATCH] org-refile.el: Fix the case of 
*scratch* buffer. Sat, 15 May 2021 19:38:39 +0900. 
https://list.orgmode.org/20210515103839.8574-2-doublequotation@gmail.com

There are several corner cases with `org-refile-cache', `org-goto', and 
buffers.
- Perhaps buffer name, not file name should be used as the cache key if 
some buffer is not associated with any file. Alternatively cache should 
not be used at all.
- When an indirect buffer is narrowed down to some region (e.g. created 
using `org-tree-to-indirect-buffer') jump targets should be filtered to 
the displayed range.

So the change is an improvement (I would prefer `and' instead of `when' 
in such expression, but it does not really matter). Leaving aside other 
issues and more serious refactoring, it seems, storing results to the 
cache requires a similar fix, so perhaps it is possible to move "(setq f 
...)" code above of "(or ...)" and reuse f as the cache key.

Please, send patches produced by "git format-patch" command.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fixing refile cache use for org-goto in indirect buffers.
  2022-09-19 15:48 ` Max Nikulin
@ 2022-09-20 12:44   ` Yuchen Pei
  2022-09-20 17:33     ` Max Nikulin
  0 siblings, 1 reply; 5+ messages in thread
From: Yuchen Pei @ 2022-09-20 12:44 UTC (permalink / raw)
  To: Max Nikulin; +Cc: emacs-orgmode mailing list

Thanks for the reply.
On Mon 2022-09-19 22:48:34 +0700, Max Nikulin wrote:

> On 19/09/2022 12:16, Yuchen Pei wrote:
>> To reprod:
>> - make sure the org-refile-targets generates a big enough list where
>>    the refile cache makes a difference
>> - visit an org file in org-refile-targets
>> - M-x clone-indirect-buffer-other-window
>> - C-0 C-c C-w to clear cache
>> - M-: (org-refile-get-targets)
>
> Have you tried to execute this command in the indirect buffer?

Yes, and it would be instant (assuming (org-refile-get-targets) has been
run in the original buffer).  This is because the code path from
org-goto has an overriding local binding of org-refile-targets to `((nil
. (:maxlevel . ,org-goto-max-level)) before calling
org-refile-get-location which calls org-refile-get-targets.

>
>> - org-goto in the original buffer takes no effort
>> - but, org-goto in the indirect buffer takes time, which is unexpected.
>
>> diff --git a/lisp/org-refile.el b/lisp/org-refile.el
>> index 16cff25bd..7189ef595 100644
>> --- a/lisp/org-refile.el
>> +++ b/lisp/org-refile.el
>> @@ -306,7 +306,10 @@ converted to a headline before refiling."
>>   	(dolist (f files)
>>   	  (with-current-buffer (if (bufferp f) f (org-get-agenda-file-buffer f))
>>   	    (or
>> -	     (setq tgs (org-refile-cache-get (buffer-file-name) descre))
>> +	     (setq tgs (org-refile-cache-get
>> +                        (buffer-file-name (when (bufferp f)
>> +                                            (buffer-base-buffer f)))
>> +                        descre))
>
> Thank you for the attempt to improve handling of indirect buffers.
>
> I am afraid, more serious refactoring is required to reuse result of
> `buffer-base-buffer', for the previous attempt to avoid issues with
> `buffer-file-name' see
> satotake to emacs-orgmode… [PATCH] org-refile.el: Fix the case of
> *scratch* buffer. Sat, 15 May 2021 19:38:39
> +0900. https://list.orgmode.org/20210515103839.8574-2-doublequotation@gmail.com
>
> There are several corner cases with `org-refile-cache', `org-goto',
> and buffers.
> - Perhaps buffer name, not file name should be used as the cache key
>   if some buffer is not associated with any file. Alternatively cache
>   should not be used at all.

It seems there may be problem with this idea.  If buffer name is the
key, that will mean a buffer and its indirect clone will generate two
caches, which duplicates the work, no?

> - When an indirect buffer is narrowed down to some region
>   (e.g. created using `org-tree-to-indirect-buffer') jump targets
>   should be filtered to the displayed range.
>
> So the change is an improvement (I would prefer `and' instead of
> `when' in such expression, but it does not really matter). Leaving
> aside other issues and more serious refactoring, it seems, storing
> results to the cache requires a similar fix, so perhaps it is possible
> to move "(setq f ...)" code above of "(or ...)" and reuse f as the
> cache key.

Sure, I will update my patch when I get around to it again.

>
> Please, send patches produced by "git format-patch" command.
>

I guess you had some problem applying the patch?  I did use
(ma)git-format-patch, but had some difficulty of getting gnus to send
the formatted patch (I basically visited the patch file and ran
(message-mode), but it was missing a few header fields compared to
composing a new mail from gnus, so I manually copied over these fields).
This was why I sent two emails.  The first one[1] had an odd extra bit
of Message-ID at the bottom of the message.  Is this the one you were
referring to as not being "standard", or is the other one[2] also not
quite right?

[1] https://lists.gnu.org/archive/html/emacs-orgmode/2022-09/msg00322.html
[2] https://lists.gnu.org/archive/html/emacs-orgmode/2022-09/msg00323.html

Best,
Yuchen

-- 
PGP Key: 47F9 D050 1E11 8879 9040  4941 2126 7E93 EF86 DFD0
          <https://ypei.org/assets/ypei-pubkey.txt>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fixing refile cache use for org-goto in indirect buffers.
  2022-09-20 12:44   ` Yuchen Pei
@ 2022-09-20 17:33     ` Max Nikulin
  0 siblings, 0 replies; 5+ messages in thread
From: Max Nikulin @ 2022-09-20 17:33 UTC (permalink / raw)
  To: Yuchen Pei; +Cc: emacs-orgmode mailing list

On 20/09/2022 19:44, Yuchen Pei wrote:
> Thanks for the reply.
> On Mon 2022-09-19 22:48:34 +0700, Max Nikulin wrote:
> 
>> On 19/09/2022 12:16, Yuchen Pei wrote:
>>> To reprod:
>>> - make sure the org-refile-targets generates a big enough list where
>>>     the refile cache makes a difference
>>> - visit an org file in org-refile-targets
>>> - M-x clone-indirect-buffer-other-window
>>> - C-0 C-c C-w to clear cache
>>> - M-: (org-refile-get-targets)
>>
>> Have you tried to execute this command in the indirect buffer?
> 
> Yes, and it would be instant (assuming (org-refile-get-targets) has been
> run in the original buffer).

My hypothesis is that if refile targets are requested from an indirect 
buffer at first then next attempt from the base buffer causes 
recalculation because of previous result is not properly stored. The 
code at the end of the function requires a similar fix.

>>> diff --git a/lisp/org-refile.el b/lisp/org-refile.el
>>> index 16cff25bd..7189ef595 100644
>>> --- a/lisp/org-refile.el
>>> +++ b/lisp/org-refile.el
>>> @@ -306,7 +306,10 @@ converted to a headline before refiling."
>>>    	(dolist (f files)
>>>    	  (with-current-buffer (if (bufferp f) f (org-get-agenda-file-buffer f))
>>>    	    (or
>>> -	     (setq tgs (org-refile-cache-get (buffer-file-name) descre))
>>> +	     (setq tgs (org-refile-cache-get
>>> +                        (buffer-file-name (when (bufferp f)
>>> +                                            (buffer-base-buffer f)))
>>> +                        descre))
>>
>> Thank you for the attempt to improve handling of indirect buffers.
>>
>> I am afraid, more serious refactoring is required to reuse result of
>> `buffer-base-buffer', for the previous attempt to avoid issues with
>> `buffer-file-name' see
>> satotake to emacs-orgmode… [PATCH] org-refile.el: Fix the case of
>> *scratch* buffer. Sat, 15 May 2021 19:38:39
>> +0900. https://list.orgmode.org/20210515103839.8574-2-doublequotation@gmail.com
>>
>> There are several corner cases with `org-refile-cache', `org-goto',
>> and buffers.
>> - Perhaps buffer name, not file name should be used as the cache key
>>    if some buffer is not associated with any file. Alternatively cache
>>    should not be used at all.
> 
> It seems there may be problem with this idea.  If buffer name is the
> key, that will mean a buffer and its indirect clone will generate two
> caches, which duplicates the work, no?

Certainly if a buffer has its base buffer then file name associated with 
the base buffer should be tried at first. However besides indirect 
buffers, buffers not associated with any files at all may exist. I am 
not sure that using buffer name is a good idea for them since temporary 
buffers may be discarded and new ones with same names may be created. My 
point is that refile targets for temporary buffers should not be cached 
at all or buffer name should be used as the key. Behavior should be 
deterministic. I do not request such change from you, but maybe it is 
just an additional couple of lines at the location you are changing anyway.

>> So the change is an improvement (I would prefer `and' instead of
>> `when' in such expression, but it does not really matter). Leaving
>> aside other issues and more serious refactoring, it seems, storing
>> results to the cache requires a similar fix, so perhaps it is possible
>> to move "(setq f ...)" code above of "(or ...)" and reuse f as the
>> cache key.
> 
> Sure, I will update my patch when I get around to it again.

Great!

>> Please, send patches produced by "git format-patch" command.
> 
> I guess you had some problem applying the patch?

No, I missed commit message in the expected format
https://orgmode.org/worg/org-contribute.html#commit-messages
Just send the patch as an attachment.



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] Fixing refile cache use for org-goto in indirect buffers.
@ 2022-09-19  5:16 Yuchen Pei
  0 siblings, 0 replies; 5+ messages in thread
From: Yuchen Pei @ 2022-09-19  5:16 UTC (permalink / raw)
  To: emacs-orgmode mailing list

To reprod:
- make sure the org-refile-targets generates a big enough list where
  the refile cache makes a difference
- visit an org file in org-refile-targets
- M-x clone-indirect-buffer-other-window
- C-0 C-c C-w to clear cache
- M-: (org-refile-get-targets)
- org-goto in the original buffer takes no effort
- but, org-goto in the indirect buffer takes time, which is unexpected.

---
 lisp/org-refile.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/org-refile.el b/lisp/org-refile.el
index 16cff25bd..7189ef595 100644
--- a/lisp/org-refile.el
+++ b/lisp/org-refile.el
@@ -306,7 +306,10 @@ converted to a headline before refiling."
 	(dolist (f files)
 	  (with-current-buffer (if (bufferp f) f (org-get-agenda-file-buffer f))
 	    (or
-	     (setq tgs (org-refile-cache-get (buffer-file-name) descre))
+	     (setq tgs (org-refile-cache-get
+                        (buffer-file-name (when (bufferp f)
+                                            (buffer-base-buffer f)))
+                        descre))
 	     (progn
 	       (when (bufferp f)
 		 (setq f (buffer-file-name (buffer-base-buffer f))))
-- 
2.37.3

Message-ID: <877d20vscj.fsf@ypei.org>


^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-09-20 23:26 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-19  5:16 [PATCH] Fixing refile cache use for org-goto in indirect buffers Yuchen Pei
2022-09-19 15:48 ` Max Nikulin
2022-09-20 12:44   ` Yuchen Pei
2022-09-20 17:33     ` Max Nikulin
2022-09-19  5:16 Yuchen Pei

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).