Re: [O] [PATCH] Fix uncaught error when trying to open a link at point
Hi, Bastien b...@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. From 5bd8eb52cb047b5290300fe850fca894babf05ff Mon Sep 17 00:00:00 2001 From: Samuel Loury konubinix...@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 000..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 000..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 -- Konubinix GPG Key: 7439106A Fingerprint: 5993 BE7A DA65 E2D9 06CE 5C36 75D2 3CED 7439 106A pgpJOvuA5BCP_.pgp Description: PGP signature
Re: [O] [PATCH] Fix uncaught error when trying to open a link at point
Hi Samuel, Samuel Loury konubi...@gmail.com writes: Bastien b...@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
Re: [O] [PATCH] Fix uncaught error when trying to open a link at point
Hi Samuel, Samuel Loury konubi...@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
Re: [O] [PATCH] Fix uncaught error when trying to open a link at point
Hi Samuel, Samuel Loury konubi...@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
Re: [O] [PATCH] Fix uncaught error when trying to open a link at point
Hi, I sent the request to ass...@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 b...@altern.org writes: Hi Samuel, Samuel Loury konubi...@gmail.com writes: In attachment is a patch making tests of the previous mail (id:87wqvtrxcp@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 pgppSQwmpU_si.pgp Description: PGP signature
Re: [O] [PATCH] Fix uncaught error when trying to open a link at point
Hi and thanks for paying attention to my patch. Bastien b...@altern.org writes: I allowed myself to fix this, with a somewhat smaller patch: http://orgmode.org/cgit.cgi/org-mode.git/commit/?h=maintid=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 From d9df74a72962020c3e695be0ad9a8646a42ee9de Mon Sep 17 00:00:00 2001 From: Samuel Loury konubinix...@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 000..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 000..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: + + +;;; 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
Re: [O] [PATCH] Fix uncaught error when trying to open a link at point
In attachment is a patch making tests of the previous mail (id:87wqvtrxcp@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 From cd52bef7715ed551972ad5331fce50260cfcda50 Mon Sep 17 00:00:00 2001 From: Samuel Loury konubinix...@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 pgp7aKdFT5b5x.pgp Description: PGP signature
Re: [O] [PATCH] Fix uncaught error when trying to open a link at point
Hi Samuel, Samuel Loury konubi...@gmail.com writes: In attachment is a patch making tests of the previous mail (id:87wqvtrxcp@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
Re: [O] [PATCH] Fix uncaught error when trying to open a link at point
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=maintid=14ffe2 Thanks Samuel for the patch and for reporting this problem! -- Bastien
Re: [O] [PATCH] Fix uncaught error when trying to open a link at point
Hello, Samuel Loury konubi...@gmail.com writes: From 0e31213fa486f7fcfe1c2b7037689df077a39fce Mon Sep 17 00:00:00 2001 From: Samuel Loury samuel.lo...@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
