PATCH Make org-open-at-point only ask once

PATCH Make org-open-at-point only ask once

[Anders Waldenborg]

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

Hi!

If an org buffer is narrowed, and one tries to do org-open-at-point on
a link that points to outside of the restriction it asks: "No match -
create this as a new heading?". When answering no the buffer is
widened and the reseach is done, and if the link still can't be
resolved the question is asked again.

For nonexistant links this happens even if the buffer isn't narrowed -
one needs to answer "n" twice.

I also attached an alternate patch which (IMHO) simplifies the
implementation by hiding the hard work in a macro, and as a bonus it
only calls org-link-search once. But it is much more intrusive.

 anders

;; simple testcase to show the bug
(progn
  (insert "* A\n\n* B\n\n[[A]]")
  (org-narrow-to-subtree)
  (org-open-at-point))

[-- Attachment #2: org-open-at-point.patch --]
[-- Type: text/x-diff, Size: 1163 bytes --]

commit 54702f063ae2df48dec7f9feb80859a6b64002a4
Author: Anders Waldenborg <anders@0x63.nu>
Date:   Sat Aug 27 21:18:46 2011 +0200

    Make org-open-at-point only ask once whether new header should be created.
    
    When following "thisfile" links org-open-at-point is kind enough to
    retry org-link-search again after widening the buffer it can't be
    found. However org-link-search also asks the question "No match -
    create this as a new heading? (y or n)" when target can't be
    found. This means that the question is asked twice when following a
    nonexistent link and answering no.
    
    This is fixed by setting org-link-search-inhibit-query in first try,
    so only second invocation asks the question.

diff --git a/lisp/org.el b/lisp/org.el
index d63b854..781de88 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -9537,7 +9537,8 @@ application the system uses for this file type."
 			      ((equal arg '(16)) ''org-occur)
 			      (t nil))
 		       ,pos)))
-	    (condition-case nil (eval cmd)
+	    (condition-case nil (let ((org-link-search-inhibit-query t))
+				  (eval cmd))
 	      (error (progn (widen) (eval cmd))))))
 
 	 (t

[-- Attachment #3: org-open-at-point-alternate.patch --]
[-- Type: text/x-diff, Size: 1618 bytes --]

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index 13aff02..153a041 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -414,6 +414,24 @@ the value in cdr."
     (cons (list (car flat) (cadr flat))
          (org-make-parameter-alist (cddr flat)))))
 
+(defmacro org-widen-and-maybe-renarrow (&rest BODY)
+  "Widen buffer and evaluate body, and if point is outside
+the previously narrowed-to region after evaluation permanetly
+lift the restriction."
+  (declare (indent defun))
+  (org-with-gensyms (res pnt)
+    `(let ((,res)
+	   (,pnt))
+       (save-restriction
+	 (widen)
+	 (setq ,res ,@BODY)
+	 (setq ,pnt (point)))
+       (when (or (< ,pnt (point-min))
+		 (> ,pnt (point-max)))
+	 (widen)
+	 (goto-char ,pnt))
+       ,res)))
+
 (provide 'org-macs)
 
 ;;; org-macs.el ends here
diff --git a/lisp/org.el b/lisp/org.el
index d63b854..efe936e 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -9531,15 +9531,12 @@ application the system uses for this file type."
 	      (switch-to-buffer-other-window
 	       (org-get-buffer-for-internal-link (current-buffer)))
 	    (org-mark-ring-push))
-	  (let ((cmd `(org-link-search
-		       ,path
-		       ,(cond ((equal arg '(4)) ''occur)
-			      ((equal arg '(16)) ''org-occur)
-			      (t nil))
-		       ,pos)))
-	    (condition-case nil (eval cmd)
-	      (error (progn (widen) (eval cmd))))))
-
+	  (org-widen-and-maybe-renarrow
+	    (org-link-search path
+			     (cond ((equal arg '(4)) 'occur)
+				   ((equal arg '(16)) 'org-occur)
+				   (t nil))
+			     pos)))
 	 (t
 	  (browse-url-at-point)))))))
   (move-marker org-open-link-marker nil)

Re: PATCH Make org-open-at-point only ask once

[Nicolas Goaziou]

Hello,

Anders Waldenborg <anders@0x63.nu> writes:

> If an org buffer is narrowed, and one tries to do org-open-at-point on
> a link that points to outside of the restriction it asks: "No match -
> create this as a new heading?". When answering no the buffer is
> widened and the reseach is done, and if the link still can't be
> resolved the question is asked again.
>
> For nonexistant links this happens even if the buffer isn't narrowed -
> one needs to answer "n" twice.
>
> I also attached an alternate patch which (IMHO) simplifies the
> implementation by hiding the hard work in a macro, and as a bonus it
> only calls org-link-search once. But it is much more intrusive.

Yes, imposing widening to the user is intrusive. Moreover, I think you
cannot avoid to call org-link-search twice (once it has failed): the
point is to do a local search and then a global one.

Though, the macro idea is interesting, as I can see at least one other
place where it might be useful. So, what about changing the macro to:

1. If current buffer is narrowed, execute body with the current
   restriction. If it fails (silently), re-execute body with a widened
   buffer. Restore default narrowing.

2. If the buffer has no narrowing in effect, just execute body.

Also:

1. Doc-strings must refer to the arguments.

2. Don't forget to add: (def-edebug-spec macro-name (arguments)) just
   after it.

3. You need to add a proper ChangeLog message in your commit.


Thank you for looking at this.


Regards,

-- 
Nicolas Goaziou

Re: PATCH Make org-open-at-point only ask once

[Anders Waldenborg]

On Mon, Aug 29, 2011 at 09:14:13AM +0200, Nicolas Goaziou wrote:
> Yes, imposing widening to the user is intrusive. Moreover, I think you
> cannot avoid to call org-link-search twice (once it has failed): the
> point is to do a local search and then a global one.

Yes, "local then global" search makes sense (I especially like how the
infobrowser pops out to a global search with an extra ^S after failure).

But for org links, I'm not 100% sure - consider this org-file:

* Notes
* Some topic
[[Notes]]
** Notes

If buffer is widen, the link goes to the toplevel header, but if I
narrow to "Some topic" the link suddenly starts going to the subheader.

So the links meaning changes depending on if the buffer is narrowed or
not.

I'm not sure it really is a problem, I just want to make sure it is taken
into consideration.

> 
> Though, the macro idea is interesting, as I can see at least one other
> place where it might be useful. So, what about changing the macro to:
> 
> 1. If current buffer is narrowed, execute body with the current
>    restriction. If it fails (silently), re-execute body with a widened
>    buffer. Restore default narrowing.

Fails means that it throws error? Or any exception?

> 
> 2. If the buffer has no narrowing in effect, just execute body.
> 
> Also:

Thanks for the review, I'll look into it soon.

 anders

Re: PATCH Make org-open-at-point only ask once

[Nicolas Goaziou]

Anders Waldenborg <anders@0x63.nu> writes:

> But for org links, I'm not 100% sure - consider this org-file:
>
> * Notes
> * Some topic
> [[Notes]]
> ** Notes
>
> If buffer is widen, the link goes to the toplevel header, but if I
> narrow to "Some topic" the link suddenly starts going to the
> subheader.

And I think that totally makes sense. If the user bothered narrowing
buffer to a sub-tree, he probably wants to focus on the Notes relative to
that sub-tree.

Now, you're right, that search behaviour won't be consistent as long as
it will rely on narrowing.

Maybe we should define a consistent link search: ignore the narrowing
but first search in current sub-tree, if that fails (any error, I guess)
search in current tree and if that one fails too, search in the whole
buffer.

> Thanks for the review, I'll look into it soon.

Np.

Regards,

-- 
Nicolas Goaziou

Re: PATCH Make org-open-at-point only ask once

[Anders Waldenborg]

On Mon, Aug 29, 2011 at 11:36:19AM +0200, Nicolas Goaziou wrote:
> Maybe we should define a consistent link search: ignore the narrowing
> but first search in current sub-tree, if that fails (any error, I guess)
> search in current tree and if that one fails too, search in the whole
> buffer.

So something with the same semantics as this:

(defun find-nearest-heading-named (l)
  (unless (find-heading-in-current-subtree l)
    (up-one-level)
    (find-nearest-heading-named l)))

i.e find the target with its parent headings list sharing the longest
prefix with current point's.

* A
** B
* C
** B
** D
*** B
*** E

A link to B inside E would go to C/D/B because (C D B) shares longer
prefix with (C D E) than (C B) and (A B) does.

 anders

Re: PATCH Make org-open-at-point only ask once

[Nicolas Goaziou]

Anders Waldenborg <anders@0x63.nu> writes:

> On Mon, Aug 29, 2011 at 11:36:19AM +0200, Nicolas Goaziou wrote:
>> Maybe we should define a consistent link search: ignore the narrowing
>> but first search in current sub-tree, if that fails (any error, I guess)
>> search in current tree and if that one fails too, search in the whole
>> buffer.
>
> So something with the same semantics as this:
>
> (defun find-nearest-heading-named (l)
>   (unless (find-heading-in-current-subtree l)
>     (up-one-level)
>     (find-nearest-heading-named l)))

Recursively, yes. And enclosing this with (org-with-wide-buffer ...)
macro.

> i.e find the target with its parent headings list sharing the longest
> prefix with current point's.
>
> * A
> ** B
> * C
> ** B
> ** D
> *** B
> *** E
>
> A link to B inside E would go to C/D/B because (C D B) shares longer
> prefix with (C D E) than (C B) and (A B) does.

To be sure we understand it the same way:

1. Go to E and search from there: failure.
2. Go to D and search from there: success.

And if there's a link to D in A/B:

1. Search from B: failure.
2. Search from A: failure.
3. Search from (point-min): success.

Also, I would use "hierarchically" (or the like) instead of "nearest",
which can be confusing, as in this example:

* A
** F
* B
** C
*** D
**** F

Linking F from B should point to **** F instead of ** F while the latter
is nearer if considered distance is positive difference between buffer
positions.

What do you (or anyone reading this) think?

Regards,

-- 
Nicolas Goaziou

Re: PATCH Make org-open-at-point only ask once

[Carsten Dominik]

Hi Anders, Nicolas

I am wondering what the status of this patch is.  There was a discussion, but I am not sure about the conclusion...

- Carsten

On Aug 28, 2011, at 10:05 PM, Anders Waldenborg wrote:

> Hi!
> 
> If an org buffer is narrowed, and one tries to do org-open-at-point on
> a link that points to outside of the restriction it asks: "No match -
> create this as a new heading?". When answering no the buffer is
> widened and the reseach is done, and if the link still can't be
> resolved the question is asked again.
> 
> For nonexistant links this happens even if the buffer isn't narrowed -
> one needs to answer "n" twice.
> 
> I also attached an alternate patch which (IMHO) simplifies the
> implementation by hiding the hard work in a macro, and as a bonus it
> only calls org-link-search once. But it is much more intrusive.
> 
> anders
> 
> ;; simple testcase to show the bug
> (progn
>  (insert "* A\n\n* B\n\n[[A]]")
>  (org-narrow-to-subtree)
>  (org-open-at-point))
> <org-open-at-point.patch><org-open-at-point-alternate.patch>

- Carsten

Re: PATCH Make org-open-at-point only ask once

[Anders Waldenborg]

On Thu, Oct 06, 2011 at 10:00:38AM +0200, Carsten Dominik wrote:
> Hi Anders, Nicolas
> 
> I am wondering what the status of this patch is.  There was a discussion, but I am not sure about the conclusion...


Carsten,

I think that the simple patch still is valid, I'm including it again
below.

The discussion was about my more intrusive alternative patch, and how
links change their meaning depending on if the buffer is narrowed or
not. I did start to hack up a alternative link resolver as a proof of
concept to be able to better understand how links should work in case
there are multiple targets, but unfortunately I got busy with other
things.

 anders

--

commit 54702f063ae2df48dec7f9feb80859a6b64002a4
Author: Anders Waldenborg <anders@0x63.nu>
Date:   Sat Aug 27 21:18:46 2011 +0200

    Make org-open-at-point only ask once whether new header should be created.
    
    When following "thisfile" links org-open-at-point is kind enough to
    retry org-link-search again after widening the buffer it can't be
    found. However org-link-search also asks the question "No match -
    create this as a new heading? (y or n)" when target can't be
    found. This means that the question is asked twice when following a
    nonexistent link and answering no.
    
    This is fixed by setting org-link-search-inhibit-query in first try,
    so only second invocation asks the question.

diff --git a/lisp/org.el b/lisp/org.el
index d63b854..781de88 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -9537,7 +9537,8 @@ application the system uses for this file type."
 			      ((equal arg '(16)) ''org-occur)
 			      (t nil))
 		       ,pos)))
-	    (condition-case nil (eval cmd)
+	    (condition-case nil (let ((org-link-search-inhibit-query t))
+				  (eval cmd))
 	      (error (progn (widen) (eval cmd))))))
 
 	 (t

Re: PATCH Make org-open-at-point only ask once

[Carsten Dominik]

OK, I have accepted the patch.  Thanks

- Carsten

On Oct 6, 2011, at 10:43 AM, Anders Waldenborg wrote:

> On Thu, Oct 06, 2011 at 10:00:38AM +0200, Carsten Dominik wrote:
>> Hi Anders, Nicolas
>> 
>> I am wondering what the status of this patch is.  There was a discussion, but I am not sure about the conclusion...
> 
> 
> Carsten,
> 
> I think that the simple patch still is valid, I'm including it again
> below.
> 
> The discussion was about my more intrusive alternative patch, and how
> links change their meaning depending on if the buffer is narrowed or
> not. I did start to hack up a alternative link resolver as a proof of
> concept to be able to better understand how links should work in case
> there are multiple targets, but unfortunately I got busy with other
> things.
> 
> anders
> 
> --
> 
> commit 54702f063ae2df48dec7f9feb80859a6b64002a4
> Author: Anders Waldenborg <anders@0x63.nu>
> Date:   Sat Aug 27 21:18:46 2011 +0200
> 
>    Make org-open-at-point only ask once whether new header should be created.
> 
>    When following "thisfile" links org-open-at-point is kind enough to
>    retry org-link-search again after widening the buffer it can't be
>    found. However org-link-search also asks the question "No match -
>    create this as a new heading? (y or n)" when target can't be
>    found. This means that the question is asked twice when following a
>    nonexistent link and answering no.
> 
>    This is fixed by setting org-link-search-inhibit-query in first try,
>    so only second invocation asks the question.
> 
> diff --git a/lisp/org.el b/lisp/org.el
> index d63b854..781de88 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -9537,7 +9537,8 @@ application the system uses for this file type."
> 			      ((equal arg '(16)) ''org-occur)
> 			      (t nil))
> 		       ,pos)))
> -	    (condition-case nil (eval cmd)
> +	    (condition-case nil (let ((org-link-search-inhibit-query t))
> +				  (eval cmd))
> 	      (error (progn (widen) (eval cmd))))))
> 
> 	 (t

- Carsten

Re: PATCH Make org-open-at-point only ask once

[Anders Waldenborg]

For the record, I'm below the limit of a cumulative change of 20
non-repetitive change lines.

On Thu, Oct 06, 2011 at 10:54:10AM +0200, Carsten Dominik wrote:
> OK, I have accepted the patch.  Thanks
> 
> - Carsten
> 
> On Oct 6, 2011, at 10:43 AM, Anders Waldenborg wrote:
> 
> > On Thu, Oct 06, 2011 at 10:00:38AM +0200, Carsten Dominik wrote:
> >> Hi Anders, Nicolas
> >> 
> >> I am wondering what the status of this patch is.  There was a discussion, but I am not sure about the conclusion...
> > 
> > 
> > Carsten,
> > 
> > I think that the simple patch still is valid, I'm including it again
> > below.
> > 
> > The discussion was about my more intrusive alternative patch, and how
> > links change their meaning depending on if the buffer is narrowed or
> > not. I did start to hack up a alternative link resolver as a proof of
> > concept to be able to better understand how links should work in case
> > there are multiple targets, but unfortunately I got busy with other
> > things.
> > 
> > anders
> > 
> > --
> > 
> > commit 54702f063ae2df48dec7f9feb80859a6b64002a4
> > Author: Anders Waldenborg <anders@0x63.nu>
> > Date:   Sat Aug 27 21:18:46 2011 +0200
> > 
> >    Make org-open-at-point only ask once whether new header should be created.
> > 
> >    When following "thisfile" links org-open-at-point is kind enough to
> >    retry org-link-search again after widening the buffer it can't be
> >    found. However org-link-search also asks the question "No match -
> >    create this as a new heading? (y or n)" when target can't be
> >    found. This means that the question is asked twice when following a
> >    nonexistent link and answering no.
> > 
> >    This is fixed by setting org-link-search-inhibit-query in first try,
> >    so only second invocation asks the question.
> > 
> > diff --git a/lisp/org.el b/lisp/org.el
> > index d63b854..781de88 100644
> > --- a/lisp/org.el
> > +++ b/lisp/org.el
> > @@ -9537,7 +9537,8 @@ application the system uses for this file type."
> > 			      ((equal arg '(16)) ''org-occur)
> > 			      (t nil))
> > 		       ,pos)))
> > -	    (condition-case nil (eval cmd)
> > +	    (condition-case nil (let ((org-link-search-inhibit-query t))
> > +				  (eval cmd))
> > 	      (error (progn (widen) (eval cmd))))))
> > 
> > 	 (t
> 
> - Carsten
> 
> 
> 
> 

-- 

Re: PATCH Make org-open-at-point only ask once

[Carsten Dominik]

On Oct 6, 2011, at 11:51 AM, Anders Waldenborg wrote:

> For the record, I'm below the limit of a cumulative change of 20
> non-repetitive change lines.


Yes, I have marked the change as TINYCHANGE

- Carsten

> 
> On Thu, Oct 06, 2011 at 10:54:10AM +0200, Carsten Dominik wrote:
>> OK, I have accepted the patch.  Thanks
>> 
>> - Carsten
>> 
>> On Oct 6, 2011, at 10:43 AM, Anders Waldenborg wrote:
>> 
>>> On Thu, Oct 06, 2011 at 10:00:38AM +0200, Carsten Dominik wrote:
>>>> Hi Anders, Nicolas
>>>> 
>>>> I am wondering what the status of this patch is.  There was a discussion, but I am not sure about the conclusion...
>>> 
>>> 
>>> Carsten,
>>> 
>>> I think that the simple patch still is valid, I'm including it again
>>> below.
>>> 
>>> The discussion was about my more intrusive alternative patch, and how
>>> links change their meaning depending on if the buffer is narrowed or
>>> not. I did start to hack up a alternative link resolver as a proof of
>>> concept to be able to better understand how links should work in case
>>> there are multiple targets, but unfortunately I got busy with other
>>> things.
>>> 
>>> anders
>>> 
>>> --
>>> 
>>> commit 54702f063ae2df48dec7f9feb80859a6b64002a4
>>> Author: Anders Waldenborg <anders@0x63.nu>
>>> Date:   Sat Aug 27 21:18:46 2011 +0200
>>> 
>>>   Make org-open-at-point only ask once whether new header should be created.
>>> 
>>>   When following "thisfile" links org-open-at-point is kind enough to
>>>   retry org-link-search again after widening the buffer it can't be
>>>   found. However org-link-search also asks the question "No match -
>>>   create this as a new heading? (y or n)" when target can't be
>>>   found. This means that the question is asked twice when following a
>>>   nonexistent link and answering no.
>>> 
>>>   This is fixed by setting org-link-search-inhibit-query in first try,
>>>   so only second invocation asks the question.
>>> 
>>> diff --git a/lisp/org.el b/lisp/org.el
>>> index d63b854..781de88 100644
>>> --- a/lisp/org.el
>>> +++ b/lisp/org.el
>>> @@ -9537,7 +9537,8 @@ application the system uses for this file type."
>>> 			      ((equal arg '(16)) ''org-occur)
>>> 			      (t nil))
>>> 		       ,pos)))
>>> -	    (condition-case nil (eval cmd)
>>> +	    (condition-case nil (let ((org-link-search-inhibit-query t))
>>> +				  (eval cmd))
>>> 	      (error (progn (widen) (eval cmd))))))
>>> 
>>> 	 (t
>> 
>> - Carsten
>> 
>> 
>> 
>> 
> 
> -- 

- Carsten