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

feat(ui): keep track of recently used item #412

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

hinell
Copy link
Contributor

@hinell hinell commented Oct 18, 2023

remember-recent-selection.mov
  • Remember most recent selection per global list and per itemlist
  • Minor refactoring
  • For global itemlist, most recently selected item is kept at State.most_recent_item
  • For itemgroup-relative selection the State.itemgroup_history[id] map is used

How to Test

  • Exposed APIs aren't affected; only UX;

Testing for Regressions

I have tested the following:

  • Triggering keymaps from legendary.nvim in all modes (normal, insert, visual)
  • Creating keymaps via legendary.nvim, then triggering via the keymap in all modes (normal, insert, visual)
  • Triggering commands from legendary.nvim in all modes (normal, insert, visual)
  • Creating commands via legendary.nvim, then running the command manually from the command line
  • augroup/autocmds created through legendary.nvim work correctly

@mrjones2014
Copy link
Owner

Thanks for the PR! Neat idea. My initial thought is that ItemGroups are themselves items as well, so we probably don't need a new state, we can probably just update the existing one to be a list instead of a single value. I'll keep looking at this and make some suggestions.

@hinell hinell changed the title feat(State): keep track of recently used item feat(ui): keep track of recently used item Oct 19, 2023
@hinell
Copy link
Contributor Author

hinell commented Oct 19, 2023

so we probably don't need a new state, we can probably just update the existing one to be a list instead of a single value.

Before this PR, this mechanism was flawed: legendary didn't remember selection inside groups, which was really annoying...

@mrjones2014
Copy link
Owner

Yeah, I took a look and the existing piece of state is also used for repeating last execution, so we'll need to keep that separate. Will give it a code review this morning.

Copy link
Owner

@mrjones2014 mrjones2014 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to update the documentation as well.

lua/legendary/data/itemlist.lua Show resolved Hide resolved
lua/legendary/data/itemlist.lua Outdated Show resolved Hide resolved
lua/legendary/data/state.lua Outdated Show resolved Hide resolved
lua/legendary/data/state.lua Outdated Show resolved Hide resolved
lua/legendary/ui/init.lua Outdated Show resolved Hide resolved
lua/legendary/ui/init.lua Outdated Show resolved Hide resolved
@mrjones2014
Copy link
Owner

For docs changes this is all we should need:

diff --git a/README.md b/README.md
index 5bb6537..5eea0db 100644
--- a/README.md
+++ b/README.md
@@ -385,7 +385,9 @@ require('legendary').setup({
   --  }
   -- })
   sort = {
-    -- sort most recently used item to the top
+    -- sort most recently used item to the top,
+    -- this works for both the top-level item list
+    -- as well as items inside of item groups
     most_recent_first = true,
     -- sort user-defined items before built-in items
     user_items_first = true,

@hinell hinell force-pushed the master branch 2 times, most recently from 0955adf to f5447d1 Compare October 19, 2023 12:54
* For global itemlist, most recently selected items is kept at `State.most_recent_item`
* For itemgroup-relative selection, use `State.most_recent_igroups[id]`
@mrjones2014
Copy link
Owner

mrjones2014 commented Oct 19, 2023

@hinell I checked out locally and prepared a patch. Here's the remaining changes we need to make:

diff --git a/lua/legendary/api/executor.lua b/lua/legendary/api/executor.lua
index ee7c175..f164e11 100644
--- a/lua/legendary/api/executor.lua
+++ b/lua/legendary/api/executor.lua
@@ -1,6 +1,7 @@
 local Toolbox = require('legendary.toolbox')
 local Log = require('legendary.log')
 local Config = require('legendary.config')
+local State = require('legendary.data.state')
 local util = require('legendary.util')
 
 local function update_item_frecency_score(item)
@@ -83,6 +84,7 @@ end
 function M.exec_item(item, context)
   vim.schedule(function()
     M.restore_context(context, function()
+      State.last_executed_item = item
       update_item_frecency_score(item)
       if Toolbox.is_function(item) then
         item.implementation()
@@ -123,11 +125,11 @@ end
 ---@param ignore_filters boolean|nil whether to ignore the filters used when selecting the item, default false
 function M.repeat_previous(ignore_filters)
   local State = require('legendary.data.state')
-  if State.most_recent_item then
+  if State.last_executed_item then
     if not ignore_filters and State.most_recent_filters then
       for _, filter in ipairs(State.most_recent_filters) do
         -- if any filter does not match, abort executions
-        local err, matches = pcall(filter, State.most_recent_item)
+        local err, matches = pcall(filter, State.last_executed_item)
         if not err and not matches then
           Log.warn(
             'Previously executed item no longer matches previously used filters, use `:LegendaryRepeat!`'
@@ -138,7 +140,7 @@ function M.repeat_previous(ignore_filters)
       end
     end
     local context = M.build_context()
-    M.exec_item(State.most_recent_item, context)
+    M.exec_item(State.last_executed_item, context)
   end
 end
 
diff --git a/lua/legendary/data/itemlist.lua b/lua/legendary/data/itemlist.lua
index a81bfa7..5d5c51e 100644
--- a/lua/legendary/data/itemlist.lua
+++ b/lua/legendary/data/itemlist.lua
@@ -14,6 +14,8 @@ local Log = require('legendary.log')
 ---@field private sorted boolean
 local ItemList = class('ItemList')
 
+ItemList.TOPLEVEL_LIST_ID = 'toplevel'
+
 ---@private
 function ItemList:initialize()
   self.items = {}
@@ -124,7 +126,13 @@ function ItemList:sort_inplace(opts)
 
   -- if no items have been added, and the most recent item has not changed,
   -- we're already sorted
-  if self.sorted and (not opts.most_recent_first or (self.items[1] == State.most_recent_item)) then
+  if
+    self.sorted
+    and (
+      not opts.most_recent_first
+      or (self.items[1] == State.itemgroup_history[opts.itemgroup or ItemList.TOPLEVEL_LIST_ID])
+    )
+  then
     return
   end
 
@@ -188,10 +196,8 @@ function ItemList:sort_inplace(opts)
     end
 
     if opts.most_recent_first then
-      if opts.itemgroup then
-        return item1 == State.itemgroup_history[opts.itemgroup]
-      else
-        return item1 == State.most_recent_item
+      if item1 == State.itemgroup_history[opts.itemgroup or ItemList.TOPLEVEL_LIST_ID] then
+        return true
       end
     end
 
@@ -224,9 +230,11 @@ function ItemList:sort_inplace(opts)
   -- sort by most recent last, and after other sorts are done
   -- if most recent is already at top, nothing to do, and attempting to sort will cause
   -- an error since it doesn't need to be sorted
-  if opts.most_recent_first and State.most_recent_item and State.most_recent_item ~= self.items[1] then
+  if
+    opts.most_recent_first and State.itemgroup_history[opts.itemgroup or ItemList.TOPLEVEL_LIST_ID] ~= self.items[1]
+  then
     items = Sorter.mergesort(items, function(item)
-      return item == State.most_recent_item
+      return item == State.itemgroup_history[opts.itemgroup or ItemList.TOPLEVEL_LIST_ID]
     end)
   end
 
diff --git a/lua/legendary/data/state.lua b/lua/legendary/data/state.lua
index bbbc78a..40249ac 100644
--- a/lua/legendary/data/state.lua
+++ b/lua/legendary/data/state.lua
@@ -2,13 +2,13 @@ local ItemList = require('legendary.data.itemlist')
 
 ---@class LegendaryState
 ---@field items ItemList
----@field most_recent_item LegendaryItem|nil
+---@field last_executed_item LegendaryItem|nil
 ---@field most_recent_filters LegendaryItemFilter[]|nil
----@field itemgroup_history ItemList[]
+---@field itemgroup_history table<string, LegendaryItem>
 local M = {}
 
 M.items = ItemList:create()
-M.most_recent_item = nil
+M.last_executed_item = nil
 M.most_recent_filters = nil
 M.itemgroup_history = {}
 
diff --git a/lua/legendary/ui/init.lua b/lua/legendary/ui/init.lua
index d33b0ab..17aa10e 100644
--- a/lua/legendary/ui/init.lua
+++ b/lua/legendary/ui/init.lua
@@ -6,12 +6,13 @@ local Toolbox = require('legendary.toolbox')
 local Format = require('legendary.ui.format')
 local Executor = require('legendary.api.executor')
 local Log = require('legendary.log')
+local ItemList = require('legendary.data.itemlist')
 
 ---@class LegendaryUi
 ---@field select fun(opts:LegendaryFindOpts)
 local M = {}
 
----@class LegendaryFindOpts
+---@class LegendaryFindOpts : ItemListSortInplaceOpts
 ---@field itemgroup string Find items in this item group only
 ---@field filters LegendaryItemFilter[]
 ---@field select_prompt string|fun():string
@@ -82,11 +83,7 @@ local function select_inner(opts, context, itemlist)
       return
     end
 
-    if opts.itemgroup then
-      State.itemgroup_history[opts.itemgroup] = selected
-    else
-      State.most_recent_item = selected
-    end
+    State.itemgroup_history[opts.itemgroup or ItemList.TOPLEVEL_LIST_ID] = selected
 
     if Toolbox.is_itemgroup(selected) then
       local item_group_id = selected:id()

@mrjones2014
Copy link
Owner

If you apply the patch, I'll be happy to test and merge!

@mrjones2014
Copy link
Owner

@hinell one small issue from CI, then I can merge:

Checking lua/legendary/api/executor.lua           1 warning

    lua/legendary/api/executor.lua:127:9: shadowing upvalue State on line 4

@dundargoc
Copy link

I dunno what legendary.nvim is, but this is clearly critical. Smashed that approve button!

@mrjones2014
Copy link
Owner

I dunno what legendary.nvim is

lol you approved the PR two hours ago though 😆

@mrjones2014 mrjones2014 merged commit a4a394c into mrjones2014:master Oct 19, 2023
2 checks passed
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.

3 participants