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

disable org-superstar in org source code block #34

Merged
merged 2 commits into from Sep 15, 2021
Merged

disable org-superstar in org source code block #34

merged 2 commits into from Sep 15, 2021

Conversation

taquangtrung
Copy link
Contributor

@taquangtrung taquangtrung commented Sep 13, 2021

Hi,

I created this PR to disable org-superstar from transforming the symbol -, +, * in org source code block.

Currently, for the following code block, org-superstar will transform the + symbol into the right-arrow symbol.

    #+begin_src c
    + 1
    #+end_src

This PR will prevent this situation from happening.

Can you advise if this feature is reasonable?

Thank you!

@alphapapa
Copy link

alphapapa commented Sep 13, 2021

This has the potential to adversely impact performance, so it should probably be thoroughly benchmarked.

As well:

  1. save-match-data is already used here, so there's no need to do it a second time.
  2. The org-element-lineage test could probably be used to test this as well. However, since org-in-src-block-p uses a simple text-property check, it might be faster to do it this way. But both approaches should probably be benchmarked, because this package is very performance-sensitive.

@taquangtrung
Copy link
Contributor Author

This has the potential to adversely impact performance, so it should probably be thoroughly benchmarked.

As well:

  1. save-match-data is already used here, so there's no need to do it a second time.
  2. The org-element-lineage test could probably be used to test this as well. However, since org-in-src-block-p uses a simple text-property check, it might be faster to do it this way. But both approaches should probably be benchmarked, because this package is very performance-sensitive.

Thanks for your advice! I just updated the code to improve the performance a bit:

  • I put the check (not (org-in-src-block-p) before org-element-lineage since you said that the former is faster than the later.
  • I also remove the expression t since in (and ... t), t can be removed.

How did you benchmark this package?

I mostly used with small org files (< 1k lines each file) and don't see any bad impact on the performance.

@alphapapa
Copy link

alphapapa commented Sep 13, 2021

I just updated the code to improve the performance a bit:

Those seem like good changes. It would also be good to fix the indentation of that function.

How did you benchmark this package?

I haven't. @integral-dw can probably speak to how he handles that.

In the meantime, see these macros for what I generally use: https://github.com/alphapapa/emacs-package-dev-handbook#bench-multi-macros

I mostly used with small org files (< 1k lines each file) and don't see any bad impact on the performance.

I'm afraid such small files can't provide a meaningful benchmark. I have some Org files with over 50,000 lines, and there are users who have even larger ones.

@integral-dw
Copy link
Owner

Hi,

Currently, for the following code block, org-superstar will transform the + symbol into the right-arrow symbol.

That is very strange, unless I am mistaken the current version should already catch such cases on default settings. Sounds like there is a bug/regression then. I'll look into it (and your patch) and report back when I figured out what is going on.

@integral-dw integral-dw self-assigned this Sep 13, 2021
@integral-dw integral-dw added the bug Something isn't working label Sep 13, 2021
@taquangtrung
Copy link
Contributor Author

@alphapapa @integral-dw : Thanks for looking into it.

I just tested again and confirm that in these two code blocks, the symbol + and * are both prettified by the current org-superstar (before my patch).

    #+begin_src
    (all letters)
    |
    ^
    &
    < >
    = !
    :
    + -
    * / %
    (all other special characters)
    #+end_src

and

    #+begin_src scala
    (all letters)
    |
    ^
    &
    < >
    = !
    :
    + -
    * / %
    (all other special characters)
    #+end_src

@integral-dw
Copy link
Owner

Very peculiar, I can't reproduce this bug (see below). Can you try to reproduce the behavior starting from emacs -Q?
My current setup uses Emacs 27.2 and Org v9.4.6, so if your version(s) differ, that may help to know too. It's odd that org-element-lineage seems to behave differently.

Screenshot from 2021-09-13 20-20-55

(org-element-lineage (org-element-at-point)
'(plain-list) t))
t)))
'(plain-list) t)))))
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be nice to keep the t at the end, see the comment above the function.

Copy link

@alphapapa alphapapa Sep 13, 2021

Choose a reason for hiding this comment

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

AFAIK that's not idiomatic Lisp. Any value can be treated as nil or non-nil, so it's not necessary to explicitly return t unless the calling function expects t to have a different meaning than any other non-nil value.

Copy link
Owner

Choose a reason for hiding this comment

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

In my experience predicates often merely guarantee to return some non-nil value, absolutely. Often their docstrings state "Return non-nil if..." or some variation for that reason. Although I have seen plenty of predicates explicitly state to return t specifically ("Return t if.." being actually used as an example for a predicate docstring in the Elisp reference manual), in which case I'd argue the function should ensure that to be the case. In my mind it's a case where coding style is influenced by documentation style.

Choose a reason for hiding this comment

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

Some of those "return t if" predicates in Elisp are core functions implemented as C subrs, and there would be no reason for them to return anything other than t.

In a function like this, there's no special meaning to t as opposed to another non-nil value, and AFAIK there's no advantage to returning t explicitly, so the code would be simpler without explicitly returning t.

Not that it really matters here, but simpler is usually better. :)

@taquangtrung
Copy link
Contributor Author

Very peculiar, I can't reproduce this bug (see below). Can you try to reproduce the behavior starting from emacs -Q?
My current setup uses Emacs 27.2 and Org v9.4.6, so if your version(s) differ, that may help to know too. It's odd that org-element-lineage seems to behave differently.

Screenshot from 2021-09-13 20-20-55

@integral-dw Hi, I justed with emacs -q, Emacs 28.05, Org 9.4.4, org-superstar-20210913.1924 installed from Elpa, and the problem still occurs:

org-superstar

org-superstar2

@integral-dw
Copy link
Owner

Hi, I justed with emacs -q, Emacs 28.05, Org 9.4.4, org-superstar-20210913.1924 installed from Elpa, and the problem still occurs:

Very odd. I suppose emacs -Q won't change anything either? Just to be on the safe side. This is very odd, since the only variable left is a newer Emacs version, which I can't imagine messing with Org internals. I'll try poking around a bit more to see if I get what is going on there.

@taquangtrung
Copy link
Contributor Author

Hi, I justed with emacs -q, Emacs 28.05, Org 9.4.4, org-superstar-20210913.1924 installed from Elpa, and the problem still occurs:

Very odd. I suppose emacs -Q won't change anything either? Just to be on the safe side. This is very odd, since the only variable left is a newer Emacs version, which I can't imagine messing with Org internals. I'll try poking around a bit more to see if I get what is going on there.

Can you try with this minimal org content? I just tested and the bug still occurs

* Operators

  - ~myObject myMethod 1~ is the same as calling ~myObject.myMethod(1)~.

  - Operator (i.e. function) names can be alphanumeric, symbolic (e.g. ~x1~, ~*~,
    ~+?%&~, ~vector_++~, ~counter_=~)

  - The precedence of an operator is determined by its first character, with the
    following increasing order of priority:

    #+begin_src scala
    (all letters)
    |
    ^
    &
    < >
    = !
    :
    + -
    * / %
    (all other special characters)
    #+end_src


  - The associativity of an operator is determined by its last character:
    Right-associative if ending with :, Left-associative otherwise.

  - Note that assignment operators have lowest precedence. (Read Scala Language
    Specification 2.9 sections 6.12.3, 6.12.4 for more info)

@integral-dw
Copy link
Owner

I managed to reproduce your bug! The missing piece was that the src-block is embedded within a list, which I didn't consider at all when writing the function. I think I'll kick out org-element-lineage completely since checking if the region is in an src block should be good enough.

@integral-dw integral-dw merged commit 979d2dd into integral-dw:master Sep 15, 2021
integral-dw added a commit that referenced this pull request Sep 15, 2021
Since SRC blocks are the only case (I'm aware of) where Org syntax
fundamentally changes it is clearer to use org-in-src-block-p anyway.

See PR #34 for context.
@taquangtrung
Copy link
Contributor Author

@integral-dw Great! Thanks for the update! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants