[-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1.1: 0001-Fix-the-uncaught-exception-when-doing-opening-a-link.patch --] [-- Type: text/x-diff, Size: 2044 bytes --] From 0e31213fa486f7fcfe1c2b7037689df077a39fce Mon Sep 17 00:00:00 2001 From: Samuel Loury <samuel.loury@cosmo-platform.org> Date: Thu, 22 Nov 2012 09:31:15 +0100 Subject: [PATCH] Fix the uncaught exception when doing opening a link from nowhere * lisp/org.el (org-open-at-point): Make sure point is on a org-plain-link-re before trying to go to its beginning In cases the custor at point did not match anything, the piece of code (goto-char (car (org-in-regexp org-plain-link-re))) threw an error. The inital intention of avoiding matching a org-plain-link-re when just after a org-bracket-link-regexp, from the commit originating the error (ad35e2ac6c6decae55dd987be738e07e7c87bd7d) was conserved. TINYCHANGE --- lisp/org.el | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/lisp/org.el b/lisp/org.el index 080b527..d036c2a 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -9745,8 +9745,29 @@ application the system uses for this file type." (save-excursion (when (or (org-in-regexp org-angle-link-re) - (and (goto-char (car (org-in-regexp org-plain-link-re))) - (save-match-data (not (looking-back "\\[\\["))))) + (let ( + (match (org-in-regexp org-plain-link-re)) + ) + (and + ;; link at point is a plain link + match + ;; check that it is not of the form + ;; [[http://orgmode.org][Org]]Mode. in that + ;; case, if the cursor is on "Mode", then the + ;; string "http://orgmode.org][Org]]Mode" is + ;; recognized as a plain link while it should + ;; not be + (progn + ;; go to the begining of the match, If we + ;; were in the special case, we should now + ;; be in a org-bracket-link-regexp + (goto-char (car match)) + (not + (org-in-regexp org-bracket-link-regexp) + ) + ) + ) + )) (setq type (match-string 1) path (org-link-unescape (match-string 2))) (throw 'match t))) -- 1.7.10.4 [-- Attachment #1.2: Type: text/plain, Size: 575 bytes --] Hi, When trying to open a link at point when no link is present, an error is thrown. Test for instance to call org-open-at-point (C-c C-o) while in an empty line. It is in fact a regression coming from ad35e2ac6c6decae55dd987be738e07e7c87bd7d that tries to go to the result of a org-in-regexp call without checking whether the result is empty. Here is a patch that keeps the idea from ad35e2ac6c6decae55dd987be738e07e7c87bd7d (avoiding matching an org-plain-link-re while it is in fact a org-bracket-link-regexp) and fixing the problem. -- Samuel [-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]
Hello, Samuel Loury <konubinix@gmail.com> writes: > From 0e31213fa486f7fcfe1c2b7037689df077a39fce Mon Sep 17 00:00:00 2001 > From: Samuel Loury <samuel.loury@cosmo-platform.org> > Date: Thu, 22 Nov 2012 09:31:15 +0100 > Subject: [PATCH] Fix the uncaught exception when doing opening a link from > nowhere > > * lisp/org.el (org-open-at-point): Make sure point is on a > org-plain-link-re before trying to go to its beginning > > In cases the custor at point did not match anything, the piece of > code (goto-char (car (org-in-regexp org-plain-link-re))) threw an > error. > > The inital intention of avoiding matching a org-plain-link-re when > just after a org-bracket-link-regexp, from the commit originating > the error (ad35e2ac6c6decae55dd987be738e07e7c87bd7d) was conserved. > > TINYCHANGE Thank you for your patch. A few comments below. > (save-excursion > (when (or (org-in-regexp org-angle-link-re) > - (and (goto-char (car (org-in-regexp org-plain-link-re))) > - (save-match-data (not (looking-back "\\[\\["))))) > + (let ( > + (match (org-in-regexp org-plain-link-re)) > + ) Please do not leave dangling parens in your code. This is quite confusing. > + (and > + ;; link at point is a plain link > + match > + ;; check that it is not of the form > + ;; [[http://orgmode.org][Org]]Mode. in that > + ;; case, if the cursor is on "Mode", then the > + ;; string "http://orgmode.org][Org]]Mode" is > + ;; recognized as a plain link while it should > + ;; not be > + (progn > + ;; go to the begining of the match, If we > + ;; were in the special case, we should now > + ;; be in a org-bracket-link-regexp > + (goto-char (car match)) > + (not > + (org-in-regexp org-bracket-link-regexp) > + ) > + ) > + ) > + )) Ditto. > When trying to open a link at point when no link is present, an > error is thrown. Test for instance to call org-open-at-point (C-c C-o) > while in an empty line. > > It is in fact a regression coming from > ad35e2ac6c6decae55dd987be738e07e7c87bd7d that tries to go to the result > of a org-in-regexp call without checking whether the result is empty. > > Here is a patch that keeps the idea from > ad35e2ac6c6decae55dd987be738e07e7c87bd7d (avoiding matching an > org-plain-link-re while it is in fact a org-bracket-link-regexp) and > fixing the problem. Would you mind providing some test cases for `org-open-at-point' (or ERT tests)? This function will need to be rewritten at some point; it's better if we can then avoid introducing regressions. Regards, -- Nicolas Goaziou
Hi Samuel and Nicolas, I allowed myself to fix this, with a somewhat smaller patch: http://orgmode.org/cgit.cgi/org-mode.git/commit/?h=maint&id=14ffe2 Thanks Samuel for the patch and for reporting this problem! -- Bastien
[-- Attachment #1.1: Type: text/plain, Size: 2186 bytes --] Hi and thanks for paying attention to my patch. Bastien <bzg@altern.org> writes: > I allowed myself to fix this, with a somewhat smaller patch: > http://orgmode.org/cgit.cgi/org-mode.git/commit/?h=maint&id=14ffe2 This is indeed a good way to fix the uncaught error problem. Nonetheless, the patch I provided was intended to also correct a functional problem (to my mind) about the consistency of the behavior of org-open-at-point relatively to plain links. Let me explain what I mean: * First test: with bracket links Imagine you have the following line (I placed a _ to indicate the position of the cursor) ╭──── │ some non r_elevant text [[id:some-id]] ╰──── Within that situation, launching org-open-at-point would follow the id:some-id link. In the following situation (with the cursor inside the link) ╭──── │ some non relevant text [[id:so_me-id]] ╰──── org-open-at-point will also open the link. That one seems obvious but I think it is worth keeping in mind though. * Second test: with plain links Now, imagine the same scenarios with plain links ╭──── │ some non relevant text id:so_me-id ╰──── With the cursor inside the link, org-open-at-point follows the link. Then with the cursor before the link: ╭──── │ some non r_elevant text id:some-id ╰──── org-open-at-point results in an error (now caught thanks to Bastien) while I (and I stress it is my personal opinion) think that it should also follow the link, like in the case of a bracket link. As suggested by Nicolas, I provided tests in attachment to show the 4 use cases I just explained. The initial patch I provided was meant to enhance the implementation of org-open-at-point to make those use cases work. What do you think? Should we enhance the function that way. I use the patched version for some time now and I often launch org-open-at-point (C-c C-o) before plain links. Thanks again for your answers. -- Konubinix GPG Key : 7439106A Fingerprint: 5993 BE7A DA65 E2D9 06CE 5C36 75D2 3CED 7439 106A [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1.2: test --] [-- Type: text/x-diff, Size: 3160 bytes --] From d9df74a72962020c3e695be0ad9a8646a42ee9de Mon Sep 17 00:00:00 2001 From: Samuel Loury <konubinixweb@gmail.com> Date: Fri, 4 Jan 2013 12:31:01 +0100 Subject: [PATCH] Addition of tests highlighting the expected behavior of org-open-at-point in several circumstances --- testing/examples/open-at-point.org | 8 ++++ testing/lisp/test-org-open-at-point.el | 63 ++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 testing/examples/open-at-point.org create mode 100644 testing/lisp/test-org-open-at-point.el diff --git a/testing/examples/open-at-point.org b/testing/examples/open-at-point.org new file mode 100644 index 0000000..b3bb92d --- /dev/null +++ b/testing/examples/open-at-point.org @@ -0,0 +1,8 @@ + +* Header 1 + :PROPERTIES: + :ID: header1_with_great_id + :END: +* Header 2 + [[id:header1_with_great_id][Header 1]] + id:header1_with_great_id diff --git a/testing/lisp/test-org-open-at-point.el b/testing/lisp/test-org-open-at-point.el new file mode 100644 index 0000000..efb70c8 --- /dev/null +++ b/testing/lisp/test-org-open-at-point.el @@ -0,0 +1,63 @@ +;;; test-org-open-at-point.el + +;; Copyright (c) Samuel Loury +;; Authors: Samuel Loury + +;; Released under the GNU General Public License version 3 +;; see: http://www.gnu.org/licenses/gpl-3.0.html + +;;;; Comments: + +;; Test for the org-open-at-point function + +;;; Code: + +\f +;;; Bracket links + +(save-excursion + (set-buffer (get-buffer-create "test-org-open-at-point.el")) + (setq ly-here + (file-name-directory + (or load-file-name (buffer-file-name))))) + +(defun test-org-open-at-point/goto-fixture () + (find-file-other-window + (concat ly-here "../examples/open-at-point.org")) + (set-buffer "open-at-point.org")) + +(ert-deftest test-org-open-at-point/bracket-link-inside () + "Test `org-open-at-point' from inside a bracket link." + (test-org-open-at-point/goto-fixture) + ;; go inside the bracket link + (goto-char 113) + (org-open-at-point) + ;; should now be in front of the header + (should (equal (point) 2))) + +(ert-deftest test-org-open-at-point/plain-link-inside () + "Test `org-open-at-point' from inside a plain link." + (test-org-open-at-point/goto-fixture) + ;; go inside the plain link + (goto-char 126) + (org-open-at-point) + ;; should now be in front of the header + (should (equal (point) 2))) + +(ert-deftest test-org-open-at-point/bracket-link-before () + "Test `org-open-at-point' from before a bracket link but in the same line." + (test-org-open-at-point/goto-fixture) + ;; go before the bracket link + (goto-char 83) + (org-open-at-point) + ;; should now be in front of the header + (should (equal (point) 2))) + +(ert-deftest test-org-open-at-point/plain-link-before () + "Test `org-open-at-point' from before a plain link but in the same line." + (test-org-open-at-point/goto-fixture) + ;; go before the plain link + (goto-char 124) + (org-open-at-point) + ;; should now be in front of the header + (should (equal (point) 2))) -- 1.7.10.4 [-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]
[-- Attachment #1.1: Type: text/plain, Size: 625 bytes --] In attachment is a patch making tests of the previous mail (id:"87wqvtrxcp.fsf@konixwork.incubateur.ens-lyon.fr") pass. It adjusts org-open-at-point to have plain links handled the same way bracket links are. It allows plain links to be followed if the cursor is before the link while still on the same line. I also got rid of dangling parenthesis as suggested by Nicolas. Personally, I think it is easier to read with dangling parens. Could you explain what is wrong with them? Thanks for your attention. -- Konubinix GPG Key : 7439106A Fingerprint: 5993 BE7A DA65 E2D9 06CE 5C36 75D2 3CED 7439 106A [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1.2: patch --] [-- Type: text/x-diff, Size: 2289 bytes --] From cd52bef7715ed551972ad5331fce50260cfcda50 Mon Sep 17 00:00:00 2001 From: Samuel Loury <konubinixweb@gmail.com> Date: Fri, 4 Jan 2013 14:01:43 +0100 Subject: [PATCH] Make org-open-at-point behave the same with plain links and with bracket links. * lisp/org.el (org-open-at-point): make org-open-at-point handle a plain link even if the cursor is before it TINYCHANGE --- lisp/org.el | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/lisp/org.el b/lisp/org.el index b0fcb58..5d75055 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -9911,12 +9911,36 @@ application the system uses for this file type." (throw 'match t)) (save-excursion - (let ((plinkpos (org-in-regexp org-plain-link-re))) - (when (or (org-in-regexp org-angle-link-re) - (and plinkpos (goto-char (car plinkpos)) - (save-match-data (not (looking-back "\\[\\["))))) - (setq type (match-string 1) - path (org-link-unescape (match-string 2))) + (when (or (org-in-regexp org-angle-link-re) + (let ((match (org-in-regexp org-plain-link-re))) + (and + ;; link at point is a plain link + match + ;; check that it is not of the form + ;; [[http://orgmode.org][Org]]Mode. in that + ;; case, if the cursor is on "Mode", then the + ;; string "http://orgmode.org][Org]]Mode" is + ;; recognized as a plain link while it should + ;; not be + (save-excursion + (progn + ;; go to the begining of the match, If we + ;; were in the special case, we should now + ;; be in a org-bracket-link-regexp + (goto-char (car match)) + (not + (org-in-regexp org-bracket-link-regexp)))))) + (let ((line_ending (save-excursion (end-of-line) + (point)))) + ;; if in a line before a plain link or a + ;; bracket link + (or + (re-search-forward org-plain-link-re + line_ending t) + (re-search-forward org-bracket-link-regexp + line_ending t)))) + (setq type (match-string 1) + path (org-link-unescape (match-string 2))) (throw 'match t)))) (save-excursion (when (org-in-regexp (org-re "\\(:[[:alnum:]_@#%:]+\\):[ \t]*$")) -- 1.7.10.4 [-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]
Hi Samuel, Samuel Loury <konubinix@gmail.com> writes: > In attachment is a patch making tests of the previous mail > (id:"87wqvtrxcp.fsf@konixwork.incubateur.ens-lyon.fr") pass. thanks for raising this issue again -- I agree with your point here, but I cannot apply the patch as it is too big to be considered a TINYCHANGE (way above 20 lines.) Would you consider assigning your copyright to the FSF? http://orgmode.org/cgit.cgi/org-mode.git/plain/request-assign-future.txt Thanks, -- Bastien
[-- Attachment #1: Type: text/plain, Size: 894 bytes --] Hi, I sent the request to assign@gnu.org 10 days ago and wait for the documents. By the way, I messed up with parens when I remove dangling ones so don't bother about the last [PATCH] mail. Bastien <bzg@altern.org> writes: > Hi Samuel, > > Samuel Loury <konubinix@gmail.com> writes: > >> In attachment is a patch making tests of the previous mail >> (id:"87wqvtrxcp.fsf@konixwork.incubateur.ens-lyon.fr") pass. > > thanks for raising this issue again -- I agree with your > point here, but I cannot apply the patch as it is too big > to be considered a TINYCHANGE (way above 20 lines.) > > Would you consider assigning your copyright to the FSF? > > http://orgmode.org/cgit.cgi/org-mode.git/plain/request-assign-future.txt > > Thanks, > > -- > Bastien -- Konubinix GPG Key : 7439106A Fingerprint: 5993 BE7A DA65 E2D9 06CE 5C36 75D2 3CED 7439 106A [-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]
Hi Samuel, Samuel Loury <konubinix@gmail.com> writes: > It adjusts org-open-at-point to have plain links handled the same way > bracket links are. It allows plain links to be followed if the cursor is > before the link while still on the same line. I applied a slightly modified version of your patch, condensing comments. See the change here: http://orgmode.org/cgit.cgi/org-mode.git/commit/?id=9964d8 > I also got rid of dangling parenthesis as suggested by > Nicolas. Personally, I think it is easier to read with dangling > parens. Could you explain what is wrong with them? It's easy to allow oneself to have one or two dangling parentheses when it clarifies code reading, but when you start using dangling parentheses, it's hard to know where to stop... in any case, more of a matter of convention + taste. Thanks, -- Bastien
[-- Attachment #1: Type: text/plain, Size: 1049 bytes --] Hi, Bastien <bzg@altern.org> writes: >> It adjusts org-open-at-point to have plain links handled the same way >> bracket links are. It allows plain links to be followed if the cursor is >> before the link while still on the same line. By the way, the documentation of org-open-at-point says: ╭──── │ If there is no link at point, this function will search forward up to │ the end of the current line. ╰──── Then the patch not only makes the behavior consistent but it makes the implementation do what the documentation says. > > I applied a slightly modified version of your patch, condensing > comments. See the change here: > > http://orgmode.org/cgit.cgi/org-mode.git/commit/?id=9964d8 > I like it. It passes the tests and looks good for me. > Thanks, I am sorry for not sending the full patch myself: I am still waiting for the documents indicating I can contribute to emacs. -- Konubinix GPG Key : 7439106A Fingerprint: 5993 BE7A DA65 E2D9 06CE 5C36 75D2 3CED 7439 106A [-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]
Hi Samuel,
Samuel Loury <konubinix@gmail.com> writes:
> I am sorry for not sending the full patch myself: I am still waiting for
> the documents indicating I can contribute to emacs.
No problem at all. I decided to go and accept patches from people
that did not confirm they got the FSF papers because merging 8.0
into Emacs trunk is not likely to happen to soon... So.
--
Bastien
[-- Attachment #1.1: Type: text/plain, Size: 629 bytes --] Hi, Bastien <bzg@altern.org> writes: > No problem at all. I decided to go and accept patches from people > that did not confirm they got the FSF papers because merging 8.0 > into Emacs trunk is not likely to happen to soon... So. Good news, I got the confirmation that my "assignment/disclaimer process with the FSF is currently complete". I do not know however if I have to do something to prove that in order to submit patches to this mailing list. I then propose in attachment a set of four tests testing the problem mentioned in this thread: the behavior of org-open-at-point in front of bracket links and plain links. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1.2: 0001-Test-the-org-open-at-point-function.patch --] [-- Type: text/x-diff, Size: 3292 bytes --] From 5bd8eb52cb047b5290300fe850fca894babf05ff Mon Sep 17 00:00:00 2001 From: Samuel Loury <konubinixweb@gmail.com> Date: Sat, 16 Mar 2013 20:12:42 +0100 Subject: [PATCH] Test the org-open-at-point function. * testing/examples/open-at-point.org: new file. * testing/lisp/test-org-open-at-point.el: new file. This tests only the function when inside or before bracket links and plain links. --- testing/examples/open-at-point.org | 8 +++++ testing/lisp/test-org-open-at-point.el | 61 ++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) create mode 100644 testing/examples/open-at-point.org create mode 100644 testing/lisp/test-org-open-at-point.el diff --git a/testing/examples/open-at-point.org b/testing/examples/open-at-point.org new file mode 100644 index 0000000..b3bb92d --- /dev/null +++ b/testing/examples/open-at-point.org @@ -0,0 +1,8 @@ + +* Header 1 + :PROPERTIES: + :ID: header1_with_great_id + :END: +* Header 2 + [[id:header1_with_great_id][Header 1]] + id:header1_with_great_id diff --git a/testing/lisp/test-org-open-at-point.el b/testing/lisp/test-org-open-at-point.el new file mode 100644 index 0000000..78724c8 --- /dev/null +++ b/testing/lisp/test-org-open-at-point.el @@ -0,0 +1,61 @@ +;;; test-org-open-at-point.el + +;; Copyright (c) Samuel Loury +;; Authors: Samuel Loury + +;; Released under the GNU General Public License version 3 +;; see: http://www.gnu.org/licenses/gpl-3.0.html + +;;;; Comments: + +;; Test for the org-open-at-point function + +;;; Code: + +(save-excursion + (set-buffer (get-buffer-create "test-org-open-at-point.el")) + (setq ly-here + (file-name-directory + (or load-file-name (buffer-file-name))))) + +(defun test-org-open-at-point/goto-fixture () + (find-file-other-window + (concat ly-here "../examples/open-at-point.org")) + (set-buffer "open-at-point.org")) + +(ert-deftest test-org-open-at-point/bracket-link-inside () + "Test `org-open-at-point' from inside a bracket link." + (test-org-open-at-point/goto-fixture) + ;; go inside the bracket link + (goto-char 113) + (org-open-at-point) + ;; should now be in front of the header + (should (equal (point) 2))) + +(ert-deftest test-org-open-at-point/plain-link-inside () + "Test `org-open-at-point' from inside a plain link." + (test-org-open-at-point/goto-fixture) + ;; go inside the plain link + (goto-char 126) + (org-open-at-point) + ;; should now be in front of the header + (should (equal (point) 2))) + +(ert-deftest test-org-open-at-point/bracket-link-before () + "Test `org-open-at-point' from before a bracket link but in the same line." + (test-org-open-at-point/goto-fixture) + ;; go before the bracket link + (goto-char 83) + (message "point %s" (point)) + (org-open-at-point) + ;; should now be in front of the header + (should (equal (point) 2))) + +(ert-deftest test-org-open-at-point/plain-link-before () + "Test `org-open-at-point' from before a plain link but in the same line." + (test-org-open-at-point/goto-fixture) + ;; go before the plain link + (goto-char 124) + (org-open-at-point) + ;; should now be in front of the header + (should (equal (point) 2))) -- 1.7.10.4 [-- Attachment #1.3: Type: text/plain, Size: 100 bytes --] -- Konubinix GPG Key : 7439106A Fingerprint: 5993 BE7A DA65 E2D9 06CE 5C36 75D2 3CED 7439 106A [-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]
Hi Samuel, Samuel Loury <konubinix@gmail.com> writes: > Bastien <bzg@altern.org> writes: > >> No problem at all. I decided to go and accept patches from people >> that did not confirm they got the FSF papers because merging 8.0 >> into Emacs trunk is not likely to happen to soon... So. > > Good news, I got the confirmation that my "assignment/disclaimer process > with the FSF is currently complete". I do not know however if I have to > do something to prove that in order to submit patches to this mailing > list. Great. Normally I should receive the confirmation myself, I will ping the copyright clerk if I don't receive it in a week or so. I have added you to the list -- welcome! http://orgmode.org/worg/org-contribute.html#contributors_with_fsf_papers > I then propose in attachment a set of four tests testing the problem > mentioned in this thread: the behavior of org-open-at-point in front of > bracket links and plain links. Applied, thanks. PS: The test suite is not (yet) included in GNU Emacs, so there is no need to provide an assignment for this, I will state this in Org's README and in Worg. -- Bastien