-
Notifications
You must be signed in to change notification settings - Fork 83
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
Fix state management issues #573
Conversation
--- @field buffers integer[] different from `state.buffers` in that the `hide` option is respected. Only updated when calling `calculate_buffers_width`. | ||
local layout = { buffers = {} } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full state should be kept in state
, makes things simple and debuggable. This has been moved to state.buffers_visible
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sidenote but I don't like that our state is gaining fields but still relies on manual mutation for updating. Some sort of automated reactivity would probably be much less bug-prone. Don't have any solution for it though, and it might be overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make note of this to take a look at in the future. I agree that some of these fields would benefit from being self contained, and between the two of us I'm sure there's a way to make that happen :)
--- @param state barbar.State | ||
--- @return barbar.ui.layout.data | ||
function layout.calculate() | ||
function layout.calculate(state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The layout
code is now pure and takes state
as a param.
@@ -85,202 +69,12 @@ local scroll = { current = 0, target = 0 } | |||
--- @class barbar.ui.Render | |||
local render = {} | |||
|
|||
--- An incremental animation for `close_buffer_animated`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The open/close code has been moved back to state
, where state management should live. I think this might have been done to avoid circular dependencies. I've solved the state/render dependency by adding state.update_callback
, which is set to render.update
in render.lua
. Think of it like an event listener callback, when the state changes, the update callback is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is important for each module to have a single responsibility, and that support of additional responsibilities should be built on top of it rather than into it.
In my view state
's job is:
- Define what the shape of the shared state is
- Provide a minimal interface to read and write state that ensures internal integrity
What do you think about splitting out the other responsibilities (e.g. get_updated_buffers
, xyz_animated
) into other modules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functions I've moved here are too tied to the state to live elsewhere. The animations require setting fields on the state object, there's no avoiding that. I'm open to suggestions but I don't think it will be possible to split them like we do for the layout module.
If you can think of a good split point me out which functions you'd split, and which name you'd give to this new module. Having a good name is a good indicator that we have a clean split, but I don't see one at the moment.
lua/barbar/bbye.lua
Outdated
function bbye.delete(action, force, buffer, mods) | ||
local state = require('barbar.state') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't want the get_focus_on_close
code in bbye, but couldn't solve the circular dependency. This is the only non-top level require
hack I've added. Keeping bbye simple is more important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a bbye
submodule would be the best place? We could put other local function
s in them too. Maybe something like:
bbye.cmd
: parsing(action, force, buffer, mods)
into a VimL command.bbye.exec
/run
/strategy
:delete
subroutines (e.g.get_focus_on_close
)
(Feel free to use different names there…)
Alternatively, if we move forward with the suggestion of breaking out state
into separate modules, require('barbar.state')
might be fine to move to the top of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the hack, wasn't necessary after the refactor. State is now leaf enough that bbye can import it without circular dependency issue. The big issue was the state/render interdependency that is now solved with the event listener pattern.
bbye ->
api -> render --------> state ------------> config/animate/constants/utils
--------> layout ----------->
I don't think splitting bbye
is a good idea at the moment, it's small enough.
Sidenote but I don't love using new features unless they're needed, e.g. this example. If the old way works, let's keep using the old way until we feel like increasing our minimum supported neovim version. Code is always a liability, the less of it we have, the happier I am.
local enew = vim.api.nvim_cmd and
--- The `:enew` command
--- @param force boolean
function(force) vim.cmd.enew {bang = force} end or
--- The `:enew` command
--- @param force boolean
function(force) command("enew" .. (force and '!' or '')) end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sidenote but I don't love using new features unless they're needed, e.g. this example.
Will (won't?) do. With this, we should also reconsider what minimum version we want to support in #394.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah not sure how often people update their neovim. Probably supporting whatever version debian stable has would be a good conservative target. 0.7 at the moment.
@@ -38,6 +46,7 @@ local WARN = severity.WARN | |||
|
|||
--- @class barbar.state.buffer.data | |||
--- @field closing boolean whether the buffer is being closed | |||
--- @field will_close boolean whether the buffer will be closed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the fix per-se for #550, we mark the buffer to avoid focusing it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will_close
and closing
seem to serve similar purposes, so would an enum
be better here?
For example:
--- @enum buffer_state
local buffer_state = {
open = 0,
will_close = 1,
closing = 2,
}
--- @field closing buffer_state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would feel cleaner but I noticed some issues when re-using closing
and I don't have enough time to make the change and debug possible issues, I'll leave as it is for now, we can refactor later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed some issues when re-using closing and I don't have enough time to make the change and debug possible issues, I'll leave as it is for now, we can refactor later
Sounds good.
As a note for later, bit flags might be better (e.g. bit.band(is_open, state.closing)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree completely with the motivation of the changes, and with what needs to change; I really like the layout
changes especially 😀
The only discussion I'm hoping to have is how to break up certain modules in the name of single-responsibility and dependency resolution
@@ -38,6 +46,7 @@ local WARN = severity.WARN | |||
|
|||
--- @class barbar.state.buffer.data | |||
--- @field closing boolean whether the buffer is being closed | |||
--- @field will_close boolean whether the buffer will be closed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will_close
and closing
seem to serve similar purposes, so would an enum
be better here?
For example:
--- @enum buffer_state
local buffer_state = {
open = 0,
will_close = 1,
closing = 2,
}
--- @field closing buffer_state
lua/barbar/bbye.lua
Outdated
function bbye.delete(action, force, buffer, mods) | ||
local state = require('barbar.state') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a bbye
submodule would be the best place? We could put other local function
s in them too. Maybe something like:
bbye.cmd
: parsing(action, force, buffer, mods)
into a VimL command.bbye.exec
/run
/strategy
:delete
subroutines (e.g.get_focus_on_close
)
(Feel free to use different names there…)
Alternatively, if we move forward with the suggestion of breaking out state
into separate modules, require('barbar.state')
might be fine to move to the top of the file.
} | ||
|
||
local constants = { | ||
ANIMATION = ANIMATION, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would these work attached to their relevant modules, e.g. animation.OPEN_DELAY
? It seems constants.ANIMATION
is always used alongside the animation
module, so no dependency resolution problems should be introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constants is a leaf module, it won't import anything, so it won't pose problems. I'd rather keep animation
abstract of barbar details, it's a cleaner separation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like animation is very pure in that I could copy/paste the file in another project without having to do any modification to adapt it. If we add ANIMATION
constants there, we lose that property, and I feel like that property is a good indicative of low-coupling modules.
@@ -85,202 +69,12 @@ local scroll = { current = 0, target = 0 } | |||
--- @class barbar.ui.Render | |||
local render = {} | |||
|
|||
--- An incremental animation for `close_buffer_animated`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is important for each module to have a single responsibility, and that support of additional responsibilities should be built on top of it rather than into it.
In my view state
's job is:
- Define what the shape of the shared state is
- Provide a minimal interface to read and write state that ensures internal integrity
What do you think about splitting out the other responsibilities (e.g. get_updated_buffers
, xyz_animated
) into other modules?
`layout.buffers` was moved in #573 to `state.buffers_visible`
`layout.buffers` was moved in #573 to `state.buffers_visible`
Closes #550
Avoid focusing back on a closing buffer.