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.

Sign up for GitHub

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

Jump to bottom

Weird behavior when org-adapt-indentation is set #144

Closed
hammerfunctor opened this issue Jul 10, 2023 · 27 comments
Closed

Weird behavior when org-adapt-indentation is set #144

hammerfunctor opened this issue Jul 10, 2023 · 27 comments

Comments

@hammerfunctor
Copy link

hammerfunctor commented Jul 10, 2023 •
edited
Loading

Thanks for your work on this great package! I found some weird things on the cooperation between org, indent-region and org-modern and have no idea whether its a feature or a bug.

When org-adapt-indentation (a built-in variable) is set, indent-region is expected to indent heading, text and blocks to their correct level like this:

** A level-2 heading
   something

   something

   #+begin_quote
   a block
   #+end_quote

   and something

But when org-modern is turned on in this buffer, indent-region makes the region:

** A level-2 heading
   something

   something

            #+begin_quote
a block
            #+end_quote

and something

Every time I run indent-region, the block tag got indented once more, but the text after a block just remain unchanged.

Currently what I can do is define a function to turn off org-modern, indent and then turn on org-modern, and bind C-M-\ to it. Blocks get indented accordingly at this time, though they shouldn't.
@minad
Copy link
Owner

minad commented Jul 10, 2023

Thanks. I can reproduce the problem. It seems that org-indent-region or org-indent-line has issues computing the indentation if there are invisible/diplay properties on the line. This is not something we can fix in org-modern, since org-modern only augments the fontification and does not override indentation functionality. It should be repaired in Org directly if even - maybe ask on the Org issue tracker? I suggest you either don't use org-adapt-identation or set org-modern-block-name to nil. With org-modern-block-name=nil the issue also seems to go away.

@minad minad closed this as completed Jul 10, 2023
@yantar92
Copy link

yantar92 commented Jul 11, 2023 •
edited
Loading

https://list.orgmode.org/als4q47p4dt4iva4s73sihe75qjdz4ni554h55e63o6izogzst@vs3olesw6da3/T/#m8d85b510def4163f8401988aaa44bf935027745f

I do not think that Org indentation is doing anything wrong here.

@minad
Copy link
Owner

minad commented Jul 11, 2023

@yantar92 Any suggestion how this should be fixed? I think the expectation is that the indentation function is based on the underlying text (in the sense that we treat Org as a text format) and not on the way the text is displayed. However somewhere in the indentation process the displayed representation is taken into account for the computations. Where does this happen, in indent-line-to?

@minad
Copy link
Owner

minad commented Jul 11, 2023 •
edited
Loading

@yantar92 Nevermind. I saw that you already mentioned that indentation is based on "visual alignment" by convention.

I believe that it is org-modern's fault. Indentation works are it
supposed to and tried hard to align text visually to third column.
org-modern fights against.

Note that indenting visually is Emacs' convention that applies
everywhere.

My opinion is that the convention does not make much sense, but the convention is surely not going to change. For variable pitch fonts one can throw the convention out of the window right away, since there one will never achieve perfect indentation. Indentation via spaces only really works well for fixed pitch fonts and as such one could as well base it on the underlying text representation only.

@hammerfunctor I don't see a way to fix this. Therefore I suggest that you avoid org-adapt-indentation. You can disable org-modern-block-name prettification or try use org-indent-mode.

@yantar92
Copy link

Emacs does it. indent-to.

As for suggestions, you can maybe try line-prefix text property.

@yantar92
Copy link

@hammerfunctor I don't see a way to fix this. Therefore I suggest that you avoid org-adapt-indentation. You can disable org-modern-block-name prettification or try use org-indent-mode.

It has little to do with org-adapt-indentation. Any indentation will be problematic except 0.

@yantar92
Copy link

yantar92 commented Jul 11, 2023 via email

@yantar92
Copy link

As for suggestions, you can maybe try line-prefix text property.

Ok. I looked into org-modern, and I do see that it is what you do already.
What you may try to do is not matching indentation spaces in
https:/minad/org-modern/blob/main/org-modern.el#L736
Or, alternatively, you can put a display property that will ensure preserving indentation.

@minad
Copy link
Owner

minad commented Jul 11, 2023 •
edited
Loading

@yantar92

Emacs does it. indent-to.

Understood. Thanks.

As for suggestions, you can maybe try line-prefix text property.

Not sure how this is going to work if I want to preserve the visual style of org-modern. I am not saying that org-modern is without downsides but so far it works well for me. Despite the styling, most elements stay editable for example (in contrast to svg tags). There are certainly problems in edge cases like this indentation issue. In the end it is the choice of the user if they prefer the plain text display or if they want some styling, same with settings like org-hide-emphasis-markers.

It has little to do with org-adapt-indentation. Any indentation will be problematic except 0.

My assumption is that Org is mostly not an indented format and org-adapt-indentation violates this assumption. You surely have a much better picture on how people actually use Org, but it seems to me that avoiding indentation is the current recommendation, perhaps inspired by other light markup formats like Markdown? Iirc older Org versions had a setting like org-adapt-indentation enabled by default?

Note that org-indent-mode will also fight with org-modern once you
start editing the buffer.

Where do you observe this fighting? The only styling which is incompatible with org-indent-mode is org-modern-block-fringe, but org-modern already disables this setting if org-indent-mode is enabled. The other settings should not lead to bigger issues. Prettification of the stars doesn't look nice in the presence of indentation due to misalignment, but I would not call this "fighting". One can try the following settings:

(org-indent-mode)
(global-org-modern-mode)
(setq org-modern-hide-stars ".")

@minad
Copy link
Owner

minad commented Jul 11, 2023

@yantar92

Ok. I looked into org-modern, and I do see that it is what you do already.
What you may try to do is not matching indentation spaces in
https:/minad/org-modern/blob/main/org-modern.el#L736
Or, alternatively, you can put a display property that will ensure preserving indentation.

Right. I don't observe this issue with either one or both of the following settings, since then the spaces before the #+begin_src keyword are not hidden.

;; Disable problematic setting 1: Do not style fringe
(setq org-modern-block-fringe nil)

;; Disable problematic setting 2: Do not style block name
(setq org-modern-block-name nil)

The problem seems to be that we cannot fix the indentation issue without changing the styling at the same time. So you are right that the block styling fights with both indentation and also org-indent-mode. There exists already a small package by @jdtsmith which should avoid these problems by implementing the block styling differently, see https:/jdtsmith/org-modern-indent. Unfortunately it has other downsides, e.g., incompatibility with line-spacing>0 and also higher complexity due to patching org-indent via advices. I try to avoid advices here if possible.

But given that org-modern-block-fringe (which is the actual root cause here) is so problematic I should maybe reconsider this feature, try some other implementation or disable it by default.

@yantar92
Copy link

yantar92 commented Jul 11, 2023 via email •
edited
Loading

@yantar92
Copy link

The problem seems to be that we cannot fix the indentation issue without changing the styling at the same time. So you are right that the block styling fights with both indentation and also org-indent-mode

Maybe https:/minad/org-modern/blob/main/org-modern.el#L741

Also, it is not necessary to set fringes via line-prefix/wrap-prefix. See "41.16.5 Displaying in the Margins" in Elisp manual:

To display something in the margin in association with certain
buffer text, without altering or preventing the display of that text,
put on that text an overlay with a ‘before-string’ property, and put the
margin display specification on the contents of the before-string.

@minad
Copy link
Owner

minad commented Jul 11, 2023 •
edited
Loading

Try the recipe from
https://list.orgmode.org/als4q47p4dt4iva4s73sihe75qjdz4ni554h55e63o6izogzst@vs3olesw6da3/T/#m84ce8f02c2bbf107b108b3470777c2d3609cef00

I am not sure I understand. You introduce some misalignment using indent-to - this is the problem described in this issue. Then it makes sense that newlines lead to further misalignment.

On the other hand if I turn on org-indent-mode and then org-modern-mode I can insert and edit quote or src blocks without problems. The indentation in source blocks is not affected negatively - no fighting.

Maybe https:/minad/org-modern/blob/main/org-modern.el#L741

Yes, exactly. This is the line which hides the spaces if both org-modern-block-name and org-modern-block-fringe are on.

Also, it is not necessary to set fringes via line-prefix/wrap-prefix. See "41.16.5 Displaying in the Margins" in Elisp manual:

Sure, but then we have to use overlays - something I try to avoid here for performance issues.

@yantar92
Copy link

yantar92 commented Jul 11, 2023 via email

@minad
Copy link
Owner

minad commented Jul 11, 2023 •
edited
Loading

Let me elaborate.

I tried the following steps:

  1. Insert content in an Org buffer
** Heading

#+begin_quote
a block
#+end_quote
  1. Enable org-indent-mode and org-modern-mode
  2. Move to the end of #+begin_quote
  3. Press RET

So far so good. I tried this both in my setup and with emacs -Q. I am on a very recent Emacs 29, compiled a few days ago. What did I do wrong or which bad effect should I observe precisely?

This is no longer necessary starting from Emacs 29, where overlays are
re-implemented using proper segment tree and actually scale even better
compared to text properties (what an irony...).

I am aware of the overlay improvements but I have my doubts about the better scaling behavior of overlays. Can you explain this in more detail? Text properties are commonly put on every line, e.g., all the faces by font locking. If I open a large source file and force font locking on every line it still works reasonably well (not sure about the scaling, I don't have any numbers). Would you argue that it is more efficient if faces are instead applied via overlays instead of via properties?

@yantar92
Copy link

yantar92 commented Jul 12, 2023 via email

@minad
Copy link
Owner

minad commented Jul 12, 2023

Do this in reverse order: enable org-modern-mode first.

It is not correct to do that. org-modern-mode relies on org-indent-mode to be enabled first. This is a simple order dependency, but not a bigger issue.

First let me explain a bit how Emacs redisplay works wrt text properties
and overlays.

Thank you very much for the precise explanation! This is pretty much what I expected.

In Emacs, 29 for both overlays and text properties we get O(logN) search time, where N is the number of overlays or number of text property intervals. If I would use overlays instead of text properties I would also need separate overlays for intervals with differing text properties. Therefore we get the same scaling for both if they are used for the same purpose, e.g., if we would do all the font locking via overlays instead of text properties.

Now the question is how the overhead is beyond the scaling behavior, the constants of the operations, the memory usage and so on. My expectation is that overlays are less efficient in that regard, but I may wrong about this. I think one should still prefer text properties where applicable.

An interesting aspect of your analysis is that org-modern adds quite a bit of overhead since it creates more unique intervals. I tested this in a fairly small buffer and it seems that org-modern roughly doubles the number of intervals, which is not too bad given the logarithmic access complexity. The memory usage is another issue however. It probably makes sense to analyze which parts of org-modern causes the most intervals and check if there are ways to reduce this.

Another aspect are markers - I've observed that markers are quite inefficient too if one has many. It severely slows down editing, since all the markers need updating. Maybe this issue has also been addressed in Emacs 29 by storing the markers in a more efficient tree data structure?

@yantar92
Copy link

yantar92 commented Jul 12, 2023 via email

@minad
Copy link
Owner

minad commented Jul 12, 2023

Overlays take several times more space compared to text properties.

Yes, as expected. Therefore one should still keep on using text properties? Or is the scaling different for the redisplay?

Not correct. Redisplay scales with NlogN because it has to traverse all
the intervals in visible region, spending logN on each stop.
So, doubling the number of intervals will double redisplay time (at
large N, when text property lookup dominates).

Why NlogN and not something like VlogN where V is bounded by the size of the visible region? In Org it may be particularly bad since everything is visible if the entire tree is folded?

Is the scaling different for overlays when redisplaying?
Is it also NlogN?

The most promising recent idea I recall was storing markers as overlays.

Yes, one should ideally reuse the data structure for markers too.

@yantar92
Copy link

yantar92 commented Jul 12, 2023 via email

@minad
Copy link
Owner

minad commented Jul 12, 2023

Okay, thank you again very much for your explanations, Ihor. The scaling behavior is mostly aligned with what I had expected.

It would be great to make the fringe properties more flexible. There are other properties which could be improved to allow even more advanced styling, e.g. vertical text alignment or rounded borders. But these are controversial topics I suspect. Some users seem to like org-modernish styling but there are certainly many more purists. :) And of course optimizations would be welcome as always.

Regarding org-modern, I will probably add such an indent mode hook to avoid the order dependency. Then I will investigate if I can reduce the amount of intervals somehow. But the factor of two I mentioned before is likely a worst case estimate which only applies to buffers with many tags, todo states, tables and so on.

@yantar92
Copy link

yantar92 commented Jul 12, 2023 via email

@minad
Copy link
Owner

minad commented Jul 12, 2023

By "more flexible", do you mean improvements in Emacs?
Note that Org itself uses vertical text alignment in org-use-sub-superscripts.

Yes, I meant improvements in Emacs, but I don't have concrete proposals. As example I would like to have an option to adjust vertical text alignment buffer locally if line-spacing>0.

@jdtsmith
Copy link
Contributor

Very interesting conversation. I am using 'display text properties for org-modern-indent, and also updating the line-prefix org-indent adds (adjacent to textually flush-left blocks only). I've been experimenting (in another mode) with the use of :stipple face properties for vertical bars, which appear to be very efficient, and might work for org-modern-indent (and would fix your line-spacing > 1 issue, @minad). But all this discussion of text-property interval explosion gives me pause.

I have been presuming that font-lock is so well optimized in terms of drawing/computing "only that which is necessary" just-in-time for redisplay that it would be challenging to beat it using a custom overlay algorithm. So I tend to prefer leveraging font-lock machinery to add text properties (although with a more refined approach than just 1 subexp = 1 face). I suppose I could use font-lock to "identify the regions to update" and then, instead of applying a custom font-lock-face directly to the text, add overlays (with the same face). Just sounds very roundabout... Thoughts?

@yantar92
Copy link

yantar92 commented Jul 12, 2023 via email

@yantar92
Copy link

yantar92 commented Jul 12, 2023 via email •
edited
Loading

@minad
Copy link
Owner

minad commented Jul 12, 2023

@jdtsmith I don't think performance is such a big problem for reasonably sized Org files. I am quite sensitive to performance and I usually avoid adding slow minor modes to my setup. org-modern hasn't so far made problems for me, but usually my Org files are also not gigantic. As another example, in your shakespeare.org I get only a 20% increase in additional text property intervals due to org-modern. The 100% increase I mentioned before really is a worst case scenario.

@minad minad mentioned this issue Jul 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@minad @jdtsmith @yantar92 @hammerfunctor