Refactor Object Browser and Add Support for Custom Key Mappings #237

she3o · 2024-09-27T21:33:41Z

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.
afe2e60
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. afe2e60
Convert Anonymous Functions to Named Declarations: Changed anonymous function assignments to named function declarations for better readability and stack traces. e3ac35c
Simplify Conditional Statements: Streamlined nested if statements to reduce complexity. e3ac35c

Add Module Documentation

0b39c6e

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

69002a6 168e901

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.

Replace multiple `vim.api.nvim_set_option_value` calls with a single options table and iterate over it to set buffer options. This reduces code repetition, improves readability, and simplifies future maintenance.

Replace multiple `vim.api.nvim_buf_set_keymap` calls with `vim.keymap.set`, including the `buffer = true` option. This change utilizes the more modern Neovim keymap API.

…al statements. 1. **Convert anonymous function assignments to named function declarations** - From: ```lua local add_backticks = function(word, esc_reserved) M.start = function(_) -- etc ``` To: ```lua local function add_backticks(word, esc_reserved) function M.start(_) -- etc ``` 2. **Fix typos** 3. **Simplify conditional statements** - Original nested `if` statements: ```lua if isutf8 then if idx == 12 then -- Code end elseif idx == 8 then -- Code end ``` - Simplified combined conditions: ```lua if isutf8 and idx == 12 then -- Code elseif not isutf8 and idx == 8 then -- Code end ```

- Introduce `config.objbr_mappings` to allow users to define custom key-command pairs. - Update `set_buf_options` to iterate over `objbr_mappings` and set corresponding key mappings. - Implement `M.run_custom_command` to execute the specified command on the selected object. - Provide a default mapping (`s` key to `summary` command) in the configuration.

jalvesaq · 2024-09-27T22:00:35Z

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

she3o · 2024-09-27T22:30:24Z

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 · 2024-09-29T01:23:19Z

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})',
      },

…commands.

she3o · 2024-09-29T02:18:58Z

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.

This commit allows users to define functions and bind them to keys in the object browser window. it also adds a new default keymap (i.e., 'v') to toggle between .GlobalEnv and libraries views. This showcases the new feature added in this commit.

she3o · 2024-09-29T03:12:00Z

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 · 2024-09-29T03:32:18Z

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,
      },

This reverts commit 2f1805e.

PMassicotte · 2024-09-29T10:29:28Z

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

jalvesaq · 2024-09-30T00:39:09Z

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 · 2024-09-30T02:15:29Z

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 · 2024-09-30T20:32:01Z

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

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

she3o · 2024-10-01T00:37:48Z

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 · 2024-10-01T01:08:48Z

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 · 2024-10-01T04:21:02Z

this would be a disruptive change

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

PMassicotte · 2024-10-01T10:47:08Z

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 · 2024-10-01T19:36:00Z

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 · 2024-10-01T23:31:14Z

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.

- Group related variables (`curview`, `ob_win`, `ob_buf`, `upobcnt`, `running_objbr`, `auto_starting`, `hasbrowsermenu`) into a unified `state` table. - Update all function implementations to reference `state` fields instead of individual variables. - Enhance code readability and maintainability by centralizing state management. - Prepare the codebase for easier future modifications and potential feature additions.

wurli · 2024-10-02T10:31:26Z

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 · 2024-10-02T10:54:20Z

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

wurli · 2024-10-02T11:01:27Z

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

Agreed. 2 points from semver which are particularly relevant:

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

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 · 2024-10-02T11:18:16Z

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 · 2024-10-02T11:26:47Z

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

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
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 · 2024-10-02T11:38:39Z

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 · 2024-10-02T11:54:45Z

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 · 2024-10-02T12:06:58Z

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 · 2024-10-02T12:14:44Z

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 · 2024-10-02T13:02:40Z

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 · 2024-10-02T13:39:47Z

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

Thanks!

wurli · 2024-10-04T13:55:04Z

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

jalvesaq · 2024-10-04T14:39:40Z

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 · 2024-10-04T14:41:06Z

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

she3o · 2024-10-06T21:01:40Z

I will do this in a couple of hours

This adds PR R-nvim#237 to this branch so that doc can be subsequently modified without introducing conflicts.

Document objbr_mappings in README and help files to explain how to configure custom key mappings for running R commands or Lua functions in the object browser. Added notes on using placeholders (e.g., {object}) and the new objbr_placeholder option for flexibility in command customization.

jalvesaq · 2024-10-07T19:22:35Z

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.

she3o added 7 commits September 27, 2024 18:26

Refactor set_buf_options to use an options table and loop

cae8db3

Replace multiple `vim.api.nvim_set_option_value` calls with a single options table and iterate over it to set buffer options. This reduces code repetition, improves readability, and simplifies future maintenance.

Update key mappings to use vim.keymap.set with buffer-local scope

afe2e60

Replace multiple `vim.api.nvim_buf_set_keymap` calls with `vim.keymap.set`, including the `buffer = true` option. This change utilizes the more modern Neovim keymap API.

Add documentation to object browser module

0b39c6e

Add plot() to default objbr_mappings

168e901

Fix Selene Warnings.

95af18f

she3o requested a review from jalvesaq September 27, 2024 21:57

Allow placeholder in object browser custom commands.

3d89282

Add ability to define a custom placeholder for object browser custom …

7bf0fa3

…commands.

Revert buggy part of commit e2ce3c9

46bcd2f

she3o added 3 commits September 29, 2024 06:57

Clean up browser.lua

2f1805e

Revert "Clean up browser.lua"

bfed43c

This reverts commit 2f1805e.

Really clean up browser.lua

3e7e0ca

wurli mentioned this pull request Oct 4, 2024

Documentation: add some notes about versioning and rework the README a bit #240

Merged

she3o and others added 4 commits October 7, 2024 05:25

Fix Style and Startup warnings.

f6d7fe7

Merge remote-tracking branch 'origin/main' into clean-object-browser

34b65ac

This adds PR R-nvim#237 to this branch so that doc can be subsequently modified without introducing conflicts.

Minor fix: the option is not available on 0.1.0

ce81b75

jalvesaq merged commit e3fc311 into R-nvim:main Oct 7, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Object Browser and Add Support for Custom Key Mappings #237

Refactor Object Browser and Add Support for Custom Key Mappings #237

she3o commented Sep 27, 2024

jalvesaq commented Sep 27, 2024

she3o commented Sep 27, 2024

she3o commented Sep 29, 2024 •

edited

Loading

she3o commented Sep 29, 2024

she3o commented Sep 29, 2024

she3o commented Sep 29, 2024

PMassicotte commented Sep 29, 2024

jalvesaq commented Sep 30, 2024

she3o commented Sep 30, 2024

jalvesaq commented Sep 30, 2024

she3o commented Oct 1, 2024

jalvesaq commented Oct 1, 2024

she3o commented Oct 1, 2024

PMassicotte commented Oct 1, 2024

wurli commented Oct 1, 2024

jalvesaq commented Oct 1, 2024

wurli commented Oct 2, 2024 •

edited

Loading

PMassicotte commented Oct 2, 2024

wurli commented Oct 2, 2024

jalvesaq commented Oct 2, 2024

wurli commented Oct 2, 2024 •

edited

Loading

jalvesaq commented Oct 2, 2024

wurli commented Oct 2, 2024

wurli commented Oct 2, 2024

jalvesaq commented Oct 2, 2024

wurli commented Oct 2, 2024

jalvesaq commented Oct 2, 2024

wurli commented Oct 4, 2024

jalvesaq commented Oct 4, 2024

jalvesaq commented Oct 4, 2024

she3o commented Oct 6, 2024

jalvesaq commented Oct 7, 2024

Refactor Object Browser and Add Support for Custom Key Mappings #237

Refactor Object Browser and Add Support for Custom Key Mappings #237

Conversation

she3o commented Sep 27, 2024

1. Code Refactoring and Cleanup

Add Module Documentation

Support for Custom Key Mappings in Object Browser

jalvesaq commented Sep 27, 2024

she3o commented Sep 27, 2024

she3o commented Sep 29, 2024 • edited Loading

she3o commented Sep 29, 2024

she3o commented Sep 29, 2024

she3o commented Sep 29, 2024

PMassicotte commented Sep 29, 2024

jalvesaq commented Sep 30, 2024

she3o commented Sep 30, 2024

jalvesaq commented Sep 30, 2024

she3o commented Oct 1, 2024

jalvesaq commented Oct 1, 2024

she3o commented Oct 1, 2024

PMassicotte commented Oct 1, 2024

wurli commented Oct 1, 2024

jalvesaq commented Oct 1, 2024

wurli commented Oct 2, 2024 • edited Loading

PMassicotte commented Oct 2, 2024

wurli commented Oct 2, 2024

jalvesaq commented Oct 2, 2024

wurli commented Oct 2, 2024 • edited Loading

jalvesaq commented Oct 2, 2024

wurli commented Oct 2, 2024

wurli commented Oct 2, 2024

jalvesaq commented Oct 2, 2024

wurli commented Oct 2, 2024

jalvesaq commented Oct 2, 2024

wurli commented Oct 4, 2024

jalvesaq commented Oct 4, 2024

jalvesaq commented Oct 4, 2024

she3o commented Oct 6, 2024

jalvesaq commented Oct 7, 2024

she3o commented Sep 29, 2024 •

edited

Loading

wurli commented Oct 2, 2024 •

edited

Loading

wurli commented Oct 2, 2024 •

edited

Loading