-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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: set properties on namespaces #28728
base: master
Are you sure you want to change the base?
Conversation
fff7048
to
60665db
Compare
It may be better to be able to change a namespace from global scope to window scope in one API call. The API will be like |
0c96b4a
to
32e9982
Compare
Can we have a generic |
24cbad6
to
2ca429b
Compare
8d80e61
to
0c91dab
Compare
2f2787f
to
1fca1d1
Compare
ac306b4
to
9ff9697
Compare
87d940b
to
7bbe030
Compare
95d9399
to
fcaac92
Compare
src/nvim/api/extmark.h
Outdated
if (!set_has(uint32_t, &namespace_scoped, ns_id)) { | ||
return true; | ||
} | ||
|
||
return set_has(uint32_t, &wp->w_ns_set, ns_id); |
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 seems strange. If ns_id is not in namespace_scoped
, this function returns true
? Why isn't the decision fully decided by w_ns_set
, why is namespace_scoped
needed at all?
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.
namespace_scoped
is needed because it currently is equivalent to "namespace is scoped in at least one window" and if it doesn't contain the namespace, then that namespace is "global scoped" (which is what you asked for).
Though namespace_scoped
can sometimes contain namespaces that are not scoped in any windows, because if a window is deleted, and that window was the only window where the namespace was scoped, then namespace_scoped
isn't updated. (I could change this so that it's always synced, but that would entail that when a window is deleted, a namespace can suddenly become global scope, which is probably not what one wants.)
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.
right, the docstring and names were confusing.
namespace_scoped
is needed because it currently is equivalent to "namespace is scoped in at least one window"
Sounds like an unnecessary optimization. We don't need namespace_scoped
, we can iterate through all windows and check w_ns_set
. That is simpler and requires less bookkeeping.
oh I missed this:
but that would entail that when a window is deleted, a namespace can suddenly become global scope, which is probably not what one wants.
Ok. namespace_scoped
is confusing because "global scope" is also a scope. Maybe namespace_localscope
is a better name.
761d0be
to
acf7866
Compare
src/nvim/api/extmark.h
Outdated
EXTERN Map(String, int) namespace_ids INIT( = MAP_INIT); | ||
EXTERN Set(uint32_t) namespace_scoped INIT( = SET_INIT); |
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.
Is it too early to introduce (pseudocode):
namespaces: Map<id, NamespaceProps>
where NamespaceProps
is a struct like:
{ scope: NsScope; ... }
Arguably that is similar to namespace_scope_map
which I questioned for other reasons. 😓
4d9c8e7
to
df9e46b
Compare
Co-authored-by: Justin M. Keyes <[email protected]>
Use a singular function that can specify properties on the namespace.
Removed
nvim__win_xx_ns
functions as they are no longer needed.