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

Simplify and improve counsel processes #1483

Closed
wants to merge 8 commits into from

Conversation

@basil-conto
Copy link
Collaborator

commented Mar 5, 2018

Summary

(counsel--async-filter): Simplify. Fix time comparison for Emacs < 24.3.
(counsel-delete-process): Take optional process name argument.
(counsel--async-command): Allow specifying process name, and default sentinel and filter functions via new :default argument value.
(counsel--async-sentinel, counsel-cmd-to-dired, counsel--gg-sentinel): Simplify. Inspect process-status and process-exit-status instead of user-facing description.
(counsel--gg-candidates, counsel--gg-count): Reuse counsel--async-command.

These should all be backward compatible, but extend the APIs of counsel-delete-process and counsel--async-command. Please see each commit individually for details.

Discussion

  • counsel--async-command could have taken mandatory sentinel and filter arguments to allow the distinction between nil meaning internal-default-process-{sentinel,filter} and counsel--async-{sentinel,filter}, respectively, but it has already been documented in the manual as requiring only a single argument.
  • internal-default-process-{sentinel,filter} were only added in Emacs 24.4 and are non-trivial to backport, hence the new :default argument to counsel--async-command.
  • Is it better to duplicate counsel-delete-process and counsel--async-command than extend their argument lists?

@basil-conto basil-conto force-pushed the basil-conto:blc/proc branch 3 times, most recently Mar 5, 2018

counsel.el Outdated

(defun counsel-set-async-exit-code (cmd number str)
"For CMD, associate NUMBER exit code with STR."
"Associate NUMBER exit code with STR for CMD."

This comment has been minimized.

Copy link
@abo-abo

abo-abo Mar 5, 2018

Owner

The idea here was to phrase the doc in a way that avoids checkdoc warning - args out of sequence.

@basil-conto basil-conto force-pushed the basil-conto:blc/proc branch Mar 5, 2018

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 5, 2018

The idea here was to phrase the doc in a way that avoids checkdoc warning - args out of sequence.

Ah, didn't realise checkdoc was so stingy :) - reverted.


As regards adding a :default argument to counsel--async-command - I notice that the only two places using it (counsel--gg-candidates and counsel--gg-count), i.e. the only two places that previously specified no process filter, seem to work even when they are given counsel--async-filter. Should I revert the :default argument business as well and allow counsel--gg-candidates and counsel--gg-count to be given counsel--async-filter?

Pros:

  • counsel--async-command remains almost as simple as it was before this PR, bar the new name argument.

Cons:

  • counsel--async-command remains less flexible, as it can't be used to specify the default internal filter/sentinel functions.

@basil-conto basil-conto force-pushed the basil-conto:blc/proc branch Mar 5, 2018

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 5, 2018

Yet another option which avoids both messing with the sentinel/filter arguments of counsel--async-command, and assigning counsel--async-filter to counsel--gg-candidates and counsel--gg-count, is to have counsel--async-command return the process object so that its callers can modify it to their heart's content. I think this affords the cleanest refactor whilst still increasing the flexibility of counsel--async-command.

WDYT?

Note that, even with this approach, the question of whether to give counsel--async-filter to counsel--gg-candidates and counsel--gg-count still remains.

counsel.el Outdated
@@ -132,113 +132,100 @@ descriptions.")
(defvar counsel-async-ignore-re nil
"Regexp matching candidates to ignore in `counsel--async-filter'.")

(defun counsel--async-command (cmd &optional process-sentinel process-filter)
(defun counsel-delete-process (&optional name)

This comment has been minimized.

Copy link
@abo-abo

abo-abo Mar 5, 2018

Owner

Please move this function back to the original place, so that it's easy to see what changed in the diff. Moving functions around is fine, but only if their content doesn't change.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Mar 6, 2018

Author Collaborator

No problem. Should I also move the function to before its first call site in a separate commit, or just leave it put?

This comment has been minimized.

Copy link
@abo-abo

abo-abo Mar 6, 2018

Owner

As you prefer. There won't be a compiler warning if you leave it put. But it might also be nice to group the API part of counsel (to which counsel-delete-process belongs) separately from the adaptations part (e.g. counsel-ag). Looking at it now, this is the case already, so it's fine not to move it.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Mar 6, 2018

Author Collaborator

OK, I'll see if I find anything which can be reordered after I refactor the other points.

counsel.el Outdated

(defcustom counsel-async-filter-update-time 500000
"The amount of time in microseconds to wait until updating
`counsel--async-filter'."
:type 'integer
:group 'ivy)

(autoload 'encode-time-value "time-date")

This comment has been minimized.

Copy link
@abo-abo

abo-abo Mar 5, 2018

Owner

I would prefer a require instead of autoload, since the likelihood of using counsel without calling counsel--async-filter is low.

Another issue is that encode-time-value is obsolete as of 25.1. A quick look at the doc doesn't suggest an alternative, strange.

Since your commit says that this change is relevant to Emacs < 24.3., maybe it makes sense to put Emacs >= 24.3 as the minimal requirement for Ivy. If I recall correctly, 24.3 is the most common one, 24.1 and 24.2 are rare.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Mar 6, 2018

Author Collaborator

I would prefer a require instead of autoload, since the likelihood of using counsel without calling counsel--async-filter is low.

OK, but... (see below)

Another issue is that encode-time-value is obsolete as of 25.1. A quick look at the doc doesn't suggest an alternative, strange.

Another issue is that encode-time-value is obsolete as of 25.1. A quick look at the doc doesn't suggest an alternative, strange.

Good catch.

Since your commit says that this change is relevant to Emacs < 24.3., maybe it makes sense to put Emacs >= 24.3 as the minimal requirement for Ivy. If I recall correctly, 24.3 is the most common one, 24.1 and 24.2 are rare.

I'm not against raising the minimum required Emacs version, but (a) it is not necessary for this particular case; and (b) perhaps it should be discussed in a dedicated issue.

I only used encode-time-value because I thought it was slightly less hacky than writing (list 0 0 counsel-async-filter-update-time). What changed in Emacs 24.3 is a fourth picoseconds element was added to time values, so the only thing needed for backward compatibility is dropping the fourth time value element from (list 0 0 counsel-async-filter-update-time 0).

This comment has been minimized.

Copy link
@basil-conto

basil-conto Mar 6, 2018

Author Collaborator

Perhaps a less hacky way of doing (list 0 0 counsel-async-filter-update-time) is (seconds-to-time (* 1e-06 counsel-async-filter-update-time)).

Honestly, I think it was sub-optimal to choose microseconds as a unit in the first place, but oh well.

@@ -1079,7 +1060,7 @@ INITIAL-INPUT can be given as the initial minibuffer input."
(defvar counsel-dired-listing-switches "-alh"
"Switches passed to `ls' for `counsel-cmd-to-dired'.")

(defun counsel-cmd-to-dired (full-cmd &optional process-filter)
(defun counsel-cmd-to-dired (full-cmd &optional filter)

This comment has been minimized.

Copy link
@abo-abo

abo-abo Mar 5, 2018

Owner

Would be nice to split off the straightforward changes like process-status from the other changes, much easier to review smaller pieces.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Mar 6, 2018

Author Collaborator

OK.

abo-abo added a commit that referenced this pull request Mar 5, 2018
Minor touch-ups
counsel.el (counsel--async-exit-code-plist)
(counsel-async-ignore-re): Fix docstring.
(counsel-locate-action-extern): Reword docstring and reindent.
(counsel-locate): Simplify default action.
ivy.el (ivy-magic-slash-non-match-action):
Revert erroneous function-quote.

Re #1483
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Mar 5, 2018

I think this affords the cleanest refactor whilst still increasing the flexibility of counsel--async-command.

Let's go with that.

(with-current-buffer (process-buffer process)
(goto-char (point-min))
(setq size (- (buffer-size) (forward-line (buffer-size))))

This comment has been minimized.

Copy link
@abo-abo

abo-abo Mar 5, 2018

Owner

This code looks a bit like a hack, but it's actually a fast way to count the lines in a buffer. Look at the code of count-lines - it's a bunch of Elisp code with regexes, loops, narrowing etc - looks expensive to me. I suggest to keep the code, maybe extract it to counsel-count-lines so that it looks less hacky.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Mar 6, 2018

Author Collaborator

it's actually a fast way to count the lines in a buffer.

I know, I recognised the algorithm from the source of count-lines. :)

Look at the code of count-lines - it's a bunch of Elisp code with regexes, loops, narrowing etc - looks expensive to me.

Actually, the only overhead involved in calling count-lines is due to save-excursion, save-restriction and narrow-to-region. The complicated loops and regexps only take effect when selective-display is enabled buffer-locally, which is never the case for counsel process buffers.

I suggest to keep the code, maybe extract it to counsel-count-lines so that it looks less hacky.

In descending order of personal preference:

  1. Call count-lines directly.
  2. Add counsel-count-lines.
  3. Add a descriptive comment to the current filter code.

This comment has been minimized.

Copy link
@basil-conto

basil-conto Mar 6, 2018

Author Collaborator

By the way, by far the fastest (and hackiest) way to count lines is the following: https://emacs.stackexchange.com/a/3822/15748.

But it comes with too many caveats to be reliably useful.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Mar 6, 2018

Let me know if the PR rework is too much work, I can slice it up into smaller commits myself in that case.
This contribution is definitely valuable, but it touches a lot of often used code at once, so I want to really make sure we don't overlook the important details behind the big diff.

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 6, 2018

Let me know if the PR rework is too much work, I can slice it up into smaller commits myself in that case.
This contribution is definitely valuable, but it touches a lot of often used code at once, so I want to really make sure we don't overlook the important details behind the big diff.

100% agreed. I'm sorry both for the slight delay in getting back to this and the initial megadump. If you don't mind waiting another day I'll try to have a cleaner set of commits with the discussed refactor ready by tomorrow. Either way, thanks for the quick feedback!

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Mar 6, 2018

100% agreed. I'm sorry both for the slight delay in getting back to this and the initial megadump. If you don't mind waiting another day I'll try to have a cleaner set of commits with the discussed refactor ready by tomorrow. Either way, thanks for the quick feedback!

Thanks, it's no trouble at all, and no urgency either. I'm just trying to avoid the situation of a promising PR going stale because the maintainer is asking the contributor to jump through too many hoops :)

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 6, 2018

I'm just trying to avoid the situation of a promising PR going stale because the maintainer is asking the contributor to jump through too many hoops :)

Don't worry, you're still shy of the number of hoops I throw myself through anyway. :)

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 6, 2018

I think this affords the cleanest refactor whilst still increasing the flexibility of counsel--async-command.

Let's go with that.

Do you have any thoughts on whether to add counsel--async-filter to counsel--gg-candidates and counsel--gg-count, or is this not the time to worry about that?

@basil-conto basil-conto force-pushed the basil-conto:blc/proc branch 2 times, most recently Mar 7, 2018

basil-conto added 8 commits Mar 7, 2018
counsel.el: Make process sentinels more robust
(counsel--async-sentinel, counsel-cmd-to-dired)
(counsel--gg-sentinel, counsel--gg-count): Inspect process-status
and process-exit-status instead of user-facing description.
counsel.el (counsel--async-filter): Fix & simplify
The fourth element in (SEC-HIGH SEC-LOW MICROSEC PICOSEC) was only
added in 24.3.
counsel.el: Call gtk-launch asynchronously
(counsel-linux-app-action-default, counsel-linux-app-action-file):
Make asynchronous.
counsel.el: Minor simplifications
(counsel--gg-count, counsel-linux-app-action-open-desktop):
Simplify logic.
counsel.el: Generalise async counsel processes
(counsel-delete-process): Take optional process name argument.
(counsel--async-command): Use it, adding similar optional process
name argument.  Return process object.  Simplify.
(counsel--gg-candidates, counsel--gg-count):
Reuse counsel--async-command.
(counsel--gg-count-sentinel):
New defun extracting lambda sentinel from counsel--gg-count.
(counsel--gg-count): Use it.

@basil-conto basil-conto force-pushed the basil-conto:blc/proc branch to 638a1a3 Mar 7, 2018

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 7, 2018

@abo-abo I've split the PR into more atomic commits (and found a couple of bugs in the process :).

The simplification of counsel--async-sentinel is done in three installments, in an attempt to aid review. If you use Magit, I recommend setting magit-diff-refine-hunk to t, as this is a vast improvement over GitHub's diff refinement. I reckon VC/diff-mode has an equivalent option (diff-auto-refine-mode?), if you use that. After reviewing, you should obviously feel free to squash/reword commits however you prefer.

Please also go through my replies to your inline and sometimes hidden review comments, in case you still don't agree with one of my proposed approaches.

Thanks.

@abo-abo abo-abo closed this in 1c64675 Mar 8, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Mar 8, 2018

Thanks.
I've removed one commit - counsel-linux-app doesn't work on my system with it.
I've also added one commit, string-to-number should be used in place of read.

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 9, 2018

Thanks!

counsel-linux-app doesn't work on my system with it.

Same here, but that seems very strange to me...

string-to-number should be used in place of read

Why build a string out of the entire buffer's contents only to pass it to string-to-number when you can read buffer contents directly? The only reason I can think of is if you expect the buffer contents to not always be a number.

@basil-conto basil-conto deleted the basil-conto:blc/proc branch Mar 9, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Mar 9, 2018

Why build a string out of the entire buffer's contents only to pass it to string-to-number when you can read buffer contents directly? The only reason I can think of is if you expect the buffer contents to not always be a number.

OK, my impression was that buffer-string was cheap in Elisp, storing only two indices into the actual buffer string object. Looking at the source code, it appears not to be the case. So indeed read might be the way to go. With some wrapper to give us 0 on error:

(condition-case nil
    (+ (read (current-buffer)))
  (error 0))
@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 9, 2018

If you want to save some typing, you can also write

(or (ignore-errors (+ (read (current-buffer))))
    0)

but I don't think it's worth the trouble.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Mar 9, 2018

I just realized that in this case all buffer-string holds is a single line with a number in it. So it's actually cheap to call it. Then I'd like to leave it as is, since (string-to-number (buffer-string)) conveys better what we want to do with the data.

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 9, 2018

Yeah, exactly.

basil-conto added a commit to basil-conto/swiper that referenced this pull request Apr 24, 2018
counsel.el: Call gtk-launch asynchronously
(counsel-linux-app-action-default, counsel-linux-app-action-file):
Make asynchronous.

Re: abo-abo#1483
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.