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

Fix priorities (#784) #785

Merged
merged 5 commits into from
Jul 29, 2024
Merged

Conversation

seflue
Copy link
Contributor

@seflue seflue commented Jul 27, 2024

This PR fixes #784.

@seflue
Copy link
Contributor Author

seflue commented Jul 27, 2024

@kristijanhusak The failing tests are unrelated to the changes and also fail locally on master.

@kristijanhusak
Copy link
Member

@seflue I don't remember Emacs orgmode supporting this. Can you confirm?

@seflue
Copy link
Contributor Author

seflue commented Jul 27, 2024

@seflue I don't remember Emacs orgmode supporting this. Can you confirm?

I am confused and do not understand what you mean with "Emacs not supporting this"? You wrote tests yourself There are existing tests for PriorityState, which contain cases supporting more than three priorities. But the integration contained a bug, which led to the quirk explained in #784. If you need a refresher on how priorities work in orgmode, see here, but reading the existing tests in priority_state_spec probably bring you faster up to speed. Or try it out yourself by configuring some examples from the test in your orgmode and try to cycle through the priorities. This is clearly a bug.

Edit: I just saw in the Git history, that you didn't implement the priority feature yourself. It was implemented by @levouh back in 2021.

Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

I misunderstood what are you fixing here.
Lets just do these minor changes and it's good to go.

lua/orgmode/api/headline.lua Show resolved Hide resolved
@kristijanhusak
Copy link
Member

Since we added 2 new hl groups we need to make them work by adding them to highlights. This patch should fix it:

diff --git a/queries/org/highlights.scm b/queries/org/highlights.scm
index f85a90a..2e737a5 100644
--- a/queries/org/highlights.scm
+++ b/queries/org/highlights.scm
@@ -12,7 +12,9 @@
 (item . (expr) @org.keyword.todo @nospell (#org-is-todo-keyword? @org.keyword.todo "TODO"))
 (item . (expr) @org.keyword.done @nospell (#org-is-todo-keyword? @org.keyword.done "DONE"))
 (item (expr "[" "#" "str" @_priority "]") @org.priority.highest (#org-is-valid-priority? @_priority "highest"))
+(item (expr "[" "#" "str" @_priority "]") @org.priority.high (#org-is-valid-priority? @_priority "high"))
 (item (expr "[" "#" "str" @_priority "]") @org.priority.default (#org-is-valid-priority? @_priority "default"))
+(item (expr "[" "#" "str" @_priority "]") @org.priority.low (#org-is-valid-priority? @_priority "low"))
 (item (expr "[" "#" "str" @_priority "]") @org.priority.lowest (#org-is-valid-priority? @_priority "lowest"))
 (list (listitem (paragraph) @spell))
 (body (paragraph) @spell)

seflue added 5 commits July 29, 2024 18:59
To prepare the fix, we need to avoid a dependency cycle between config
and priority_state, because we will need priority_state in config to
implement the fix.

We inject the needed values, whereever an instance of PriorityState is
created.
A headline priority gets only detected correctly, if it is part of the
table returned by Config:get_priorities(). While it previously only
returned the priorities explicitly defined by the user, it now uses
PriorityState to generate also intermediate ones. This happens, if the
highest and the lowest priority define a range of more than three
priorities.
Add new highlight groups to "Colors" chapter.
Copy link
Member

@kristijanhusak kristijanhusak left a comment

Choose a reason for hiding this comment

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

I'll add highlights since I plan to push some minor changes anyway. Thanks!

@kristijanhusak kristijanhusak merged commit c2595bd into nvim-orgmode:master Jul 29, 2024
6 checks passed
SlayerOfTheBad pushed a commit to SlayerOfTheBad/orgmode that referenced this pull request Aug 16, 2024
* test: implement test to expose the bug

* test: add more cases in priority_state_spec

* refactor: make priority_state independent of config

To prepare the fix, we need to avoid a dependency cycle between config
and priority_state, because we will need priority_state in config to
implement the fix.

We inject the needed values, whereever an instance of PriorityState is
created.

* fix: generate missing intermediate priorities

A headline priority gets only detected correctly, if it is part of the
table returned by Config:get_priorities(). While it previously only
returned the priorities explicitly defined by the user, it now uses
PriorityState to generate also intermediate ones. This happens, if the
highest and the lowest priority define a range of more than three
priorities.

* docs: mention new highlight groups

Add new highlight groups to "Colors" chapter.

---------

Co-authored-by: Sebastian Flügge <seflue@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing existing priority breaks with more than three priorities
2 participants