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

[WIP] Convert to a minor mode (Fix #18) #19

Open
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@thblt
Collaborator

thblt commented Jun 28, 2018

This is a WIP PR to convert the activation mechanism from a hacky override to a regular minor mode.

Calvin behaving

This PR is to track advancement, especially in case I give up before it's done (which may happen...), so anyone who feels like finishing the work could hopefully have half the job already done and the remaining part in a neat TODO list.

TODO/FIXME

Namespace isolation issues

These issues concern isolation = not touching anything not defined in outshine.el and not in the outshine- pseudo namespaces (these two definitions should be strictly equivalent)

  • outshine-set-local-outline-regexp-and-level sets outline-level[1]
  • outshine-set-local-outline-regexp-and-level sets outline-regexp[1]
  • outshine-set-local-outline-regexp-and-level sets outline-heading-end-regexp[1]

[1] I don't think this can be avoided, but commit "Save and restore outline- variables we manipulate" introduces a mechanism to revert theses variables to their original values when deactivating outshine.

  • IDEA Would it be useful to save the modified values of these variables and to restore them only if they weren't modified externally? (I don't think so, but maybe)

Purity issues

These issues concern purity:

  1. (require 'outshine) should have no effect except defining new symbols in the outshine- pseudo namespace.
  2. (outshine-minor-mode 1) should have no effect outside the current buffer.
  3. (progn (outshine-minor-mode 1) (outshine-minor-mode 0)) should have absolutely no effects.
  • The first invocation of (outshine-minor-mode 1) adds outshine--outline-minor-mode-hook on outline-minor-mode-hook. This is hard to avoid if we want sane behavior, and I don't consider it a bug. (The purpose of this function is to deactivate outshine when outline gets deactivated)
  • SEVERE What is the defadvice org-store-log-note for? It isn't local, but it shouldn't be activated by (require 'outshine).[2]
  • What is the series of (put SYMBOL :advertised-binding BINDING) useful for?

[2] I still have no idea what that's for, but I wrapped the advice in (when outshine-minor-mode and moved the defadvice to outshine--minor-mode-activate

Compatibility

  • Fix Outorg compatibility. Outorg calls outshine-hook-function.

Miscellaneous

  • Should Outshine work with outline-mode as well (the major mode)? In my tests, it doesn't work. (No: see #19 (comment))
  • Slightly unrelated: outshine does not really depend on Outorg, but it knows how to use it. Could we remove the dependency and replace it with (require 'outorg t) ... (when (featurep 'outorg) ...)?

thblt added some commits Jun 25, 2018

Delete functions already defined in outline.el
This deletes:

 - `outline-hide-other`
 - `outline-hide-sublevels`
 - `outline-move-subtree-up`
 - `outline-move-subtree-down`
 - `outline-promote`
 - `outline-demote`

These functions are already defined in outline.el, and Outshine should
not override them.
Bring outline- functions in the outshine- namespace
The following functions were renamed by adding outshine- before their
name (ie, they all match "^outshine-outline-")

 - `outline-cycle-emulate-tab`
 - `outline-change-level`
 - `outline-headings-list`
 - `outline-change-heading`
 - `outline-headings-atom`
 - `outline-cleanup-match`
 - `outline-static-level-p`
 - `outline-next-line`
 - `outline-cycle`
 - `outline-body-p`
 - `outline-body-visible-p`
 - `outline-subheadings-p`
 - `outline-subheadings-visible-p`
 - `outline-hide-more`
 - `outline-show-more`
Delete variable `outline-minor-mode-prefix`
This variable is defined in outline.el
Bring variable outline-promotion-headings into correct namespace
This renames `outline-promotion-headings` to
`outshine-outline-promotion-headings`.
Remove outline-mode-easy-bindings.el
 - The file was marked globally obsolete in a comment.
 - All it does is define/override functions and variables in the
   `outline-` pseudo-namespace.
 - These functions were merged in outshine.el and were renamed or
   removed by a previous commit.
Delete custom variable `outline-structedit-modifiers`
What this is meant to be is unclear, but there's no other occurence of
the name in any outshine elisp file.
Leave `outline-minor-mode-keymap` alone
Outshine used to work by overriding outline-minor-mode-map, which is a
Very Bad Thing.  This commit gives it its own separate keymap.
Create minor mode
This converts the `outshine-hook-function` into a minor mode.
Use defvar-local
This removes multiple invocations of (make-variable-buffer-local) from toplevel.
Don't auto-activate outline-mode
This looks like a good idea (to me at least), but it makes it very
easy to introduce confusing bugs.  Consider the obvious configuration
line:

(add-hook 'outline-minor-mode-hook 'outshine-minor-mode)

this actually triggers a Lisp stack overflow, since
outshine-minor-mode will call outline-minor-mode, which will run its
hooks, calling back `outshine-minor-mode`, and so on.  Since hooks are
run when the mode is activated *or* deactivated, there's no simple way
to make this predictible and safe.
@casouri

This comment has been minimized.

Show comment
Hide comment
@casouri

casouri Jul 3, 2018

I dont't think outshine needs to work with outline-mode. I think it only needs to work with outline-minor-mode since it serves primarily all the programming major modes.

casouri commented Jul 3, 2018

I dont't think outshine needs to work with outline-mode. I think it only needs to work with outline-minor-mode since it serves primarily all the programming major modes.

@thblt

This comment has been minimized.

Show comment
Hide comment
@thblt

thblt Jul 3, 2018

Collaborator

Thanks for the clarification, I marked the point as solved (and opened two new ones :)

Collaborator

thblt commented Jul 3, 2018

Thanks for the clarification, I marked the point as solved (and opened two new ones :)

@thblt

This comment has been minimized.

Show comment
Hide comment
@thblt

thblt Jul 3, 2018

Collaborator

Also, but just so Github creates explicit links between related things: this is an attempt at solving #18.

Collaborator

thblt commented Jul 3, 2018

Also, but just so Github creates explicit links between related things: this is an attempt at solving #18.

@casouri

This comment has been minimized.

Show comment
Hide comment
@casouri

casouri Jul 4, 2018

I have to express my admiration of you for refactoring such a large package...

casouri commented Jul 4, 2018

I have to express my admiration of you for refactoring such a large package...

@alphapapa

This comment has been minimized.

Show comment
Hide comment
@alphapapa

alphapapa Jul 4, 2018

Owner

I have to express my admiration of you for refactoring such a large package...

Indeed. While having no expectation that Thibault will necessarily complete the refactoring, or do so anytime soon, I am very grateful for his undertaking to start this work. It is not a glorious or very fun task, but it is much needed and will be much appreciated!

Owner

alphapapa commented Jul 4, 2018

I have to express my admiration of you for refactoring such a large package...

Indeed. While having no expectation that Thibault will necessarily complete the refactoring, or do so anytime soon, I am very grateful for his undertaking to start this work. It is not a glorious or very fun task, but it is much needed and will be much appreciated!

Show outdated Hide outdated outshine.el
@alphapapa

This all looks amazing! Would you be interested in being a co-maintainer on the outshine packages? There would be no obligation on your part, of course, but you clearly know what you're doing, and I would welcome the help in general.

Show outdated Hide outdated outshine.el
Show outdated Hide outdated outshine.el
Show outdated Hide outdated outshine.el
Show outdated Hide outdated outshine.el
Show outdated Hide outdated outshine.el
Show outdated Hide outdated outshine.el
Don't (error), activate
Third attempt of solving the problem of what to do whenever the user
tries to activate Outshine when Outline isn't activated.  Just
calling (outline-minor-mode 1) enters an endless hook
loop (outline-minor-mode-hook calls outshine which calls
outline-minor-mode which runs its hook etc).  (error)ing raises error
in strange circumstances, like changing the major mode.  This seems to work.
@thblt

This comment has been minimized.

Show comment
Hide comment
@thblt

thblt Jul 5, 2018

Collaborator

@casouri @alphapapa Thanks a lot for the kind words, much appreciated :)

@alphapapa I'd be happy to assist you in maintaining outshine if you need a hand, of course!

Collaborator

thblt commented Jul 5, 2018

@casouri @alphapapa Thanks a lot for the kind words, much appreciated :)

@alphapapa I'd be happy to assist you in maintaining outshine if you need a hand, of course!

@JohnLunzer

This comment has been minimized.

Show comment
Hide comment
@JohnLunzer

JohnLunzer Aug 22, 2018

Is there anything holding this PR back from being merged with master? Just thought I'd poke at it.

JohnLunzer commented Aug 22, 2018

Is there anything holding this PR back from being merged with master? Just thought I'd poke at it.

@alphapapa

This comment has been minimized.

Show comment
Hide comment
@alphapapa

alphapapa Aug 22, 2018

Owner

Well, it's a major change, and there are still some unchecked boxes on Thibault's to-do lists up there. I expect that he will say when he thinks it's ready to merge. Feel free to test the branch and let us know how it works for you.

Owner

alphapapa commented Aug 22, 2018

Well, it's a major change, and there are still some unchecked boxes on Thibault's to-do lists up there. I expect that he will say when he thinks it's ready to merge. Feel free to test the branch and let us know how it works for you.

@abradd

This comment has been minimized.

Show comment
Hide comment
@abradd

abradd Oct 11, 2018

It seems that when outshine is loaded, but the minor mode isn't running in an org-note buffer, the capture of the note doesn't complete. Pressing C-c C-c or C-c C-k has no effect. I noticed that there is advice for `org-store-log-note' even when the outshine-minor-mode hasn't been invoked in the note buffer. I don't know much about advising functions so I have no idea if this is expected behaviour.

When debugging it seems that calling C-c C-c in the note buffer doesn't make it past the advising function to `org-store-log-note' when outshine-minor-mode isn't enabled. If i enable outshine-minor-mode in the note buffer, C-c C-c correctly parses the note.

I'm running outshine revision c8a2c7e, org mode from master 2fda33bfe in emacs 27.0.50.

abradd commented Oct 11, 2018

It seems that when outshine is loaded, but the minor mode isn't running in an org-note buffer, the capture of the note doesn't complete. Pressing C-c C-c or C-c C-k has no effect. I noticed that there is advice for `org-store-log-note' even when the outshine-minor-mode hasn't been invoked in the note buffer. I don't know much about advising functions so I have no idea if this is expected behaviour.

When debugging it seems that calling C-c C-c in the note buffer doesn't make it past the advising function to `org-store-log-note' when outshine-minor-mode isn't enabled. If i enable outshine-minor-mode in the note buffer, C-c C-c correctly parses the note.

I'm running outshine revision c8a2c7e, org mode from master 2fda33bfe in emacs 27.0.50.

@alphapapa

This comment has been minimized.

Show comment
Hide comment
@alphapapa

alphapapa Oct 12, 2018

Owner

@abradd Thanks for reporting that. We should only advise that function when the minor mode is enabled, and preferably only locally, within the buffers it's enabled in.

Owner

alphapapa commented Oct 12, 2018

@abradd Thanks for reporting that. We should only advise that function when the minor mode is enabled, and preferably only locally, within the buffers it's enabled in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment