alphapapa / outshine

Org-mode for non-Org buffers
GNU General Public License v2.0
212 stars 23 forks source link

outline-{promote, demote} does not seem to work well. #63

Open mahito1594 opened 5 years ago

mahito1594 commented 5 years ago

I'm very sorry for my poor English and for not being familiar with Emacs Lisp.

I think, outline-{promote, demote} does not work well with outshine. outshine.el defines outshine-change-level with the docstring

Workhorse for outline-demote and outline-promote.

but they are not defined in outshine.el. nor in outshine-org-cmds.el.

For example, for latex-mode, I set outshine-promotion-headings as follows:

(add-hook 'latex-mode-hook #'(lambda ()
                   (outshine-mode)
                               (setq outline-regexp latex-outline-regexp)
                               (setq outline-level #'latex-outline-level)
                   (setq outshine-promotion-headings
                     '("\\chapter" "\\section" "\\subsection"
                       "\\subsubsection" "\\paragraph" "\\subparagraph"))))

If I call outline-demote on \section{foo}, then I get \sectionn{foo} which is not expected.

I think, by redefine outline-{promote, demote} as in outline-magic.el, we get expected one. Isn't it?

Thank you,

alphapapa commented 5 years ago

Hi,

Your English is actually quite good. :)

  1. outline-promote and outline-demote are defined in outline.el which is included with Emacs.
  2. As part of the modernization work, we are trying to use as much existing outline and org functionality as possible. The implementation of outline-promote you linked to in outline-magic.el is different than the one in outline.el. The names of the functions are the same, so they conflict.
  3. I don't use LaTeX much, so I've never tried to use promotion/demotion commands on LaTeX headings. But it appears that the variable outline-heading-alist supports them. See the function outline-head-from-level, which appears to be roughly the outline counterpart of the equivalent functionality in outshine-change-heading.
  4. Due to the many changes to outline, outshine, and outline-magic over the years, I don't know where outline-magic stands in relation to current code. The readme doesn't explain exactly what it implements on top of outline or whether it's still necessary. My understanding was that outshine grew out of outline-magic. In any case, outline-magic is not a part of this project and we can't support it. But I don't think you need it to do what you're asking.

@thblt What do you think?

Thanks.

mahito1594 commented 5 years ago

Thank you for your polite advice! Your description on outshine is easy to understand.

I will look up outline-heading-alist and outline-head-from-level.

Thanks,

mahito1594 commented 5 years ago

Owing to @alphapapa, outline-promote and outline-demote work well, e.g., with following config:

;; with AUCTeX, see
;; https://www.gnu.org/software/auctex/
(add-hook 'LaTeX-mode-hook #'(lambda ()
                               (outshine-mode 1)
                               (setq outline-level #'LaTeX-outline-level)
                               (setq outline-regexp (LaTeX-outline-regexp t))
                               (setq outline-heading-alist
                                     (mapcar (lambda (x)
                                               (cons (concat "\\" (nth 0 x)) (nth 1 x)))
                                             LaTeX-section-list))))

Thank you for giving me great advice.

However, now I have problems on moving headings up or down.

First, M-S-<up> and M-S-<down> do not work. This is discussed on #48 and #46.

Another one is about Easy Menu. On outshine.el#L2316-L2318, there are outline-move-heading-up and outline-move-heading-down. I think they are not defined. Should we replace them with outline-move-subtree-{up, down}?

Regards,

alphapapa commented 5 years ago

Thank you for giving me great advice.

Well, you asked a good question in a good way, so you made it easy. I wish all users asked questions like you. :)

However, now I have problems on moving headings up or down. First, M-S- and M-S- do not work. This is discussed on #48 and #46.

As the question on #48 asks, is it only the keybinding that doesn't work, or is it the command itself that doesn't work?

I'm glad that the promotion and demotion commands work for you with LaTeX, but I don't know if commands that move headings and subtrees around will also work with LaTeX; if they are written to work with *-prefixed outline headings, they may not. outshine is designed as an extension of outline, and AFAIK outline is generally designed to work with *-prefixed headings. Some parts of it may work more flexibly, but some parts may not.

For any parts that don't, I'm not sure that outshine would be the best place to work around it. Ideally outline itself would be modified to be more flexible (although it's already pretty flexible, AFAIK).

Anyway, since I don't use LaTeX much, I'm mostly thinking out loud and rambling here. If there is anything to be done, it would probably need to be done by someone who actually uses LaTeX regularly.

mahito1594 commented 5 years ago

Well, you asked a good question in a good way, so you made it easy. I wish all users asked questions like you. :)

I'm glad to hear that.

As the question on #48 asks, is it only the keybinding that doesn't work, or is it the command itself that doesn't work?

The command itself works well. I try following 4 cases on a .tex file: (1) When I type M-S-<up>, I get following message:

Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
  outline-move-subtree-up()
  #f(compiled-function (&optional arg) (interactive "P") #<bytecode 0x43a188b9>)(nil)
  funcall-interactively(#f(compiled-function (&optional arg) (interactive "P") #<bytecode 0x43a188b9>) nil)
  call-interactively(#f(compiled-function (&optional arg) (interactive "P") #<bytecode 0x43a188b9>) nil nil)
  command-execute(#f(compiled-function (&optional arg) (interactive "P") #<bytecode 0x43a188b9>))

(2) When I call M-x outline-subtree-move-up, the command works perfectly. (3) When I click Move Heading Up (M-S-<up>) in Easy Menu, I get following message:

Debugger entered--Lisp error: (wrong-type-argument commandp outline-move-heading-up)
  call-interactively(outline-move-heading-up nil nil)
  command-execute(outline-move-heading-up)

(4) With following setting as below, the keybinding M-S-<up> works well.

(with-eval-after-load 'outshine
  (define-key outshine-mode-map (kbd "M-S-<up>") #'outline-move-subtree-up))

Also I install dkwf branch of yuhan0's fork, and try (1--3) as above, then (1) and (2) work correctly, but (3) fails. So, I guess this problem is due to outshine-define-key-with-fallback and to easy-menu-define.

I don't know if commands that move headings and subtrees around will also work with LaTeX;

AUCTeX --- a famous package for editing LaTeX documents --- supports outline features. The functions such as outline-{promote, demote}, outline-move-subtree-{up, down} work well in AUCTeX with outline-minor-mode. However, the default keybindigs of outline-minor-mode make my fingers get cramps. So outshine is a great help for me. Thanks for this excellent package.

dkrieger commented 5 years ago

I get inconsistent behavior when demoting in emacs-lisp-mode -- sometimes it properly demotes a subtree, other times it inserts an additional space after the semicolons (rather than an additional semicolon). I haven't been able to pin down what causes it to work/break intermittently.

alphapapa commented 5 years ago

@dkrieger I don't know why, but I've never tried to use outline or outshine to promote/demote headings in an elisp file. It never even occurred to me. So I don't know whether that functionality is considered supported, and if so, by which of the two packages. It also depends on how you're using commented outlines, i.e. are you using ones with asterisks like ;; ** for a level 2 heading, or ones with only semicolons, like ;;;; for a level 2 heading. I use the latter in elisp files, but there is no official way, and software support may vary.

thblt commented 5 years ago

@dkrieger Do errors appear with the functions themselves or with the M-S-<left> and M-S-<right> bindings?

valff commented 5 years ago

outline-promote and outline-demote works fine for me if I manually set outline-regexp to ;; \\*+. The value that is set by outshine contains a trailing space. So this space is inserted by outline-demote instead of inserting *.

My current workaround is to place this footer to each file:

;; Local Variables:
;; outline-regexp: ";; \\*+"
;; End:

@alphapapa Why is this trailing space needed? Can we get rid of it? And also why do we need to limit max level?

thblt commented 5 years ago

@valff What's the major mode?

valff commented 5 years ago

@thblt It's emacs-lisp-mode.

thblt commented 5 years ago

@valff On a quick test, I cannot reproduce the issue. Do you use the most recent version of outshine? If so, can you please provide instructions to reproduce from emacs -q? Thanks!

valff commented 5 years ago

@thblt Here is my snippet. Open it with emacs -q and follow instructions at the end.

;;; -*- lexical-binding: t -*-

(defvar bootstrap-version)
(let ((bootstrap-file
       (expand-file-name "straight/repos/straight.el/bootstrap.el"
                         user-emacs-directory))
      (bootstrap-version 5))
  (unless (file-exists-p bootstrap-file)
    (with-current-buffer
        (url-retrieve-synchronously
         (concat "https://raw.githubusercontent.com/"
                 "raxod502/straight.el/develop/install.el")
         'silent 'inhibit-cookies)
      (goto-char (point-max))
      (eval-print-last-sexp)))
  (load bootstrap-file nil 'nomessage))
(require 'straight)

(straight-use-package 'outshine)
(require 'outshine)
(require 'outline)

(global-set-key (kbd "<C-left>") 'outline-promote)
(global-set-key (kbd "<C-right>") 'outline-demote)

;; Run M-x eval-buffer
;; Run M-x outshine-mode
;; Run M-x eval-buffer (don't know why is it needed)
;; * One <- Try <C-right> on this few times
;; ** Two

Or the same result can be reproduced with outline-minor-mode only:

(outline-minor-mode +1)
(setq outline-regexp ";; [*]\\{1,8\\} "); it is what outshine sets for elisp, right?
(global-set-key (kbd "<C-left>") 'outline-promote)
(global-set-key (kbd "<C-right>") 'outline-demote)

;; * One <- Try <C-right> on this few times
;; ** Two

Clearly outline-promote/demote doesn't work well with the trailing space.

c0001 commented 5 years ago

@thblt Here is my snippet. Open it with emacs -q and follow instructions at the end. Clearly outline-promote/demote doesn't work well with the trailing space.

Same as what you mentioned, the trailing white-space was the 'demote' and 'promote' bug trace core, that the origin mechinsm of outline to re-generate the head level sign was using 'substring' function manipulate the outline-regexp matced group of the asterisk sequence, the trailing white space will be recognized as the 'level' sign char, thus any demote will mess as the scene.

I given the 'around' advice for outshine-calc-outline-regexp to shrink the trailing white space exp and redefined outshine-fontify-headlines with the corresponding adjustment fix this problem.

May I push one PR to this issue? Or does it conflict with the sake of designation of outshine utilities?

alphapapa commented 5 years ago

@valff Thanks for the test case. If there is a bug in outline-minor-mode, as that seems to indicate, that should be fixed in Emacs. Would you file a bug report with M-x report-emacs-bug and link it here please?

@c0001 Generally, we should avoid modifying variables defined in other packages, like outline-regexp, which is defined in Emacs's outline.el. And I think we should avoid putting language- or filetype-specific workarounds in outshine, because 1) the package is complicated enough as it is, and 2) the bugs ought to be fixed upstream where possible, and 3) it seems that people already look to outshine to do some things that are really provided by other packages. In other words, I think we should avoid working around bugs in outline or org in this package. I think outshine will do better in the long term if we try to make it simpler, not more complicated. I appreciate your offer to send a PR. Perhaps you could post your code in a comment here so we can see exactly what it does, and so other users can use it in their own configs. Thanks.

@thblt Does all that make sense to you? I feel like the intertwining of outline, outshine, etc. is very confusing at times, and I think we should try to untangle them when possible.

thblt commented 5 years ago

@valff Thanks a lot for the MWE, and thank you for your patience. By my tests, the following snippet also triggers weird behavior:

(outline-minor-mode +1)
(global-set-key (kbd "<C-left>") 'outline-promote)
(global-set-key (kbd "<C-right>") 'outline-demote)

(outline-minor-mode)

;; * One <- Try <C-right> on this few times
;; ** Two

The semicolon syntax is also broken:

;;; One <- Try <C-right> on this few times
;;;; Two
;;;; Two prime
;;;;; Three
;;;; Two again

Unlike your example, I don't set outline-regexp to simulate outshine behavior, so in my opinion the first thing that needs doing is getting the outline bug fixed. Notice I'm not saying there's no bug in outshine, but we shall tackle things in order.

thblt commented 5 years ago

@alphapapa As a side note, and for the outshine part of the bug:

Generally, we should avoid modifying variables defined in other packages, like outline-regexp, which is defined in Emacs's outline.el.

Outshine actually defines this variable and a few others it doesn't own, and “protects” them by restoring them to their original value when it's disabled. See the following bits:

https://github.com/alphapapa/outshine/blob/3e2aa4429789e33144b55214898540a6afec167d/outshine.el#L292-L295

https://github.com/alphapapa/outshine/blob/3e2aa4429789e33144b55214898540a6afec167d/outshine.el#L969-L970

https://github.com/alphapapa/outshine/blob/3e2aa4429789e33144b55214898540a6afec167d/outshine.el#L1029-L1030

c0001 commented 5 years ago

@c0001 Generally, we should avoid modifying variables defined in other packages, like outline-regexp, which is defined in Emacs's outline.el. And I think we should avoid putting language- or filetype-specific workarounds in outshine, because 1) the package is complicated enough as it is, and 2) the bugs ought to be fixed upstream where possible, and 3) it seems that people already look to outshine to do some things that are really provided by other packages. In other words, I think we should avoid working around bugs in outline or org in this package. I think outshine will do better in the long term if we try to make it simpler, not more complicated. I appreciate your offer to send a PR. Perhaps you could post your code in a comment here so we can see exactly what it does, and so other users can use it in their own configs. Thanks.

For the trailing whitespace bug fix for:

(defun temp/fix--outshine-advice-1 (orig-func &rest orig-args)
    (let ((rtn (apply orig-func orig-args)))
      (when (string-match-p " +$" rtn)
        (setq
         rtn
         (replace-regexp-in-string
          "\\( +\\)$" "" rtn)))
      rtn))

(advice-add 'outshine-calc-outline-regexp
             :around
             #'temp/fix--outshine-advice-1)

The trailing whitespace issue was mentioned as the previous post, if any confused for, that I post the raw outline-invent-heading function which invoked by outine-{demote,promote} feature here for declaring the crux for as:


(defun outline-invent-heading (head up)
  "Create a heading by using heading HEAD as a template.
When UP is non-nil, the created heading will be one level above.
Otherwise, it will be one level below."
  (save-match-data
    ;; Let's try to invent one by repeating or deleting the last char.
    (let ((new-head (if up (substring head 0 -1)
    >>>>>>>>>>>>>>>>>>>>>>>*_notice here_ 
                      (concat head (substring head -1)))))
      (if (string-match (concat "\\`\\(?:" outline-regexp "\\)")
                        new-head)
          ;; Why bother checking that it is indeed higher/lower level ?
          new-head
        ;; Didn't work, so ask what to do.
        (read-string (format-message "%s heading for `%s': "
                     (if up "Parent" "Demoted") head)
                     head nil nil t)))))

For referred face keywords fixing by altering the basic regexp generator:

(defun temp/fix--outshine-fontify-headlines (orig-func &rest orig-args)
  "Calculate heading regexps for font-lock mode."
  (let* ((outline-regexp (car orig-args))
         (outline-rgxp (substring outline-regexp 0 -7)) >>>>>> alter shrink counter
         (heading-1-regexp
          (format "%s%s%s%s%s%s"
                  outline-rgxp
                  "\\{1\\}" (format "[^%s]" outshine-regexp-base-char) "\\(.*" >>>>>> restruct font-lock face
                  (if outshine-fontify-whole-heading-line "\n?" "")
                  "\\)"))
         (heading-2-regexp
          (format "%s%s%s%s%s%s"
                  outline-rgxp
                  "\\{2\\}" (format "[^%s]" outshine-regexp-base-char) "\\(.*"
                  (if outshine-fontify-whole-heading-line "\n?" "")
                  "\\)"))
         (heading-3-regexp
          (format "%s%s%s%s%s%s"
                  outline-rgxp
                  "\\{3\\}" (format "[^%s]" outshine-regexp-base-char) "\\(.*"
                  (if outshine-fontify-whole-heading-line "\n?" "")
                  "\\)"))
         (heading-4-regexp
          (format "%s%s%s%s%s%s"
                  outline-rgxp
                  "\\{4\\}" (format "[^%s]" outshine-regexp-base-char) "\\(.*"
                  (if outshine-fontify-whole-heading-line "\n?" "")
                  "\\)"))
         (heading-5-regexp
          (format "%s%s%s%s%s%s"
                  outline-rgxp
                  "\\{5\\}" (format "[^%s]" outshine-regexp-base-char) "\\(.*"
                  (if outshine-fontify-whole-heading-line "\n?" "")
                  "\\)"))
         (heading-6-regexp
          (format "%s%s%s%s%s%s"
                  outline-rgxp
                  "\\{6\\}" (format "[^%s]" outshine-regexp-base-char) "\\(.*"
                  (if outshine-fontify-whole-heading-line "\n?" "")
                  "\\)"))
         (heading-7-regexp
          (format "%s%s%s%s%s%s"
                  outline-rgxp
                  "\\{7\\}" (format "[^%s]" outshine-regexp-base-char) "\\(.*"
                  (if outshine-fontify-whole-heading-line "\n?" "")
                  "\\)"))
         (heading-8-regexp
          (format "%s%s%s%s%s%s"
                  outline-rgxp
                  "\\{8\\}" (format "[^%s]" outshine-regexp-base-char) "\\(.*"
                  (if outshine-fontify-whole-heading-line "\n?" "")
                  "\\)"))
         (font-lock-new-keywords
          `((,heading-1-regexp 1 'outshine-level-1 t)
            (,heading-2-regexp 1 'outshine-level-2 t)
            (,heading-3-regexp 1 'outshine-level-3 t)
            (,heading-4-regexp 1 'outshine-level-4 t)
            (,heading-5-regexp 1 'outshine-level-5 t)
            (,heading-6-regexp 1 'outshine-level-6 t)
            (,heading-7-regexp 1 'outshine-level-7 t)
            (,heading-8-regexp 1 'outshine-level-8 t))))

    (add-to-list 'outshine-font-lock-keywords font-lock-new-keywords)
    (font-lock-add-keywords nil font-lock-new-keywords)
    (outshine-font-lock-flush)))

(advice-add 'outshine-fontify-headlines
            :around
            #'temp/fix--outshine-fontify-headlines)

Further more patching to outshine-set-outline-regexp-base

(defun outshine-set-outline-regexp-base ()
  "Return the actual outline-regexp-base."
  (if (and
       (not (outshine-modern-header-style-in-elisp-p))
       (eq major-mode 'emacs-lisp-mode))
      (progn
        (setq outshine-enforce-no-comment-padding-p t)
        (setq outshine-regexp-base
              outshine-oldschool-elisp-outline-regexp-base)
        (setq-local outshine-regexp-base-char ";")) ;adding local base-char setting preventing recursive face map
    (setq outshine-enforce-no-comment-padding-p nil)
    (setq outshine-regexp-base
          outshine-default-outline-regexp-base)
    (setq-local outshine-regexp-base-char
                (default-value 'outshine-regexp-base-char))))

There's no need to hacking with outline-mode but just pacthing the outshine itself only.

Thanks for reviewing -V-

alphapapa commented 5 years ago

Outshine actually defines this variable and a few others it doesn't own, and “protects” them by restoring them to their original value when it's disabled.

Yeah, and I think it would be good if we could avoid that. :) But fixing that would probably be quite an undertaking.

jdtsmith commented 2 years ago

This still doesn't work well, simply because outshine adds a final " " in outshine-calc-outline-regexp. If such a level is not yet seen in the buffer, outline-demote (via outline-invent-heading) takes the final character of the outline regexp as the character to repeat for demotion. So we intend # *** (demote) -> # ****, but outline-demote, using outshine's own computed outline-regexp, uses the final character for demotion (the space!), and we get # ***.

The fix is trivial: remove the final space from the end outshine-calc-outline-regexp, and change the -8 to -7 in the beginning of outshine-fontify-headlines.

Thanks for outshine mode, really great.