Closed jgollenz closed 2 years ago
It's because the this function interprets the current prio-state as a single char. That's fine for an alphabetic prio-state, but breaks for numbers larger than 9.
The cycling also breaks when e.g. the lowest prio is set to 5 and someone manually sets it to 6. Instead of wrapping back to 1, it will increase to 7 on cir
. This also goes for letters. If lowest is still at 5 and someone sets it to 'A', cir
will switch it to 'B'. That's because the current cycling logic assumes that the values are in the valid range.
I propose the following:
First, upon loading the user-config, we validate whether the triplet of highest-, lowest- and default-priority are conforming to the types we allow (whole numbers and one-char strings). That also means 1) verifying that they all agree on that type and 2) that they are in the right order. If a user only sets the highest-priority to e.g. 1, we get a mismatch because the default for lowest-priority is C
. If a user were to set highest-priority to F
, we get undefined behavior because F > C.
Doing this validation saves us from whole class of bugs and undefined behavior. Users should not be able to do this by accident, neither by choice.
Second, when we have a valid triplet, we store the type of the triplet. If it is a one-char string, our current logic works. If it is a number, we do simple in-/decrementing arithmetic on that number instead of only the first char of the number represented as a string. We just branch on the type of the triplet.
This much for the functional behavior. I have some comments on the current implementation. On every prio in-/decrement we create a new PriorityState object to which we pass the current priority of the headline as an init value. We then call in-/decrease on that object, get that value and store the value in the headline. Nothing is done with that object afterwards, its discarded. I'm arguing that this is unnecessarily wasteful. We could just call Priority:decrease(value)
with our current prio-state and have that function store no state at all. OR if we want to store state and not pull the config triplet every time, create a PriorityHandler
object once for the OrgMappings
object and use that every time we want to change a priority.
I'm all for bundling related functionality in a file, but creating an object just to apply some logic on a single value and then discarding that object appears quite wasteful to me.
@jgollenz was this solved with your PR?
Nope
Describe the bug
When due to custom values for
org_priority_[highest|lowest]
a prio-state above 9 becomes a valid state, the cycling is broken. See the gif.Steps to reproduce
minimal_lua.init
cir
on a headline until you reach a prio-state above 9Expected behavior
According to this prio-states up to 64 are valid. I highly suspect this is because 65 is the ASCII code for
A
. If we want to replicate this, we should add it to the docs.Emacs functionality
It appears, that vanilla orgmode is also buggy here, but the behavior is different. Try setting#+PRIORITIES: 1 35 1
at the beginning of a buffer. For me, the cycling only ranges between 1 and 3. Looks like the 35 is parsed as 3. If it works for you, please let me know!Edit: I had an outdated version of orgmode in my emacs. It works as expected.
Minimal init.lua
Screenshots and recordings
The bug is even more peculiar when doing reverse cycling with
ciR
:OS / Distro
Ubuntu 20.04
Neovim version/commit
0.7.0
Additional context
No response