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

More intuitive priority cycling #236

Closed
wants to merge 3 commits into from

Conversation

gerazov
Copy link
Contributor

@gerazov gerazov commented Mar 27, 2022

I'm not sure if it's bound by emacs orgmode behavior, but current priority increase and decrease is a bit counterintuitive.

  • increase cycle
    • none (default priority) -> [#C] (low priority) -> [#B] (default priority) -> [#A] (high priority) -> none.
  • decrease cycle
    • none (default priority) -> [#A] (high priority) -> [#B] (default priority) -> [#C] (low priority) -> none.

Thus, the first press actually decreases priority, and then sets it to the initial state, and only then actually increases priority. The reverse is true for decrease - one needs to press 3 times to actually reduce priority.

In other words to give [#A] priority to a task with no priority, the fastest way to do it is by pressing decrease priority.

This PR modifies the cycle as follows:

  • increase cycle
    • none (default priority) -> [#A] (high priority) -> none
    • [#C] (low priority) -> [#B] (default priority) -> [#A] (high priority) -> none
  • decrease cycle
    • none (default priority) -> [#C] (low priority) -> none
    • [#A] (high priority) -> [#B] (default priority) -> [#C] (low priority) -> none

While it breaks the cycle starting with none, the increase and decrease reflect the change in priority.

@kristijanhusak
Copy link
Member

This code seems to completely break the cycle.
priority-pr-bug

@gerazov
Copy link
Contributor Author

gerazov commented Apr 4, 2022

Yes it does ... but it does consistently increase (decrease) priority. We could have it loop, i.e. none -> A -> C -> none -> A, but then the B is missing.

Maybe it would make sense to have a separate cycle up/down command that would do what increase and decrease do atm. But it all ties back to how it's done in emacs.

btw - great work on the Clocking summary table 😎

@TravonteD
Copy link
Contributor

In other words to give [#A] priority to a task with no priority, the fastest way to do it is by pressing decrease priority.
There is a binding for changing the priority to any state (iirc it's <leader>o, by default). I would say that's the fastest way to get to the highest priority.

@gerazov
Copy link
Contributor Author

gerazov commented Apr 21, 2022

Good tip! but you still need to write in the priority, so that's 4 keys. Currently I use reduce priority to mark tasks as important - it's one key in the agenda - and 3 in the org file cir 🙂

@TravonteD
Copy link
Contributor

A possible solution to this (for the sake of convenience) could be to have a set_to_priority(<num>) function that can be used in custom mappings, so that one could make a keymap for priority A,B,etc.

@gerazov
Copy link
Contributor Author

gerazov commented Apr 26, 2022

Hmm 🤔 maybe a toggle function would be convenient? e.g. toggle_high_priority() for [#A] and toggle_low_priority() for [#C], imo [#B] is not that useful.

@TravonteD
Copy link
Contributor

My previous suggestion is now possible by using the new treesitter functions. Here's an example.

local Headline = require('orgmode.treesitter.headline')
local tree_utils = require('orgmode.utils.treesitter')

vim.keymap.set('n', '<some-keymap>', function()
  local headline = Headline:new(tree_utils.closest_headline())
  headline:set_priority('A')
end)

@seflue
Copy link
Contributor

seflue commented Oct 17, 2024

This PR is quite old, but I thought about the problem and want to propose another solution - because the current behavior remains to be unintuitive.

The priority settings allow to define a assumed priority for unprioritized headlines (which is [#B] by default). If we take this priority as baseline, increase would cycle to [#A] -> [#C] -> [#B] and decrease would cycle to [#C] -> [#A] -> [#B]. It would not cycle through none, because none is already equivalent with [#B]. This would be also somehow consistent with the behavior for todo states, because there is no "no todo" state I can cycle through (actually there is currently also no toggle function I am aware of). If the user really want's to remove the priority, he can either remove it by hand or with the "set priority" feature. Also a separate toggle mapping could be implemented, which would remove the current priority or set the default priority. But the main use cases for increase and decrease would then behave more intuitive for all cases.

@kristijanhusak What do you think?

@kristijanhusak
Copy link
Member

@seflue I prefer having it work as Emacs orgmode does. I'm not 100% sure how it works there from top of my head, but that would be the end goal.

@gerazov
Copy link
Contributor Author

gerazov commented Oct 18, 2024

I agree with @seflue although sticking to Emacs is a good approach to keep consistency.

Otherwise, maybe it makes even more sense to remove [#B] altoghether. So to cycle up none -> [#A] -> [#C] -> none and cycle down none -> [#C] -> [#A] -> none.

Also at some point I was thinking about just having a toggle_high_priority() and toggle_low_priority(). If it is something that could be integrated I can make a PR.

Anyways, I'm already used to the current shortcuts but in my mind Ctr-r is set high priority and Ctr-R is set low priority.

@seflue
Copy link
Contributor

seflue commented Oct 18, 2024

@seflue I prefer having it work as Emacs orgmode does. I'm not 100% sure how it works there from top of my head, but that would be the end goal.

So I actually installed emacs and checked. The Emacs behavior is close to what I described, but has a little quirk.

When you move the cursor into a headline without a todo it works like:
none -> [#B] (or whatever is configured as default prio) -> [#A] (or default + 1) -> none -> [#C] -> [#B] -> [#A] -> none
So if you don't move the cursor, it will move from the highest priority into none and then cycle up from the lowest prio. But as soon as you move the cursor on a headline with no explicit priority, it will first set the default priority explicitly and then increase from there. Which is close to the behavior as I just described. Decrease works the same, but the other way around. Start with default ([#B]) -> [#C] -> none -> [#A] -> [#B] -> [#C] -> none -> ...

I don't know, if it is possible how to detect in Neovim, that the cursor didn't move. So the simplified version would be, what I formerly proposed. The current implementation in nvim-orgmode is obviously not, how Emacs orgmode works, especially, if you just want to increase or decrease the priority of an empty headline.

@kristijanhusak
Copy link
Member

@seflue thanks for testing. We can go with your proposed solution.

@seflue
Copy link
Contributor

seflue commented Oct 18, 2024

@gerazov: I wouldn't remove the default priority altogether from the cycle, because when someone is increasing or decreasing it, one can assume, they actually care about it and it is more surprising, if it just vanishes completely. And if someone wants to remove it again, they have already a convenient way with org_priority.

Would you be motivated to adjust your implementation to the behavior I described, or should we create a new PR?

@gerazov
Copy link
Contributor Author

gerazov commented Oct 19, 2024

Uff this is an old merge request - should be rebased 2 years in the future 😅

Maybe I'll start one anew?

@seflue
Copy link
Contributor

seflue commented Oct 20, 2024

@gerazov I tested to rebase the branch locally and had no issues, because there were no code changes in the last 2 years. 😉

@seflue
Copy link
Contributor

seflue commented Oct 20, 2024

@kristijanhusak The merge actually introduced an old bug I fixed in the past with a bigger refactoring of the priority_state class. Because of that adjusting the tests to the new behavior was not trivial anymore, so I sat down myself to implement the improvement and created a new PR: #817

So this PR is now obsolete and can be closed. Sorry for the inconvenience.

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.

4 participants