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

Add narrowing to consult-outline #277

Merged
merged 3 commits into from
Apr 18, 2021
Merged

Conversation

astoff
Copy link
Contributor

@astoff astoff commented Apr 18, 2021

Here's a sketch for the narrowing by outline level in consult-outline.

Some comments:

  • I don't like the consult--ouline-level function too much. It would be easy enough to compute the level when building the candidate list, but then I would have to change consult--location-candidate to allow including this extra datum in each candidate.
  • For now, I'm requiring outline in the body of consult--ouline-level. If we reimplemented code from outline to avoid requiring it, it wouldn't add that many lines of code. Still, I don't quite see why not (require 'outline) in the body of consult-outline itself. Then consult--outline-candidates could also be slightly simplified.
  • There's nothing forcing outline levels to be contiguous numbers (for instance, the smallest level in consult.el is 4). I think it's best not to be overly fancy when handling this. So I'm just shifting outline numbers so that Level ≤ 1 narrows to the smallest level, but otherwise leave any holes as they are. This also means it's impossible to reach the level of (defun in consult.el, because those have outline-level=16 a.k.a. Level ≤ 13.

@minad
Copy link
Owner

minad commented Apr 18, 2021

For now, I'm requiring outline in the body of consult--ouline-level. If we reimplemented code from outline to avoid requiring it, it wouldn't add that many lines of code. Still, I don't quite see why not (require 'outline) in the body of consult-outline itself. Then consult--outline-candidates could also be slightly simplified.

The local require style is a no-go. We can either require it globally, code the needed functions ourselves or create a separate consult-outline.el in case on wants to preserve lazy loading. Which code is simplified exactly if you require outline? As far as I understand both outline-level and outline-regexp may be present if outline is not loaded. Is that right?

I don't like the consult--ouline-level function too much. It would be easy enough to compute the level when building the candidate list, but then I would have to change consult--location-candidate to allow including this extra datum in each candidate.

Yes, it should be done when generating the candidate list for performance. You can use add-text-properties in consult-location-candidate and add an optional argument.

@@ -1935,10 +1935,14 @@ See `multi-occur' for the meaning of the arguments BUFS, REGEXP and NLINES."
(consult--forbid-minibuffer)
(let* ((line (line-number-at-pos (point-min) consult-line-numbers-widen))
(heading-regexp (concat "^\\(?:"
(if (boundp 'outline-regexp)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If outline-regexp is bound and nil (which shouldn't happen), there will be an error in the ensuing re-search-forward. So I changed to (or (bound-and-true-p ...`

consult.el Outdated
@@ -1950,6 +1954,7 @@ See `multi-occur' for the meaning of the arguments BUFS, REGEXP and NLINES."
'fontify)
(point-marker) line)
candidates)
(put-text-property 0 1 'consult-level (funcall level-function) (car candidates))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for the record, this is a tricky line, because it depends on the match data from the previous re-search-forward.

@astoff
Copy link
Contributor Author

astoff commented Apr 18, 2021

You can use add-text-properties in consult-location-candidate and add an optional argument.

Oops, I misread this. If you think it's a one off thing, we can keep as in the current version of the PR, otherwise I'll do as you suggest and add a &rest argument to consult--location-candidate.

@minad
Copy link
Owner

minad commented Apr 18, 2021

@astoff Yes, I meant add a rest argument to consult--location-candidate and use add-text-properties in consult--location-candidate instead.

@astoff
Copy link
Contributor Author

astoff commented Apr 18, 2021

Should be good now ;-)

@minad minad merged commit 83b11cb into minad:main Apr 18, 2021
@minad
Copy link
Owner

minad commented Apr 18, 2021

Thanks!

@astoff astoff deleted the outline-narrowing branch April 18, 2021 17:47
@minad minad mentioned this pull request Apr 21, 2021
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants