Make add blank overlays a custom feature, disabled by default. #17
Conversation
|
Thanks, I'll make a few suggestions as review comments. You should be able to use |
| @@ -413,6 +413,11 @@ is removed before converting back from Org to source-code." | |||
| :group 'outorg | |||
| :type 'boolean) | |||
|
|
|||
| (defcustom outorg-ensure-blank-line nil | |||
| "Non-nil means insert an overlay between text and blocks. | |||
There was a problem hiding this comment.
I think the docstring should focus on the feature as presented to the user rather than its underlying implementation. So, e.g. it need not mention that it uses overlays. Instead, it could simply be something like:
Always show a blank line before a source code block when editing.
The blank line is not added to the source buffer after editing.
There was a problem hiding this comment.
I agree. That sounds good.
| (defcustom outorg-ensure-blank-line nil | ||
| "Non-nil means insert an overlay between text and blocks. | ||
| This overlay displays a blank line. It is inserted in the editing buffer and | ||
| persists after the editing buffer is closed.") |
There was a problem hiding this comment.
What do you mean by persists after the editing buffer is closed? The blank line does not (and should not) persist, because it's an overlay, which is not part of the buffer's text content.
There was a problem hiding this comment.
Say you have an outshine file. I would edit it with outorg-edit-as-org. In the editing buffer the blank lines are there as expected. But when I leave the editing buffer with outorg-copy-edits-and-exit the overlays are still in the edited elisp buffer. This is what I meant. If they were only an in-editing convenience I would expect them to be removed after exiting.
There was a problem hiding this comment.
Maybe this should be changed as well. I don't think leaving the overlays is desirable.
There was a problem hiding this comment.
Say you have an outshine file.
Friendly point-of-order: there is no such thing as an Outshine file. :) Outshine is a minor mode that is compatible with outline-minor-mode and (at least to an extent) Org mode. I mention this because it's important to be clear about these terms and expectations.
I would edit it with
outorg-edit-as-org. In the editing buffer the blank lines are there as expected. But when I leave the editing buffer withoutorg-copy-edits-and-exitthe overlays are still in the edited elisp buffer. This is what I meant. If they were only an in-editing convenience I would expect them to be removed after exiting.
My understanding of the intended workflow is that, once you have called outorg-copy-edits-and-exit, you should not continue to edit the buffer created by Outorg. Instead you should call outorg-edit-as-org again. This mirrors the org-edit-special/org-edit-src-exit workflow.
Maybe this should be changed as well. I don't think leaving the overlays is desirable.
The Outorg buffer should be reset when outorg-edit-as-org is called, and an overlay left behind in that buffer shouldn't matter, because it shouldn't be used, anyway. Perhaps we should kill that buffer instead.
There was a problem hiding this comment.
Friendly point-of-order: there is no such thing as an Outshine file
You're right I was not precise. When I said outshine file I meant it informally as an elisp file whose comments are written so that they can be converted to an org file.
The Outorg buffer should be reset when outorg-edit-as-org is called, and an overlay left behind in that buffer shouldn't matter, because it shouldn't be used, anyway.
I don't think we should leave an overlay in the original buffer because we think it won't matter. I don't think we can be sure it won't matter. We have no idea what the user will do with that buffer. For me it did matter and it was unpleasantly unexpected.
Further I don't think we should assume the user is going to follow the intended workflow so strictly. With small edits, I just edit the elisp buffer itself and with significant edits I use outorg.
I maintain the only change to the buffer should be the edits.
Perhaps we should kill that buffer instead.
We definitely should not kill the buffer. I think this would be worse than leaving the overlays.
There was a problem hiding this comment.
You may be misunderstanding me. When I say we should kill the buffer, I mean the *outorg-edit-buffer* buffer. The user should not continue to use that buffer after calling outorg-copy-edits-and-exit, just like the user would not continue to edit the *Org Src* buffer after calling org-edit-src-exit. If the user wants to "edit as Outorg" again, he should call outorg-edit-as-org again.
There was a problem hiding this comment.
When I say we should kill the buffer, I mean the outorg-edit-buffer buffer.
I see, then that's a good idea. I am using edit-indirect instead of the buffer outorg provides so I didn't notice it wasn't killed. But yes, I agree it should be.
I can submit a separate pr to clean-up the overlays after editing if you want. They end up being copied over to the elisp buffer.
There was a problem hiding this comment.
When I say we should kill the buffer, I mean the outorg-edit-buffer buffer.
I see, then that's a good idea. I am using edit-indirect instead of the buffer outorg provides so I didn't notice it wasn't killed.
So all this time you've been reporting problems with Outorg, you've been using a different package? I don't understand what's going on here.
I can submit a separate pr to clean-up the overlays after editing if you want. They end up being copied over to the elisp buffer.
No, the overlays are not copied over to the elisp buffer.
Before going any further, please verify the behavior you are seeing happens with emacs -q and only Outorg loaded.
There was a problem hiding this comment.
So all this time you've been reporting problems with Outorg, you've been using a different package?
Just because I don't use the function outorg-edit-as-org to edit doesn't mean I don't use Outorg. I don't like that particular function. I use outorg-convert-to-org and outorg-convert-back-to-code. The value I get of outorg as a package is ability to convert from org and back. Regardless of how unconventionally I use this package, I have still provided valuable user feedback and seek to improve its main functionality.
No, the overlays are not copied over to the elisp buffer.
Ok. I double-checked. I was mistaken.
| (overlay-put (make-overlay (1- (point)) (1- (point))) | ||
| 'before-string "\n") | ||
| (when outorg-ensure-blank-line | ||
| (overlay-put (make-overlay (1- (point)) (1- (point))) |
There was a problem hiding this comment.
The feature could also be improved by only inserting a blank line when necessary, which would avoid showing an extra blank line when one is already present. e.g.:
;; Display blank line between text and block
(unless (looking-back "\n\n")
(overlay-put (make-overlay (1- (point)) (1- (point)))
'before-string "\n"))There was a problem hiding this comment.
I had considered this. Personally, this would be fine for my usage.
However, I still think we need to make this optional because even this is opinionated. I maintain that the default behavior should just be a vanilla conversion with no attempt at trying to guess how the user wants their blank lines displayed.
I would be fine with:
;; Display blank line between text and block
(when org-ensure-blank-line
(unless (looking-back "\n\n")
(overlay-put (make-overlay (1- (point)) (1- (point)))
'before-string "\n")))
I create customizable variable for the overlays.
Side note: what do you use to format fill lisp comments? Autofill mode doesn't fill elisp comments right.