Priority Cycling broken when prio-state above 9

[jgollenz]

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

• open a buffer with the provided minimal_lua.init
• enter cir on a headline until you reach a prio-state above 9

Expected 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

vim.cmd([[set runtimepath=$VIMRUNTIME]])
vim.cmd([[set packpath=/tmp/nvim/site]])

local package_root = '/tmp/nvim/site/pack'
local install_path = package_root .. '/packer/start/packer.nvim'

local function load_plugins()
  require('packer').startup({
    {
      'wbthomason/packer.nvim',
      { 'nvim-treesitter/nvim-treesitter' },
      { 'kristijanhusak/orgmode.nvim', branch = 'master' },
    },
    config = {
      package_root = package_root,
      compile_path = install_path .. '/plugin/packer_compiled.lua',
    },
  })
end

_G.load_config = function()
  require('orgmode').setup_ts_grammar()
  require('nvim-treesitter.configs').setup({
    highlight = {
      enable = true,
      additional_vim_regex_highlighting = { 'org' },
    },
  })
  require('orgmode').setup({
    org_priority_highest = '1',
    org_priority_default = '5',
    org_priority_lowest = '15'
  })

  vim.cmd([[packadd nvim-treesitter]])
  vim.cmd([[runtime plugin/nvim-treesitter.lua]])
  vim.cmd([[TSUpdateSync org]])

  -- Close packer after install
  if vim.bo.filetype == 'packer' then
    vim.api.nvim_win_close(0, true)
  end

  require('orgmode').setup()

  -- Reload current file if it's org file to reload tree-sitter
  if vim.bo.filetype == 'org' then
    vim.cmd([[edit!]])
  end
end

if vim.fn.isdirectory(install_path) == 0 then
  vim.fn.system({ 'git', 'clone', 'https://github.com/wbthomason/packer.nvim', install_path })
  load_plugins()
  require('packer').sync()
  vim.cmd([[autocmd User PackerCompileDone ++once lua load_config()]])
else
  load_plugins()
  load_config()
end

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

[jgollenz]

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.

[jgollenz]

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.

[jgollenz]

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.

[kristijanhusak]

@jgollenz was this solved with your PR?

[jgollenz]

Nope