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

An attempt to fix #75 #76

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

An attempt to fix #75 #76

wants to merge 7 commits into from

Conversation

thblt
Copy link
Collaborator

@thblt thblt commented Feb 12, 2020

This removes (yay!) speed-commands code and replaces it with an equivalent (I hope) implementation using only outshine-define-key. I think it works, but I've never used speed commands, so I don't know.

  • This requires more testing than I've given it. To enable speed commands, (setq outshine-use-speed-commands t), then press p, n, + or - at the beginning of an header
  • Speed commands are cool.
  • @tj64 do you remember the reason speed commands were implemented the way they were? I don't think I'm breaking anything, but I'd rather be sure.

@thblt
Copy link
Collaborator Author

thblt commented Feb 12, 2020

This should interest people tracking #75 (and the reference in the title apparently didn't trigger a notification)

Copy link
Owner

@alphapapa alphapapa left a comment

Choose a reason for hiding this comment

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

This looks good to me--with the caveat that I don't use Outshine speed commands (I do use Org speed commands), so I'm probably not a good judge of functionality here. Thanks for your work on this, Thibault!

(and
outshine-use-speed-commands
(outline-on-heading-p)
(>= (point) (match-beginning 0))
Copy link
Owner

Choose a reason for hiding this comment

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

It's unclear at first glance where the match data is set that is used here, so it might be good to write this code to not depend on it. At least, it should probably be documented where the match data comes from and in what circumstances this function is intended to be called. :)


;;;;;;; Outline Navigation

(outshine-define-key outshine-mode-map (kbd "n") (lambda () (interactive) (outshine-speed-move-safe 'outline-next-visible-heading))
Copy link
Owner

Choose a reason for hiding this comment

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

I'd suggest either putting these lambdas on multiple lines to make them easier to read, or using a macro to define them more concisely. :)

(lambda () (interactive) (org-priority ?\ ))))
(outshine-run-speed-command-p))

(outshine-define-key outshine-mode-map (kbd "1") (lambda (nil) (interactive)
Copy link
Owner

Choose a reason for hiding this comment

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

This lambda formatting is...non-standard... :) (If you haven't tried it, aggressive-indent-mode works very well for Elisp buffers.)

@tj64
Copy link

tj64 commented Feb 18, 2020 via email

@thblt
Copy link
Collaborator Author

thblt commented Mar 31, 2020

Thanks for the review, and thanks a lot @tj64 for the explanations! On second thought, my solution is bad, and shouldn't be applied. The Correct Way in Emacs to do local keymaps (ie local to some text in the buffer) is with text properties, I'll look into it when I find the time.

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.

3 participants