Refactor Object Browser and Add Support for Custom Key Mappings

[she3o]

This pull request introduces several enhancements to the Object Browser module in R.nvim, focusing on code refactoring, improved readability, and added flexibility through custom key mappings. Key Changes

1. Code Refactoring and Cleanup

• Use Options Table in set_buf_options: Replaced multiple vim.api.nvim_set_option_value calls with a single options table and a loop to set buffer options. This reduces code repetition and simplifies future maintenance. afe2e6084beef9c991bddbc04736d8a825824bff

• Update Key Mappings to Use vim.keymap.set: Switched from vim.api.nvim_buf_set_keymap to vim.keymap.set with the buffer = true option, utilizing the more modern Neovim keymap API. afe2e6084beef9c991bddbc04736d8a825824bff

• Convert Anonymous Functions to Named Declarations: Changed anonymous function assignments to named function declarations for better readability and stack traces. e3ac35c052beeb51301a074ee26019689f717e38

• Simplify Conditional Statements: Streamlined nested if statements to reduce complexity. e3ac35c052beeb51301a074ee26019689f717e38

Add Module Documentation

0b39c6ed38c8f202c53d2b791de90aac70d25c24

• Introduced documentation at the top of browser.lua to explain the module's purpose, key components, and integration with other parts of the plugin.
• Added documentation through out the file.

Support for Custom Key Mappings in Object Browser

69002a60f650ebed90a9b875c663ed40bd7e57e3 168e901a39f53cfcd4e02af914d3bc3ee2b040f1

• Introduce config.objbr_mappings: Added a new configuration option objbr_mappings to allow users to define custom key-command pairs for the Object Browser.

  local config = {
  -- ... existing configuration options ...
  objbr_mappings = { s = "summary", p = "plot" },
  -- ... rest of the configuration options ...
  }

• Update set_buf_options Function: Modified set_buf_options to iterate over objbr_mappings and set the corresponding key mappings within the Object Browser buffer.

  for key, command in pairs(config.objbr_mappings) do
  vim.keymap.set(
      "n",
      key,
      function() require("r.browser").run_custom_command(command) end,
      opts
  )
  end

• Implement M.run_custom_command: Created a new function to execute the specified command on the selected object in the Object Browser.

  
  function M.run_custom_command(command)
  local lnum = vim.api.nvim_win_get_cursor(0)[1]
  if lnum < 3 then return end
  
  local curline = vim.api.nvim_buf_get_lines(0, lnum - 1, lnum, true)[1]
  local object_name = M.get_name(lnum, curline)
  if object_name == "" then
      warn("No object selected.")
      return
  end
  
  -- Execute the command on the selected object
  require("r.send").cmd(command .. "(" .. object_name .. ")")
  end

- Provide Default Mappings: Added default key mappings (s for summary, p for plot) to enhance out-of-the-box functionality.

[jalvesaq]

Thanks! And welcome back! I will look at this on the weekend.

[she3o]

I can define arbitrary mapping in my init.lua or even define custom functions in .Rprofile and use them in case a function requires complicated syntax

  {
    'she3o/tmp-R-Nvim',
    lazy = false,
    branch = 'clean-object-browser',
    opts = {
      auto_start = 'on startup',
      R_args = { '--quiet', '--no-save' },
      objbr_mappings = {
        g = 'dplyr::glimpse',
        dd = 'rm',
        m = 'object.size',
      },
-- Rest on init.lua

But the dd = 'rm' sometimes produce weird rendering in the object browser. probably because it modifies the object browser.

I also wished to re-implement the keymapping for <CR> and <2-LeftMouse> but feared it wouldn't be backward compatible.

[she3o]

This now works. 3d89282

      objbr_mappings = {
        c = 'class({object})',
        dd = 'rm',
        g = 'dplyr::glimpse',
        h = 'head({object}, 10)', -- Command with arguments
        l = 'length({object})',
        m = 'object.size',
        n = 'names', -- Command without placeholder, object name will be appended.
        p = 'plot({object})',
        s = 'str({object})',
        ss = 'summary({object})',
      },

[she3o]

In the edge case that the {object} placeholder conflicts with weird R code, 7bf0fa3 adds objbr_placeholder

objbr_placeholder = "<<<>>>",
objbr_mapping ={
   plot = "plot(<<<>>>)"
}

I can't think of a character sequence that is extremely readable, extremely short, and guaranteed to not overlap with lua/R syntax. so I shift this dilemma to the user.

[she3o]

Commit e2ce3c9 added the default objbr keymap:

        v = function()
          require('r.browser').toggle_view()
        end,

It didn't work but, it works if I define it in my init.lua. I don't know why! for now I will revert that hunk.

[she3o]

Potential use...

-- init.lua
local function view_object_details()
  local browser = require 'r.browser'
  local lnum = vim.api.nvim_win_get_cursor(0)[1]
  local curline = vim.api.nvim_buf_get_lines(0, lnum - 1, lnum, true)[1]
  local object_name = browser.get_name(lnum, curline)
  if object_name == '' then
    require('r').warn 'No object selected.'
    return
  end

  -- Open a split window and show the object's details
  vim.cmd 'split'
  vim.cmd 'enew'
  vim.api.nvim_buf_set_lines(0, 0, -1, false, { 'Details of ' .. object_name })
  -- More logic here to fetch and display details
end

-- more config

-- Bind e to view_object_details
      objbr_mappings = {
        c = 'class({object})',
        dd = 'rm',
        e = view_object_details,
        g = 'dplyr::glimpse',
        h = 'head({object}, 10)', -- Command with arguments
        l = 'length({object})',
        m = 'object.size',
        n = 'names', -- Command without placeholder, object name will be appended.
        p = 'plot({object})',
        s = 'str({object})',
        ss = 'summary({object})',
        v = function()
          require('r.browser').toggle_view()
        end,
      },

[PMassicotte]

Would it be appropriate to add something in https://github.com/R-nvim/R.nvim/blob/main/doc/R.nvim.txt?

[jalvesaq]

Looking at this pull request now...

On the one hand, since the Object Browser is not supposed to be edited, with this pull request, we can easily create custom commands with single-letter shortcuts in Normal mode that would normally clash with default Vim edit commands, and that's good. On the other hand, the more cumbersome keybindings starting with <LocalLeader>r have the advantage of already being memorized to be used when editing R scripts. It also gives users more choices. For example, if g is mapped, we can't use gg to go to the beginning of the Object Browser buffer. So, could the user create a keybinding starting with <LocalLeader>, or that's not possible and not a goal to achieve?

With the example given in a comment above, I'm getting these warnings on startup:

Invalid option `objbr_mappings.m`.
Invalid option `objbr_mappings.dd`.
Invalid option `objbr_mappings.g`.

Would it be appropriate to add something in https://github.com/R-nvim/R.nvim/blob/main/doc/R.nvim.txt?

Yes, when the development of the feature is finished or immediately to allow us to know how it should work.

[she3o]

So, could the user create a keybinding starting with

['<localleader>gg'] = 'R_command' will work

      objbr_mappings = {
        ['<localleader>gg'] = 'head', -- Use <localleader> to  define keymaps
        c = 'class',
        dd = 'rm',
        e = view_object_details,
        g = 'dplyr::glimpse',
        h = 'head({object}, 10)', -- Command with arguments
        l = 'length',
        m = 'object.size',
        n = 'names', -- Command without placeholder, object name will be appended.
        p = 'plot',
        s = 'str',
        ss = 'summary',
        v = function()
          require('r.browser').toggle_view()
        end,
      },

With the example given in a comment above, I'm getting these warnings on startup:

I just noticed that I get the same warnings.

[jalvesaq]

['<localleader>gg'] = 'R_command' will work

Great! When the function is documented, it will be good to include an example similar to this.

[she3o]

What are your opinions on encapsulating parts of the config in separate subtables @jalvesaq @PMassicotte , like this

-    objbr_allnames      = false,
-    objbr_auto_start    = false,
-    objbr_h             = 10,
-    objbr_opendf        = true,
-    objbr_openlist      = false,
-    objbr_place         = "script,right",
-    objbr_w             = 40,
-    objbr_mappings  = {
-   s = "summary",
-   p = "plot",
-     },
-     objbr_placeholder   = "{object}",
+    object_browser = {
+      allnames      = false,
+      auto_start    = false,
+      height  = 10,
+      open_data_frame        = true,
+      open_list      = false,
+      place         = "script,right",
+      width             = 40,
+      mappings    = {
+                   s = "summary",
+                   p = "plot",
+      },
+      placeholder   = "{object}",
+    },

[jalvesaq]

What are your opinions on encapsulating parts of the config in separate subtables @jalvesaq @PMassicotte , like this

It looks more organized. It reminds me of going from C (global variables) to C++ (classes). But it's important to hear from more people because this would be a disruptive change requiring users to read the documentation and update their config files. It would not introduce any new functionality or improve how the plugin works.

[she3o]

this would be a disruptive change

I will try and make it so that old configuration style also works

[PMassicotte]

What are your opinions on encapsulating parts of the config in separate subtables @jalvesaq @PMassicotte , like this

It looks more organized. It reminds me of going from C (global variables) to C++ (classes). But it's important to hear from more people because this would be a disruptive change requiring users to read the documentation and update their config files. It would not introduce any new functionality or improve how the plugin works.

I agree. Maybe we could support both for a while, with an upcoming deprecation message.

[wurli]

I like subtables! Regarding breaking changes, in my opinion R.nvim is probably young enough that the inconvenience for existing users would be okay. However I do think some concessions should be made to support anyone affected. Namely:

• A note in the README that breaking changes may happen from time to time.

• Adoption of tagged commits and semantic versioning so folks can freeze their installed version if they like, with corresponding guidance in the README. Eventually, perhaps at 1.0.0, a firm commitment to backwards compatibility could be made.

• Informative errors (i.e. how to roll back, how to update) for when attempting to use broken features, although I’m not sure what the best way to do this would be in practice.

[jalvesaq]

Maybe we could follow the path suggested by @wurli. Keeping old code alive is extra work for us. It's easier to add tags, although I have never created tags (except when releasing a new version because Github creates them automatically). When do you think we should add them? After every commit or only before making breaking changes?

I tried to maintain a stable branch on Nvim-R. Now, I think it would be better to create a branch corresponding to a released version (not calling it "stable") and port bug fixes to this branch for a while. But this also means extra work that I'm not willing to have.

[wurli]

When do you think we should add them? After every commit or only before making breaking changes?

I'd suggest that a good rule of thumb might be to bump the minor version either when enough new features have been added that users would probably find the hassle of updating and resolving any unforeseen issues worthwhile, or in case of a clear breaking change. Refactors, optimisations, bug fixes etc would probably all be good reasons to bump the patch version number; I don't think it's necessary for every commit. Perhaps we could use GitHub milestones to plan out when to bump the version too...

I think it would be better to create a branch corresponding to a released version (not calling it "stable") and port bug fixes to this branch for a while. But this also means extra work that I'm not willing to have.

My possibly controversial opinion is that we probably don't need to worry about backporting bug fixes; I think it's sufficient to advise that users upgrade. My two main reasons for this are that (1) I think truly critical bugs, i.e. things which cause significant system crashes or security risks, are very unlikely to occur because of the nature of the project, and (2) most R packages also follow a similar system, so users will be unlikely to see it as unreasonable/burdensome. NB, one advantage of this system is that we wouldn't need to create a new branch for each release and clutter the working tree, just a new tag on main.

In practice I would imagine this would mean continuing to use main for development and occasionally creating tagged releases. I think most plugin managers support this model quite well. E.g. Lazy.nvim supports versioning like so:

-- Would use the latest commit to main
{ "R-nvim/R.nvim", branch = "main" }

-- Would use the latest stable version
{ "R-nvim/R.nvim", version = "*" }

-- Would use any version compatible with 1.2.3, e.g. 1.2.4, 1.2.5, but not 1.3.0 or 1.2.2
{ "R-nvim/R.nvim", version = "~1.2.3" }

[PMassicotte]

Using versioning is a good idea. We should follow https://semver.org/

[wurli]

Using versioning is a good idea. We should follow https://semver.org/

Agreed. 2 points from semver which are particularly relevant:

4. Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

5. Version 1.0.0 defines the public API. The way in which the version number is incremented after this release is dependent on this public API and how it changes.

[jalvesaq]

I agree too. We need a roadmap to define what we expect to reach in each future version. The only feature that I still plan to develop is better YAML completion for Quarto:

• In R code blocks: completion of valid values of cell fields
• In the YAML metadata header: know fields and their valid values

This can be either before or after the 1.0.0.

[wurli]

Glad you agree too! So, here's my suggested roadmap to getting this PR merged, breaking changes and all:

1. Create a 0.1.0 tag for the current version of main and make the aforementioned changes to the README to inform users about how to handle breaking changes

2. Merge this PR and bump the version to 0.2.0, OR, in the case that there are other breaking API changes we'd like to make, merge this PR without the breaking changes to the API, then create another PR specifically focussed on API improvements.

[jalvesaq]

I agree.

I'm reading the semver.org document now... So, just to clarify: what we can consider our public "API" is our config table, right?

[wurli]

I agree.

I'm reading the semver.org document now... So, just to clarify: what we can consider our public "API" is our config table, right?

Yep :)

[wurli]

I'm reading the semver.org document now... So, just to clarify: what we can consider our public "API" is our config table, right?

Actually, I guess it would also include Lua functions, since these are accessible to users... Maybe we need some notion of 'public' so users know which functions they can rely on. Or maybe we should say they're currently all subject to change without notice until 1.0.0?

[jalvesaq]

We can declare in doc/R.nvim.txt some functions to be public and everything else private. Currently, only two functions are used in examples in doc/R.nvim.txt: require('r.send').cmdandrequire('r.run').action`. So, we will have to keep compatibility only for these functions.

[wurli]

I might be able to find time for a PR tomorrow to add docs about versioning, fine by me if anyone else wants to do it before then though :)

[jalvesaq]

I might be able to find time for a PR tomorrow to add docs about versioning

Thanks!

[wurli]

Versioning docs are complete and 0.1.0 released :) This should now be okay to merge I think!

[jalvesaq]

The warnings at startup have to be fixed before merging the pull request. Also, the option was added to the config table following a different style. The change below is a way of fixing both issues:

diff --git a/lua/r/config.lua b/lua/r/config.lua
index 947f931..38ae045 100644
--- a/lua/r/config.lua
+++ b/lua/r/config.lua
@@ -62,11 +62,11 @@ local config = {
     objbr_openlist      = false,
     objbr_place         = "script,right",
     objbr_w             = 40,
-       objbr_mappings          = {
-                                                           s = "summary",
-                                                           p = "plot",
-                                                   },
-       objbr_placeholder   = "{object}",
+    objbr_mappings      = {
+                              s = "summary",
+                              p = "plot",
+                          },
+    objbr_placeholder   = "{object}",
     open_example        = true,
     open_html           = "open and focus",
     open_pdf            = "open and focus",
@@ -270,7 +270,7 @@ local apply_user_opts = function()
         -----------------------------------------------------------------------
         -- 4. If the option is a dictionary, check each value individually
         -----------------------------------------------------------------------
-        if type(user_opt) == "table" then
+        if type(user_opt) == "table" and key_name ~= "objbr_mappings" then
             for k, v in pairs(user_opt) do
                 if type(k) == "string" then
                     local next_key = {}

[jalvesaq]

Also: the new feature has to be documented...

[she3o]

I will do this in a couple of hours

[jalvesaq]

Thank you! The option isn't available on version 0.1.0 which was already released. We are currently developing version 0.2.0. But I already fixed this detail in the documentation and will merge the pull request now.