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

Add Missing Internal Fields #318

Merged
merged 2 commits into from
Dec 28, 2023

Conversation

collindutter
Copy link
Contributor

@collindutter collindutter commented Dec 26, 2023

This PR resolves the following:

  1. LuaLS warning: Missing required fields in type `nui_popup_internal`: `position`, `size`.
  2. Error when opening a Menu in a Layout without the size field.

Very new to the project so please let me know if there are specific unit tests I should update/add!

Error:

Error executing Lua callback: ...er/.local/share/nvim/lazy/nui.nvim/lua/nui/menu/init.lua:80: attempt to index field 'size' (a nil value)
stack traceback:
	...er/.local/share/nvim/lazy/nui.nvim/lua/nui/menu/init.lua:80: in function 'make_default_prepare_node'
	...er/.local/share/nvim/lazy/nui.nvim/lua/nui/menu/init.lua:286: in function 'init'
	.../.local/share/nvim/lazy/nui.nvim/lua/nui/object/init.lua:132: in function 'Menu'
	...uments/griptape/griptape.nvim/lua/griptape/structure.lua:19: in function 'run_structure'
	...r/Documents/griptape/griptape.nvim/lua/griptape/init.lua:7: in function <...r/Documents/griptape/griptape.nvim/lua/griptape/init.lua:6>

To reproduce:

local Menu = require("nui.menu")
local Layout = require("nui.layout")

local menu = Menu({
	border = {
		style = "rounded",
	},
}, {
	lines = {
		Menu.item("Hydrogen (H)"),
	},
})
local layout = Layout(
	{
		position = "50%",
		size = {
			width = 80,
			height = "60%",
		},
	},
	Layout.Box({
		Layout.Box(menu, { size = "100%" }),
	}, { dir = "row" })
)

layout:mount()

Copy link

codecov bot commented Dec 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c9b4de6) 99.04% compared to head (34c7be3) 99.04%.

❗ Current head 34c7be3 differs from pull request most recent head 83beb09. Consider uploading reports for the commit 83beb09 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #318   +/-   ##
=======================================
  Coverage   99.04%   99.04%           
=======================================
  Files          20       20           
  Lines        2720     2723    +3     
=======================================
+ Hits         2694     2697    +3     
  Misses         26       26           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@MunifTanjim MunifTanjim Dec 27, 2023

Choose a reason for hiding this comment

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

Hey thanks for looking into it!

A fix like this would be more appropriate for it:

diff --git a/lua/nui/menu/init.lua b/lua/nui/menu/init.lua
index dff06a7..22b79de 100644
--- a/lua/nui/menu/init.lua
+++ b/lua/nui/menu/init.lua
@@ -283,7 +283,7 @@ function Menu:init(popup_options, options)
   self._.sep = options.separator
 
   self._.should_skip_item = defaults(options.should_skip_item, default_should_skip_item)
-  self._.prepare_item = defaults(options.prepare_item, make_default_prepare_node(self))
+  self._.prepare_item = options.prepare_item
 
   self.menu_props = {}
 
@@ -316,6 +316,13 @@ function Menu:init(popup_options, options)
   end
 end
 
+---@param config? nui_layout_options
+function Menu:update_layout(config)
+  Menu.super.update_layout(self, config)
+
+  self._.prepare_item = defaults(self._.prepare_item, make_default_prepare_node(self))
+end
+
 function Menu:mount()
   Menu.super.mount(self)
 

Here we're defering the execution of make_default_prepare_node if NuiMenu is used in a layout and prepare_item is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated PR! Thank you for the guidance 🙂

@MunifTanjim MunifTanjim merged commit cbfb97c into MunifTanjim:main Dec 28, 2023
4 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.

2 participants