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

Generalize counsel-outline to handle major modes and display styles #1681

Closed
wants to merge 3 commits into from

Conversation

@ericdanan
Copy link
Contributor

commented Jul 25, 2018

Hello,

The first of the two commits is a simple fix making sure that counsel-outline-candidates only searches for outline-regexp at the beginning of a line (because from the docstring of outline-regexp, it does not necessarily start with ^).

The second commit is the one that generalizes counsel-outline, in two ways. First, to handle display/face styles as in counsel-org-goto. This is simply done by moving code from counsel-org-goto--get-headlines to counsel-outline-candidates and make the former call the latter. Note that the display/face style in counsel-outline is also controlled by the variables counsel-org-goto-display/face-style, perhaps that's not elegant and new variables should be introduced, I'm not sure.

Second, to handle major modes in which the outline title does not simply go from the end of outline-regexp to the end of the line. For instance, in markdown-mode, the title is iside the regexp, such as markdown or latex. This is done by introducing a variable counsel-outline-title storing the function to use, which can be locally bound in major mode hooks. There are instructions in the docstring of counsel-outline about how to set this up. I've also put there instructions for emacs-lisp-mode, because the default values of outline-regexp and outline-level set by this mode are not adequate for counsel-outline (by the way, the dir-local value in the swiper repository ìs better but still matches all lines starting with ( or the autoload cookie, which we don't want I think).

I think this approach is quite flexible but I'm not sure it's the best one because counsel-outline will not work out of the box for these modes. Alternative I thought of:

  • Putting the add-hook statements directly in counsel.el, but I'm not sure it's good practice.
  • Storing mode settings for counsel-outline (regexp, level function, title function) in an alist varaible, with a working default value, but I'm not sure it's good practice either for the regexp and level because it would overlook file/dir-local bindings of these variables and introduce an inconsistency between counsel-outline and the vanilla outline commands (eg outline-next-heading).

Note that after this generalization, counsel-org-goto is actually equivalent to counsel-outline after counsel-outline-title in org-mode-hook (actually there's one difference remaining in the action function, but that could accomodated). But counsel-org-goto works without setting anything, by let-binding counsel-outline-title. I didn't remove any existing function.

Finally, I implemeted a :preselect for counsel-outline, it seems useful to me to have the current heading preselected, but this is an independent thing.

If you're interesed in this PR, let me know of course if I should make changes.

Thanks for your attention.

ericdanan added 2 commits Jul 19, 2018
counsel.el (counsel-outline): only look for outline regexp at bol
From the docstring of `outline-regexp':

"Note that Outline mode only checks this regexp at the start of a
line, so the regexp need not (and usually does not) start with ‘^’."
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jul 25, 2018

Thanks, looks like a good amount of work.

Putting the add-hook statements directly in counsel.el, but I'm not sure it's good practice.

I don't like this approach. Packages should not do that, I think.

Storing mode settings for counsel-outline (regexp, level function, title function) in an alist varaible, with a working default value

I like this. counsel-outline could retrieve these settings from the alist and bind them dynamically for the duration. Then I don't see any disadvantage of this approach. Of course, the best way would be to simply reuse or improve the existing built-in outline-regexp and outline-level.

abo-abo added a commit that referenced this pull request Jul 25, 2018
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jul 25, 2018

(by the way, the dir-local value in the swiper repository ìs better but still matches all lines starting with ( or the autoload cookie, which we don't want I think).

Fixed, thanks.

@ericdanan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2018

I implemented storing mode settings in an alist variable (counsel-outline-settings). Let me know if that's not what you had in mind.

I also added the ivy-read action as a mode setting to cater for e.g. org-mode.

Do you think something should be done for the counsel-org-goto-display/face-stye variables? (renaming them or adding separate variables for counsel-outline)

@abo-abo abo-abo closed this in 3825ad5 Jul 26, 2018

abo-abo added a commit that referenced this pull request Jul 26, 2018
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jul 26, 2018

Thanks.

Do you think something should be done for the counsel-org-goto-display/face-stye variables?

They look OK to me, besides some obsolete aliases. If you see an improvement to be made here, please PR.

`counsel-outline-title'.
- `:action' is a function of one arg, a marker corresponding to the
beginning of the selected outline heading, performing

This comment has been minimized.

Copy link
@basil-conto

basil-conto Jul 26, 2018

Collaborator

Isn't this inaccurate? Neither counsel-outline-action nor counsel-org-goto-action take a marker; rather, they take a cons whose cdr is an integer or marker. I think it would be clearer to mention that :action corresponds directly to its ivy-read namesake.

This comment has been minimized.

Copy link
@ericdanan

ericdanan Jul 26, 2018

Author Contributor

Yes of course, sorry about that. I just modified the docstring.

current outline heading in latex-mode buffers. See
`counsel-outline-settings'."
;; `outline-regexp' is set by `latex-mode' (see
;; `LaTeX-outline-regexp') to match section macros, in which case we

This comment has been minimized.

Copy link
@basil-conto

basil-conto Jul 26, 2018

Collaborator

Did you mean to refer to the built-in variable latex-outline-regexp from lisp/textmodes/tex-mode.el, or are you intentionally referring to the external function LaTeX-outline-regexp from the AUCTeX package?

This comment has been minimized.

Copy link
@ericdanan

ericdanan Jul 26, 2018

Author Contributor

The function from AUCTeX. But actually it also work with the built-in mode, provided we use the variable latex-section-alist instead of LaTeX-section-alist when the latter is not bound. The outline-level function of the built-in mode is less sophisticated than that of AUCTeX (the former always assumes the first section level is \part whereas the latter adapts this do the document class), but it works. Just committed the simple change to support both variants.

current outline heading in emacs-lisp-mode buffers. See
`counsel-outline-settings'."
(if (looking-at ";;\\([;*]+\\)")
(length (match-string 1))

This comment has been minimized.

Copy link
@basil-conto

basil-conto Jul 26, 2018

Collaborator

This can be simplified as (- (match-end 1) (match-beginning 1)), but why is counsel-outline-level-emacs-lisp even needed? Can't we reuse lisp-outline-level, which is the default buffer-local value of outline-level in Lisp modes? How is the latter inadequate for counsel-outline?

This comment has been minimized.

Copy link
@ericdanan

ericdanan Jul 26, 2018

Author Contributor

Just made the simplification.

lisp-outline-level simply computes the length of the outline-regexp match, or sets the level to 1000 for the autoload cookie or lines starting with (. But to get the correct outine level (1 for ;;; or ;;*), we must discard the first two leading ; as well as any trailing space or tab. (And with the default value of outline-regexp, and additional issue would be that the first character of the title is part of the regexp).

level and title, respectively. For major modes where this is not
adequate or optimal, these variables can be rebound locally in
the major mode hook. Replacements are provided for some such
modes. To set them up, add this to your init file:"

This comment has been minimized.

Copy link
@basil-conto

basil-conto Jul 26, 2018

Collaborator

Should the last sentence be removed, or was it accidentally truncated?

This comment has been minimized.

Copy link
@ericdanan

ericdanan Jul 26, 2018

Author Contributor

Yes the whole paragraph should have been removed, done.

@ericdanan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

If you see an improvement to be made here, please PR.

I was just wondering whether they should be renamed to counsel-outline-display-style/face-style/custom-faces/path-separator since they now apply more generally to counsel-outline. In a sense counsel-org-goto is now an application of counsel-outline, but on the other hand many users may have customized counsel-org-goto and not use counsel-outline, so I don't know. It's fine with me like this in any case.

@ericdanan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

Ah, I thought I could continue to push commits to this branch but I don't see them, so will make another PR.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jul 26, 2018

@ericdanan Thanks for working on this and answering my questions. As a heads up, I intend to submit a PR building on your next one with some documentation copy-edit suggestions, which of course I would be grateful if you reviewed.

@ericdanan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

Sure!

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.