[BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Jens Schmidt]

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

Tags: patch

`org-store-link' has a number of related issues when storing links from
article buffers related to nnvirtual, nnselect, or nnir groups.  I
describe them here in prose without providing a full repro case, which
would be somewhat difficult to set up.  Just let me know if you think
you need more information, I have the data available.

The most obvious symptom is this:

- Create an nnselect group and open an article from that.  In the
   article buffer, do M-x org-store-link RET, then paste the link with
   C-c C-l in some Org mode buffer.  The resulting link looks like

     gnus:#E18xcfu-0004HT-00@fencepost.gnu.org

   That is, it lacks the group name before the hash sign.  Correct would
   have been:

     gnus:nnml+archive:test01#E18xcfu-0004HT-00@fencepost.gnu.org

   Starting with Emacs 30, you even more obviously get an error:

     Debugger entered--Lisp error: (wrong-type-argument 
number-or-marker-p nil)
       nnselect-article-group(nil)
       org-gnus-store-link()
       org-store-link(nil 1)
       funcall-interactively(org-store-link nil 1)
       call-interactively(org-store-link record nil)
       command-execute(org-store-link record)
       execute-extended-command(nil "org-store-link" nil)
       funcall-interactively(execute-extended-command nil 
"org-store-link" nil)
       call-interactively(execute-extended-command nil nil)
       command-execute(execute-extended-command)

Less obvious, occuring for nnvirtual groups:

- Create an nnvirtual group and open an article from that.  In the
   article buffer, do M-x org-store-link RET.  Observe the "current
   article arrow" in the fringe being set in the article header, even
   though that arrow should be used only in a summary buffer.

The root cause is that some of the Gnus functions used in
`org-gnus-store-link' must be called only in summary buffers, and not in
article buffers.  These are:

   gnus-summary-article-number
   nnselect-article-group

Not sure about these, but it is probably also better to call these in
summary buffers only:

   nnvirtual-map-article
   nnir-article-group

The remedy for these issues is simple: When calling above functions just
temporarily and unconditionally switch to the summary buffer with

   (with-current-buffer gnus-summary-buffer ...)

where buffer-local variable `gnus-summary-buffer' in an article buffer
points to the summary buffer where the articles comes from.  (And for
a summary buffer the variable points to the summary buffer itself.)

Finally, there is a related inefficiency when determining the article
header structure in function `org-gnus-store-link': Here the authors
indeed switch to the summary buffer when currently in the article
buffer, but using "user-land" interactive function
`gnus-article-show-summary' to do so where a simple

   (with-current-buffer gnus-summary-buffer ...)

would suffice.

Emacs  : GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 
3.24.24, cairo version 1.16.0)
  of 2023-07-20
Package: Org mode version 9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ 
/home/jschmidt/work/org-mode/lisp/)

[-- Attachment #2: 0001-ol-gnus.el-Fix-issue-when-storing-links-from-Gnus-ar.patch --]
[-- Type: text/x-patch, Size: 2451 bytes --]

From e1bc9aefd4fd0080012c172d1c21c684a5b2fe51 Mon Sep 17 00:00:00 2001
From: farblos <43711228+farblos@users.noreply.github.com>
Date: Sat, 22 Jul 2023 10:30:19 +0200
Subject: [PATCH] ol-gnus.el: Fix issue when storing links from Gnus article
 buffers

* lisp/ol-gnus.el (org-gnus-store-link): Switch to
`gnus-summary-buffer' when calling functions that are intended to be
called only there.
---
 lisp/ol-gnus.el | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/lisp/ol-gnus.el b/lisp/ol-gnus.el
index 7c07ce045..f0e04ce66 100644
--- a/lisp/ol-gnus.el
+++ b/lisp/ol-gnus.el
@@ -137,27 +137,23 @@ If `org-store-link' was called with a prefix arg the meaning of
      (let* ((group
 	     (pcase (gnus-find-method-for-group gnus-newsgroup-name)
 	       (`(nnvirtual . ,_)
-		(save-excursion
-		  (car (nnvirtual-map-article (gnus-summary-article-number)))))
+		(with-current-buffer gnus-summary-buffer
+		  (save-excursion
+		    (car (nnvirtual-map-article (gnus-summary-article-number))))))
 	       (`(,(or `nnselect `nnir) . ,_)  ; nnir is for Emacs < 28.
-		(save-excursion
-		  (cond
-		   ((fboundp 'nnselect-article-group)
-		    (nnselect-article-group (gnus-summary-article-number)))
-		   ((fboundp 'nnir-article-group)
-		    (nnir-article-group (gnus-summary-article-number)))
-		   (t
-		    (error "No article-group variant bound")))))
+		(with-current-buffer gnus-summary-buffer
+		  (save-excursion
+		    (cond
+		     ((fboundp 'nnselect-article-group)
+		      (nnselect-article-group (gnus-summary-article-number)))
+		     ((fboundp 'nnir-article-group)
+		      (nnir-article-group (gnus-summary-article-number)))
+		     (t
+		      (error "No article-group variant bound"))))))
 	       (_ gnus-newsgroup-name)))
-	    (header (if (eq major-mode 'gnus-article-mode)
-			;; When in an article, first move to summary
-			;; buffer, with point on the summary of the
-			;; current article before extracting headers.
-			(save-window-excursion
-			  (save-excursion
-			    (gnus-article-show-summary)
-			    (gnus-summary-article-header)))
-		      (gnus-summary-article-header)))
+	    (header (with-current-buffer gnus-summary-buffer
+		      (save-excursion
+			(gnus-summary-article-header))))
 	    (from (mail-header-from header))
 	    (message-id (org-unbracket-string "<" ">" (mail-header-id header)))
 	    (date (org-trim (mail-header-date header)))
-- 
2.30.2

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Ihor Radchenko]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> The most obvious symptom is this:
>
> - Create an nnselect group and open an article from that.  In the
>    article buffer, do M-x org-store-link RET, then paste the link with
>    C-c C-l in some Org mode buffer.  The resulting link looks like
>
>      gnus:#E18xcfu-0004HT-00@fencepost.gnu.org
>
>    That is, it lacks the group name before the hash sign.  Correct would
>    have been:
>
>      gnus:nnml+archive:test01#E18xcfu-0004HT-00@fencepost.gnu.org
>
>    Starting with Emacs 30, you even more obviously get an error:
>
>      Debugger entered--Lisp error: (wrong-type-argument 
> number-or-marker-p nil)

Thanks for reporting!
ol-gnus is not very actively maintained, so there be dragons.

> The root cause is that some of the Gnus functions used in
> `org-gnus-store-link' must be called only in summary buffers, and not in
> article buffers.  These are:
>
>    gnus-summary-article-number
>    nnselect-article-group
>
> Not sure about these, but it is probably also better to call these in
> summary buffers only:
>
>    nnvirtual-map-article
>    nnir-article-group
>
> The remedy for these issues is simple: When calling above functions just
> temporarily and unconditionally switch to the summary buffer with
>
>    (with-current-buffer gnus-summary-buffer ...)
>
> where buffer-local variable `gnus-summary-buffer' in an article buffer
> points to the summary buffer where the articles comes from.  (And for
> a summary buffer the variable points to the summary buffer itself.)

I am not familiar with Gnus, but looking at the code, may it be that a
Gnus article is open when Gnus summary buffer is not?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Jens Schmidt]

On 2023-07-22  15:48, Ihor Radchenko wrote:

> I am not familiar with Gnus, but looking at the code, may it be that
> a Gnus article is open when Gnus summary buffer is not?

Theoretically yes, if you actively and malignantly kill the summary 
buffer, for example.  In practice and through Gnus key bindings, this
should not happen.  IOW, Gnus stops behaving reasonably as well if you
kill the summary buffer other than through Gnus key bindings ("Selecting 
deleted buffer", etc.).

If you check function `gnus-summary-work-articles' from gnus-sum.el, the
main work horse for article processing and also good for calling in
article buffers, you will note (focusing on the default case) the same
paradigm

   (with-current-buffer gnus-summary-buffer
     (cond
      [...]
      (t
       ;; Just return the current article.
       (list (gnus-summary-article-number))))))

As a last resort we could also try to drag in Andrew Cohen as a
reviewer, he has been helpful already with one or two of my Gnus bugs.

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Eric Abrahamsen]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> On 2023-07-22  15:48, Ihor Radchenko wrote:
>
>> I am not familiar with Gnus, but looking at the code, may it be that
>> a Gnus article is open when Gnus summary buffer is not?
>
> Theoretically yes, if you actively and malignantly kill the summary
> buffer, for example.  In practice and through Gnus key bindings, this
> should not happen.  IOW, Gnus stops behaving reasonably as well if you
> kill the summary buffer other than through Gnus key bindings
> ("Selecting deleted buffer", etc.).
>
> If you check function `gnus-summary-work-articles' from gnus-sum.el, the
> main work horse for article processing and also good for calling in
> article buffers, you will note (focusing on the default case) the same
> paradigm
>
>   (with-current-buffer gnus-summary-buffer
>     (cond
>      [...]
>      (t
>       ;; Just return the current article.
>       (list (gnus-summary-article-number))))))
>
> As a last resort we could also try to drag in Andrew Cohen as a
> reviewer, he has been helpful already with one or two of my Gnus bugs.

We should definitely be using the paradigm above (using the
gnus-summary-buffer as the current buffer). The article number fetching
only works by accident in the article buffer, and other stuff (like
finding the original nnselect group name) won't work at all.

Later in the function we've got this:

(save-window-excursion
  (save-excursion
    (gnus-article-show-summary)
    (gnus-summary-article-header)))

If we're currently in article-mode. The call to
`gnus-article-show-summary' would protect against the case where the
summary buffer has been killed in the meantime, but I agree that's kind
of a pathological case.

Probably it would be enough to wrap the whole containing `let*' in a
(with-current-buffer gnus-summary-buffer ...). If we're already in the
summary buffer, no harm done.

Eric

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Ihor Radchenko]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> We should definitely be using the paradigm above (using the
> gnus-summary-buffer as the current buffer). The article number fetching
> only works by accident in the article buffer, and other stuff (like
> finding the original nnselect group name) won't work at all.

I am convinced then.
Ideally, it would be nice to have tests, though I have no clue how to
approach writing them.

> Later in the function we've got this:
>
> (save-window-excursion
>   (save-excursion
>     (gnus-article-show-summary)
>     (gnus-summary-article-header)))
>
> If we're currently in article-mode. The call to
> `gnus-article-show-summary' would protect against the case where the
> summary buffer has been killed in the meantime, but I agree that's kind
> of a pathological case.

I'd say that the patch will be an improvement anyway.

> Probably it would be enough to wrap the whole containing `let*' in a
> (with-current-buffer gnus-summary-buffer ...). If we're already in the
> summary buffer, no harm done.

I am not sure if it is safe.
There is
(save-window-excursion (gnus-summary-select-article))
which calls (set-buffer gnus-summary-buffer)

`with-current-buffer' will certainly alter how things work (although,
switching buffer when capturing link is already fishy).

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Max Nikulin]

On 22/07/2023 16:06, Jens Schmidt wrote:
> - Create an nnselect group and open an article from that.  In the
>    article buffer, do M-x org-store-link RET, then paste the link with
>    C-c C-l in some Org mode buffer.  The resulting link looks like
> 
> gnus:#E18xcfu-0004HT-00@fencepost.gnu.org
> 
>    That is, it lacks the group name before the hash sign.  Correct would
>    have been:
> 
> gnus:nnml+archive:test01#E18xcfu-0004HT-00@fencepost.gnu.org

I am not a gnus user, so my comments may be irrelevant due to my 
ignorance. In that case I am sorry.

Is nnselect a real NNTP group or is it some instance existing solely in 
user's configuration? Does gnus have global Message-ID index?

Generally news group is not necessary to specify a message on a NNTP 
server, it may be retrieved by Message-ID only. The name of the news 
group is mandatory if *article number* is used.

(info "(url) news/nntp/snews") or <info:url#news/nntp/snews> or
https://www.gnu.org/software/emacs/manual/html_node/url/news_002fnntp_002fsnews.html

I am curious whether namely "gnus" links have to be used or it is 
possible to rely on more generic "news:"/"nntp:" links, or even 
<mid:E18xcfu-0004HT-00@fencepost.gnu.org> (perhaps with some 
`browse-url' configuration)? It should improve portability.

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Jens Schmidt]

On 2023-07-23  12:26, Max Nikulin wrote:

> Is nnselect a real NNTP group or is it some instance existing solely
> in user's configuration?

Like nnvirtual and nnir, nnselect exists only in user's configuration,
yes.  All these three backends are "wrapper backends" that keep track of
the original messages which they wrap by pairs

   (ORIGINAL-GROUP, MESSAGE-ID)

> Does gnus have global Message-ID index?

Gnus can have a global Message-ID cache, but it's not on by default (at
least not in Emacs 28, haven't checked others).  And anyway, it's a
potentially incomplete *cache*, and not an *index*.

> (info "(url) news/nntp/snews") or <info:url#news/nntp/snews> or 
> https://www.gnu.org/software/emacs/manual/html_node/url/news_002fnntp_002fsnews.html

In any case, Gnus is *not only* an NNTP newsreader, since it has a
plethora of other backends (aka. "select methods"), ranging from nnimap
to nneething, which turns a directory into a, well, group?  See
<info:gnus#Select Methods>.  Funnily enough, I personally stopped using
Gnus as NNTP newsreader long ago ...

> I am curious whether namely "gnus" links have to be used or it is 
> possible to rely on more generic "news:"/"nntp:" links, or even 
> <mid:E18xcfu-0004HT-00@fencepost.gnu.org> (perhaps with some 
> `browse-url' configuration)? It should improve portability.

For nntp groups you already have the option to store links as web links
to groups.google.com, by means of `org-gnus-prefer-web-links'.  Here
again I'm not really an NNTP expert so I cannot tell whether this is
portable/optimal/whatever.

This could be extended to provide more options for backends that support 
more specific link types, but I think the default should stay the gnus:"
links, since only that covers all the available Gnus backends.

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Eric Abrahamsen]

Ihor Radchenko <yantar92@posteo.net> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> We should definitely be using the paradigm above (using the
>> gnus-summary-buffer as the current buffer). The article number fetching
>> only works by accident in the article buffer, and other stuff (like
>> finding the original nnselect group name) won't work at all.
>
> I am convinced then.
> Ideally, it would be nice to have tests, though I have no clue how to
> approach writing them.
>
>> Later in the function we've got this:
>>
>> (save-window-excursion
>>   (save-excursion
>>     (gnus-article-show-summary)
>>     (gnus-summary-article-header)))
>>
>> If we're currently in article-mode. The call to
>> `gnus-article-show-summary' would protect against the case where the
>> summary buffer has been killed in the meantime, but I agree that's kind
>> of a pathological case.
>
> I'd say that the patch will be an improvement anyway.
>
>> Probably it would be enough to wrap the whole containing `let*' in a
>> (with-current-buffer gnus-summary-buffer ...). If we're already in the
>> summary buffer, no harm done.
>
> I am not sure if it is safe.
> There is
> (save-window-excursion (gnus-summary-select-article))
> which calls (set-buffer gnus-summary-buffer)
>
> `with-current-buffer' will certainly alter how things work (although,
> switching buffer when capturing link is already fishy).

Ugh, this whole thing is a mess. I think the first question is: should
this function "fix" the state of Gnus before it makes a link? Should it
attempt to re-open the Summary buffer if it's been closed? Should it
switch current articles if the open article buffer is not the one that
point is on in the Summary buffer?

If we make a decision about that, then it should be easier to decide how
to handle the code changes themselves.

Eric

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Ihor Radchenko]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Ugh, this whole thing is a mess. I think the first question is: should
> this function "fix" the state of Gnus before it makes a link? Should it
> attempt to re-open the Summary buffer if it's been closed? Should it
> switch current articles if the open article buffer is not the one that
> point is on in the Summary buffer?
>
> If we make a decision about that, then it should be easier to decide how
> to handle the code changes themselves.

ol-gnus should store link for thing at point in current buffer. Ideally,
without side effects. Everything else should be implementation detail.

Judging from the shaman dances ol-gnus is performing, Gnus API is not
sufficient to get information about thing at point in an arbitrary Gnus
buffer. Or, at least, it was not sufficient at the time when ol-gnus has
been written (quite a while ago).

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Max Nikulin]

On 23/07/2023 21:13, Jens Schmidt wrote:
> Gnus can have a global Message-ID cache, but it's not on by default (at
> least not in Emacs 28, haven't checked others).  And anyway, it's a
> potentially incomplete *cache*, and not an *index*.

Thank you for clarification. Certainly just Message-ID is not enough 
without the index.

> For nntp groups you already have the option to store links as web links
> to groups.google.com, by means of `org-gnus-prefer-web-links'.

I believe that links may be converted to web archives either to open or 
to export them. In a document I would prefer a more general form like
news://news.gmane.io/gmane.emacs.orgmode/b0d3fee0-0dc9-a7ee-32dd-478297cb6b2d@vodafonemail.de
though group name is redundant.

In Thunderbird links to known articles work reasonably well
<mid:b0d3fee0-0dc9-a7ee-32dd-478297cb6b2d@vodafonemail.de>
I do not think, mid: links are ideal, but it is off-topic in this thread.

For storing links I would avoid volatile parts like mail folder where a 
message *currently* resides or name of virtual folder/group specific to 
user configuration as well as application-specific URIs.

I admit that due to gnus design, group names are unavoidable for some 
backends. I do not see a better way than the approach implemented in the 
suggested patch.

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Jens Schmidt]

Uh, I had technical issues and did not get all mails as I expected.
Cobbling things together in one big reply now, with references and
quotes hopelessly broken ... hope you can sort it out.

Anyway, thanks to Eric for chiming in.

 > Ideally, it would be nice to have tests, though I have no clue how to
 > approach writing them.

I have created a somewhat minimal Gnus setup to develop and test this
patch on my development laptop, where I normally do not use Gnus.  It
consists of a bunch of files and directories and a bit of configuration.
I can follow up on this if you like, but preferably in a separate
thread.

 >> If we're currently in article-mode. The call to
 >> `gnus-article-show-summary' would protect against the case where the
 >> summary buffer has been killed in the meantime [...].

Not really.  The following executed in an article buffer:

   (progn
    (kill-buffer gnus-summary-buffer)
    (gnus-article-show-summary))

results in

   Debugger entered--Lisp error:
       (error "There is no summary buffer for this article buffer")
     signal(error ("There is no summary buffer for this article buffer"))
     error("There is no summary buffer for this article buffer")
     gnus-article-show-summary()
     [...]

Which, OTOH, shows that I was wrong in one aspect: Gnus at least in some
cases *does* give a reasonable error message when the summary buffer for
an article buffer is gone.

 >> Probably it would be enough to wrap the whole containing `let*' in a
 >> (with-current-buffer gnus-summary-buffer ...). If we're already in the
 >> summary buffer, no harm done.
 >
 > I am not sure if it is safe.
 > There is
 > (save-window-excursion (gnus-summary-select-article))
 > which calls (set-buffer gnus-summary-buffer)

I agree with Ihor here and would rather go for individual wraps into
`with-current-buffer'.  As I have done in my patch already,
incidentially.

 >> Ugh, this whole thing is a mess. I think the first question is: should
 >> this function "fix" the state of Gnus before it makes a link? Should it
 >> attempt to re-open the Summary buffer if it's been closed? Should it
 >> switch current articles if the open article buffer is not the one that
 >> point is on in the Summary buffer?
 >>
 >> If we make a decision about that, then it should be easier to decide how
 >> to handle the code changes themselves.
 >
 > ol-gnus should store link for thing at point in current buffer. Ideally,
 > without side effects. Everything else should be implementation detail.

Could we agree on: ol-gnus should store Gnus links with as little effort
and side-effect as possible while providing reasonable functionality for
the common use cases.  I think, again incidentially, that my patch
matches this criterion.

What optionally could be improved, though, is error handling in these
pathological cases.  But that would probably require some macro

   (ol-gnus-with-current-summary-buffer BODY)

to have the error handling available in the separate places.  Not sure
whether this is worth the effort.

 > Or, at least, it was not sufficient at the time when ol-gnus has been
 > written (quite a while ago).

I don't think this has changed, really.

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Ihor Radchenko]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

>  > Ideally, it would be nice to have tests, though I have no clue how to
>  > approach writing them.
>
> I have created a somewhat minimal Gnus setup to develop and test this
> patch on my development laptop, where I normally do not use Gnus.  It
> consists of a bunch of files and directories and a bit of configuration.
> I can follow up on this if you like, but preferably in a separate
> thread.

That would be welcome. Thanks in advance!

>  >> Probably it would be enough to wrap the whole containing `let*' in a
>  >> (with-current-buffer gnus-summary-buffer ...). If we're already in the
>  >> summary buffer, no harm done.
>  >
>  > I am not sure if it is safe.
>  > There is
>  > (save-window-excursion (gnus-summary-select-article))
>  > which calls (set-buffer gnus-summary-buffer)
>
> I agree with Ihor here and would rather go for individual wraps into
> `with-current-buffer'.  As I have done in my patch already,
> incidentially.

Ok. Then, if no further objections, I will apply the patch tomorrow.

>  > ol-gnus should store link for thing at point in current buffer. Ideally,
>  > without side effects. Everything else should be implementation detail.
>
> Could we agree on: ol-gnus should store Gnus links with as little effort
> and side-effect as possible while providing reasonable functionality for
> the common use cases.  I think, again incidentially, that my patch
> matches this criterion.
>
> What optionally could be improved, though, is error handling in these
> pathological cases.  But that would probably require some macro
>
>    (ol-gnus-with-current-summary-buffer BODY)
>
> to have the error handling available in the separate places.  Not sure
> whether this is worth the effort.

We haven't had many bug reports about ol-gnus in the past, so I do not
have much statistics on whether getting such errors is common.

I do not think that implementing error handling is worth an effort here.
If killing summary buffer is already calling for trouble when using
Gnus itself, Org mode handling the error will not make things much
better.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Ihor Radchenko]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> Package: Org mode version 9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ 
> /home/jschmidt/work/org-mode/lisp/)
> From e1bc9aefd4fd0080012c172d1c21c684a5b2fe51 Mon Sep 17 00:00:00 2001

I looked into applying your patch, but there are several minor issues I
need to clarify.

> From: farblos <43711228+farblos@users.noreply.github.com>

This is not a proper email address and name. Should I use Jens Schmidt
<jschmidt4gnu@vodafonemail.de> ?

> Date: Sat, 22 Jul 2023 10:30:19 +0200
> Subject: [PATCH] ol-gnus.el: Fix issue when storing links from Gnus article
>  buffers
>
> * lisp/ol-gnus.el (org-gnus-store-link): Switch to
> `gnus-summary-buffer' when calling functions that are intended to be
> called only there.

May you please add TINYCHANGE cookie to commit message. I do not see you
having a copyright status in our records. See
https://orgmode.org/worg/org-contribute.html#first-patch

And maybe add a link to this discussion.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Jens Schmidt]

On 2023-07-26  18:04, Ihor Radchenko wrote:

> This is not a proper email address and name. Should I use Jens 
> Schmidt <jschmidt4gnu@vodafonemail.de> ?

Sorry, forgot to maintain that yet in my org-mode clone.  Will do.

> May you please add TINYCHANGE cookie to commit message. I do not see
>  you having a copyright status in our records. See 
> https://orgmode.org/worg/org-contribute.html#first-patch

I completed already an FSF copyright assignment for Emacs.  Does that
count for org-mode as well or do I need to extend/repeat that somehow?
A TINYCHANGE might be sufficient here but I think we could as well
clarify that issue "the thorough way" right from the start.

> And maybe add a link to this discussion.

Will do as well.

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Ihor Radchenko]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> I completed already an FSF copyright assignment for Emacs.  Does that
> count for org-mode as well or do I need to extend/repeat that somehow?

No, you don't need to do anything. Org mode is a part of Emacs.

Bastien, may your please check FSF records?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Eric Abrahamsen]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> Uh, I had technical issues and did not get all mails as I expected.
> Cobbling things together in one big reply now, with references and
> quotes hopelessly broken ... hope you can sort it out.
>
> Anyway, thanks to Eric for chiming in.
>
>> Ideally, it would be nice to have tests, though I have no clue how to
>> approach writing them.
>
> I have created a somewhat minimal Gnus setup to develop and test this
> patch on my development laptop, where I normally do not use Gnus.  It
> consists of a bunch of files and directories and a bit of configuration.
> I can follow up on this if you like, but preferably in a separate
> thread.

Someone mentioned gnus-mock in the other thread, which I made for
exactly these scenarios. It's a bit hairy making a Gnus installation
from nothing -- if you try it out and would like it to do something
different, I'd be interested in feedback.

>>> If we're currently in article-mode. The call to
>>> `gnus-article-show-summary' would protect against the case where the
>>> summary buffer has been killed in the meantime [...].
>
> Not really.  The following executed in an article buffer:
>
>   (progn
>    (kill-buffer gnus-summary-buffer)
>    (gnus-article-show-summary))
>
> results in
>
>   Debugger entered--Lisp error:
>       (error "There is no summary buffer for this article buffer")
>     signal(error ("There is no summary buffer for this article buffer"))
>     error("There is no summary buffer for this article buffer")
>     gnus-article-show-summary()
>     [...]
>
> Which, OTOH, shows that I was wrong in one aspect: Gnus at least in some
> cases *does* give a reasonable error message when the summary buffer for
> an article buffer is gone.
>
>>> Probably it would be enough to wrap the whole containing `let*' in a
>>> (with-current-buffer gnus-summary-buffer ...). If we're already in the
>>> summary buffer, no harm done.
>>
>> I am not sure if it is safe.
>> There is
>> (save-window-excursion (gnus-summary-select-article))
>> which calls (set-buffer gnus-summary-buffer)
>
> I agree with Ihor here and would rather go for individual wraps into
> `with-current-buffer'.  As I have done in my patch already,
> incidentially.

Somehow I missed that this whole thread started with an actual patch!
Sorry, that should have been the subject of discussion the whole time. I
can help with applying it, once all issues are resolved.

Thanks,
Eric

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Bastien Guerry]

Hi Ihor and Jens,

Ihor Radchenko <yantar92@posteo.net> writes:

> Bastien, may your please check FSF records?

Done, Jens records are okay.  Thanks for contributing!

-- 
 Bastien Guerry

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Ihor Radchenko]

Bastien Guerry <bzg@gnu.org> writes:

>> Bastien, may your please check FSF records?
>
> Done, Jens records are okay.  Thanks for contributing!

Thanks for checking!
Updated our records.
https://git.sr.ht/~bzg/worg/commit/95559f89

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Jens Schmidt]

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

On 2023-07-29  09:04, Ihor Radchenko wrote:

> Thanks for checking!
> Updated our records.

And thanks for doing the paperworks.

Attached is a patch and commit message with:

- patch payload unchanged,

- email address in commit message fixed

- link to this thread in commit message added.

Does look right now?

[-- Attachment #2: 0001-ol-gnus.el-Fix-issue-when-storing-links-from-Gnus-ar.patch --]
[-- Type: text/x-patch, Size: 2536 bytes --]

From 74d5c1f5caf4ce425eb888340c2115276b4ddf8a Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Sun, 30 Jul 2023 17:51:27 +0200
Subject: [PATCH] ol-gnus.el: Fix issue when storing links from Gnus article
 buffers

* lisp/ol-gnus.el (org-gnus-store-link): Switch to
`gnus-summary-buffer' when calling functions that are intended to be
called only there.

Link: https://list.orgmode.org/orgmode/2fa5914d-2cbf-f41f-8be6-e79e77794140@vodafonemail.de
---
 lisp/ol-gnus.el | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/lisp/ol-gnus.el b/lisp/ol-gnus.el
index 7c07ce045..f0e04ce66 100644
--- a/lisp/ol-gnus.el
+++ b/lisp/ol-gnus.el
@@ -137,27 +137,23 @@ If `org-store-link' was called with a prefix arg the meaning of
      (let* ((group
 	     (pcase (gnus-find-method-for-group gnus-newsgroup-name)
 	       (`(nnvirtual . ,_)
-		(save-excursion
-		  (car (nnvirtual-map-article (gnus-summary-article-number)))))
+		(with-current-buffer gnus-summary-buffer
+		  (save-excursion
+		    (car (nnvirtual-map-article (gnus-summary-article-number))))))
 	       (`(,(or `nnselect `nnir) . ,_)  ; nnir is for Emacs < 28.
-		(save-excursion
-		  (cond
-		   ((fboundp 'nnselect-article-group)
-		    (nnselect-article-group (gnus-summary-article-number)))
-		   ((fboundp 'nnir-article-group)
-		    (nnir-article-group (gnus-summary-article-number)))
-		   (t
-		    (error "No article-group variant bound")))))
+		(with-current-buffer gnus-summary-buffer
+		  (save-excursion
+		    (cond
+		     ((fboundp 'nnselect-article-group)
+		      (nnselect-article-group (gnus-summary-article-number)))
+		     ((fboundp 'nnir-article-group)
+		      (nnir-article-group (gnus-summary-article-number)))
+		     (t
+		      (error "No article-group variant bound"))))))
 	       (_ gnus-newsgroup-name)))
-	    (header (if (eq major-mode 'gnus-article-mode)
-			;; When in an article, first move to summary
-			;; buffer, with point on the summary of the
-			;; current article before extracting headers.
-			(save-window-excursion
-			  (save-excursion
-			    (gnus-article-show-summary)
-			    (gnus-summary-article-header)))
-		      (gnus-summary-article-header)))
+	    (header (with-current-buffer gnus-summary-buffer
+		      (save-excursion
+			(gnus-summary-article-header))))
 	    (from (mail-header-from header))
 	    (message-id (org-unbracket-string "<" ">" (mail-header-id header)))
 	    (date (org-trim (mail-header-date header)))
-- 
2.30.2

Re: [BUG] Issues in ol-gnus when storing links in nnvirtual and nnselect articles [9.7-pre (release_9.6.7-570-gd6f3ae.dirty @ /home/jschmidt/work/org-mode/lisp/)]

[Ihor Radchenko]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> Attached is a patch and commit message with:
>
> - patch payload unchanged,
>
> - email address in commit message fixed
>
> - link to this thread in commit message added.
>
> Does look right now?

Yup.
Applied, onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=eabc9bfec
Fixed.

Thanks for your contribution!

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>