Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

counsel.el: increase org-goto/org-agenda-headlines UI consistency #1324

Closed
wants to merge 8 commits into from

Conversation

@eigengrau
Copy link
Contributor

commented Nov 27, 2017

  • counsel.el (counsel-org-goto): obey new ‘counsel-org-goto-display-priority’
    (counsel-org-agenda-headlines): obey ‘counsel-org-goto-foo’ variables (except
    for ‘…-face-style’, ‘…-custom-faces’)
counsel.el: increase org-goto/org-agenda-headlines UI consistency
* counsel.el (counsel-org-goto): obey new ‘counsel-org-goto-display-priority’
(counsel-org-agenda-headlines): obey ‘counsel-org-goto-foo’ variables (except
for ‘…-face-style’, ‘…-custom-faces’)
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Nov 27, 2017

Thanks. I don't see your email on the copyright.list. Do you have an Emacs CA?

@eigengrau

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

Thanks. I don't see your email on the copyright.list. Do you have an Emacs CA?

It should be in there, I have an assignment (#1027005). I’m not sure which e-mail address I used though.

counsel.el Outdated
@@ -2669,7 +2669,8 @@ otherwise continue prompting for tags."
(fset 'org-set-tags store))))

(defcustom counsel-org-goto-display-style 'path
"The style for displaying headlines in `counsel-org-goto' functions.
"The style for displaying headlines in `counsel-org-goto' and \
`counsel-org-agenda-headlines' functions.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 27, 2017

Collaborator

Please follow the conventions outlined in (elisp) Documentation Tips, in particular keeping the first line of docstrings brief and to the point, e.g.

  "The style for displaying Org headlines.

This is used by functions `counsel-org-goto' and
`counsel-org-agenda-headlines'.

or similar. The basic idea is not to list call sites in the variable's summary. The same applies to the other docstrings in this patch.

counsel.el Outdated
?*))
(todo (nth 2 components))
(priority (nth 3 components))
(level (when (eq counsel-org-goto-display-style 'headline)

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 27, 2017

Collaborator

Since we care about the expression's value, it's more correct to use and in place of when here and in all corresponding cases below.

counsel.el Outdated
(make-string
(if org-odd-levels-only
(nth 1 components)
(nth 0 components))

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 27, 2017

Collaborator

I'd write this as (nth (if org-odd-levels-only 1 0) components), but maybe that's just me.

This comment has been minimized.

Copy link
@eigengrau

eigengrau Nov 27, 2017

Author Contributor

These lines were just reindented. I prefer to hold back on refactoring for a separate commit.

counsel.el Outdated
@@ -4107,7 +4123,10 @@ candidate."
level
todo
(if priority (format "[#%c]" priority))

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 27, 2017

Collaborator

Ditto re: preferring and over conditionals.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 27, 2017

Collaborator

Why not replace the surrounding (cl-remove-if 'null ...) with (delq nil ...) while we're at it?

counsel.el Outdated
(if path (mapconcat 'identity
(append path (list text))
counsel-org-goto-separator)
text)

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 27, 2017

Collaborator
(if path
    (mapconcat #'identity
               (append path (list text))
               counsel-org-goto-separator)
  text)

is the same as

(mapconcat #'identity
           (append path (list text))
           counsel-org-goto-separator))

This comment has been minimized.

Copy link
@eigengrau

eigengrau Nov 27, 2017

Author Contributor

Good point!

counsel.el Outdated
@@ -2796,7 +2805,8 @@ to custom."
(while start-pos
(let ((name (org-get-heading
(not counsel-org-goto-display-tags)
(not counsel-org-goto-display-todo)))
(not counsel-org-goto-display-todo)
(not counsel-org-goto-display-priority)))

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 27, 2017

Collaborator

org-get-heading only accepts two optional arguments in Org versions < 9.1.1, which was only released a couple of months ago.

This comment has been minimized.

Copy link
@eigengrau

eigengrau Nov 27, 2017

Author Contributor

Would you opt for leaving it out rather, or do a version comparison?

@eigengrau

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

I’m pushing some fix-ups with sloppier commit messages to be squashed if desired.

@eigengrau eigengrau force-pushed the eigengrau:master branch to 5d36c6b Nov 27, 2017

@eigengrau

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

If we were nit-picky, generalizing counsel-org-goto-…-Variables to another command that is not prefixed with counsel-org-goto, one might consider using counsel-org-headline-foo for the variable names instead. Would you prefer that, with the old names potentially defvaraliased to the new ones?

@eigengrau
Copy link
Contributor Author

left a comment

Generally, I find the when variants preferable. People seem to be somewhat divided on this. In line with other languages, I don’t see when as a signal that the return value is discarded. Rather, I tend to read and as a signal that only the truthiness of the return value matters. But if this style is used in the remainder of counsel it’s probably best to apply it.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Nov 28, 2017

@eigengrau

Would you opt for leaving it out rather, or do a version comparison?

I don't see a reason to leave it out, though feature detection is usually better than version comparison, especially with a package as unstable as Org. Unfortunately the function func-arity was only introduced in Emacs 26.1, so until then we can fall back to version comparison. How about

From dfa538f86beb56078df32cd904e139b52062ec45 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Tue, 28 Nov 2017 17:18:11 +0000
Subject: [PATCH] counsel.el: Fix org-get-heading invocation

(counsel--org-get-heading-args): New defun.
(counsel-org-goto--get-headlines): Use it.
Hoist argument calculation out of loop.
---
 counsel.el | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/counsel.el b/counsel.el
index 8bafff3..f51b022 100644
--- a/counsel.el
+++ b/counsel.el
@@ -2802,21 +2802,33 @@ counsel-org-goto-action
   "Go to headline in candidate X."
   (org-goto-marker-or-bmk (cdr x)))
 
+(defun counsel--org-get-heading-args ()
+  "Return list of arguments for `org-get-heading'.
+Try to return the right number of arguments for the current Org
+version.  Argument values are based on the
+`counsel-org-headline-display-*' user options."
+  (nbutlast (mapcar #'not (list counsel-org-headline-display-tags
+                                counsel-org-headline-display-todo
+                                counsel-org-headline-display-priority))
+            (if (if (fboundp 'func-arity)
+                    (< (cdr (func-arity #'org-get-heading)) 3)
+                  (version< org-version "9.1.1"))
+                1 0)))
+
 (defun counsel-org-goto--get-headlines ()
   "Get all headlines from the current org buffer."
   (save-excursion
     (let (entries
           start-pos
           stack
-          (stack-level 0))
+          (stack-level 0)
+          (heading-args (counsel--org-get-heading-args)))
       (goto-char (point-min))
       (setq start-pos (or (and (org-at-heading-p)
                                (point))
                           (outline-next-heading)))
       (while start-pos
-        (let ((name (org-get-heading
-                     (not counsel-org-headline-display-tags)
-                     (not counsel-org-headline-display-todo)))
+        (let ((name (apply #'org-get-heading heading-args))
               level)
           (search-forward " ")
           (setq level
-- 
2.15.0

Alternatively, we could go with a more Pythonic approach, such as

From d87557d9a020c5d9b1503faa3e94e65a42976872 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Tue, 28 Nov 2017 17:35:28 +0000
Subject: [PATCH] counsel.el: Fix org-get-heading invocation

(counsel-org-goto--get-headlines): Handle differing arity of
org-get-heading across Org versions. Hoist argument calculation out
of loop.
---
 counsel.el | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/counsel.el b/counsel.el
index 8bafff3..4f0080a 100644
--- a/counsel.el
+++ b/counsel.el
@@ -2808,15 +2808,19 @@ counsel-org-goto--get-headlines
     (let (entries
           start-pos
           stack
-          (stack-level 0))
+          (stack-level 0)
+          (args (mapcar #'not (list counsel-org-headline-display-tags
+                                    counsel-org-headline-display-todo
+                                    counsel-org-headline-display-priority))))
       (goto-char (point-min))
       (setq start-pos (or (and (org-at-heading-p)
                                (point))
                           (outline-next-heading)))
       (while start-pos
-        (let ((name (org-get-heading
-                     (not counsel-org-headline-display-tags)
-                     (not counsel-org-headline-display-todo)))
+        (let ((name (condition-case nil
+                        (apply #'org-get-heading args)
+                      (wrong-number-of-arguments
+                       (apply #'org-get-heading (nbutlast args)))))
               level)
           (search-forward " ")
           (setq level
-- 
2.15.0

This is more reliable than both func-arity and version comparison, but could potentially catch unrelated errors and/or require error handling in every loop iteration.

Either way I think this is an opportunity to hoist the argument calculation out of the loop.

I prefer to hold back on refactoring for a separate commit.

Sure, I wasn't suggesting a Tour-de-France commit; but I think it's perfectly acceptable for the same PR to include multiple relevant commits.

Even if you don't like my (nth (if org-odd-levels-only 1 0) components) suggestion, I still recommend replacing (cl-remove-if 'null ...) with (delq nil ...) in a commit of your choosing.

If we were nit-picky, generalizing counsel-org-goto-…-Variables to another command that is not prefixed with counsel-org-goto, one might consider using counsel-org-headline-foo for the variable names instead. Would you prefer that, with the old names potentially defvaraliased to the new ones?

I don't mind this, but it's @abo-abo 's call. I prefer define-obsolete-variable-alias to defvaralias; naming things is already hard enough without having multiple valid names floating around.

Generally, I find the when variants preferable. People seem to be somewhat divided on this. In line with other languages, I don’t see when as a signal that the return value is discarded. Rather, I tend to read and as a signal that only the truthiness of the return value matters. But if this style is used in the remainder of counsel it’s probably best to apply it.

Obviously there exist multiple opinions and tastes on this and many other coding styles, and mine carry little to no weight here. Nevertheless, there is no arguing that when is a control flow structure ("subject to a condition, do the following") and and is a Boolean operator ("return a function of the arguments"). The fact that they can often be used interchangeably is merely a result of Lisp being an expression-oriented language and the logical relation of conjunction and implication.

In this particular case, we are evaluating simple logical conditional expressions that can easily be expressed in terms of and, without the need for (even the possibility of) control flow. So why not do what we mean? Though this is by no means a universal idiom, it is certainly quite widespread in the Elisp world. The choice comes down to you and @abo-abo; I promise I won't continue bikeshedding this. :)

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Nov 29, 2017

It should be in there, I have an assignment (#1027005). I’m not sure which e-mail address I used though.

I can't check by assignment number, only by name and/or email. I can't find neither. There's only one possibility with the surname spelled differently (Rose).
In any case, it would be best if you get in touch with assign@gnu.org and have them update the email to the one you used on the Git commit.

counsel.el: Fix org-get-heading invocation
(counsel--org-get-heading-args): New defun.
(counsel-org-goto--get-headlines): Use it.
Hoist argument calculation out of loop.
@eigengrau

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

The copyright clerk was so kind to take care of the copyright.list entry. In the meantime I pushed Basil’s patch for the version guard logic.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 5, 2017

The copyright clerk was so kind to take care of the copyright.list entry. In the meantime I pushed Basil’s patch for the version guard logic.

Thanks. I see one entry that could be it, with email: mail at reusse dot net. It's your name, but the surname has "n" added to the end. Is that you? In any case, I would prefer if the email addresses on the commit message and on the list coincided. You can change either one, whichever you prefer.

@eigengrau

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

Thanks for checking. I’ll contact FSF again to see if they would update the e-mail address, and possibly get rid of that name typo.

@eigengrau

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2017

Also followed @basil-conto’s suggestion to use define-obsolete-variable-alias.

@eigengrau

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2017

So, hopefully, copyright.list should now list both e-mail addresses and the correct name.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 7, 2017

@eigengrau Good thing you didn't follow my Pythonic version guard suggestion as I just noticed it suffers from a significant bug by using nbutlast. :)

@eigengrau

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2017

@basil-conto, because nbutlast is an in-place operation and is iterated inside the scope of args? I just went for the first one assuming it reflected your preference. ;)

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 7, 2017

@eigengrau Sorry about the noise, I was actually right the first time. It works, but in an error-prone way, which only further supports my preference for the approach you went with. :)

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 7, 2017

So, hopefully, copyright.list should now list both e-mail addresses and the correct name.

It's funny, your surname now has an extra S in front of it. But both emails are there now; good enough for me to merge.

@abo-abo abo-abo closed this in ed7d6a0 Dec 7, 2017

abo-abo added a commit that referenced this pull request Dec 7, 2017
counsel.el: Fix org-get-heading invocation
(counsel--org-get-heading-args): New defun.
(counsel-org-goto--get-headlines): Use it.
Hoist argument calculation out of loop.

Fixes #1324
abo-abo added a commit that referenced this pull request Dec 7, 2017
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 7, 2017

Thanks, all.

@eigengrau

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2017

Cheers! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.