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.

Sign up for GitHub

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

Fixing comment plugin not using user settings when overriding default setting #3424

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fixing comment plugin not using user settings when overriding default setting #3424

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d40a952
Updating comment plugin option and fixing comment option reset bug
Neko-Box-Coder Aug 22, 2024
b4920fb
Added error message when using old comment option
Neko-Box-Coder Aug 22, 2024
7134d3f
Rewording error message for old comment option
Neko-Box-Coder Aug 22, 2024
5dc41ce
Fixing old comment option error message being spammed
Neko-Box-Coder Aug 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions runtime/plugins/comment/comment.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,15 @@ ft["zig"] = "// %s"
ft["zscript"] = "// %s"
ft["zsh"] = "# %s"

local last_ft

function updateCommentType(buf)
if buf.Settings["commenttype"] == nil or (last_ft ~= buf.Settings["filetype"] and last_ft ~= nil) then
-- NOTE: Don't use SetOptionNative() to set "comment.type",
-- otherwise "comment.type" can't be reset by a "filetype" change.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not nearly obvious why can't it be reset by a "filetype" change if we use SetOptionNative() (it took me some time to figure it out, for instance). We should explain it better, e.g. "Don't use SetOptionNative() to set "comment.type", otherwise "comment.type" will be marked as locally overridden and will not be updated when "filetype" changes."

Or actually it seems better to:

  1. use DoSetOptionNative() instead of setting the option directly
  2. add documentation to both SetOptionNative() and DoSetOptionNative(), so that everyone knows what is the difference between them and when to use each of them, and then we don't need this comment here.

...Another option would be to somehow extend RegisterCommonOption() to allow registering default per-filetype values of an option, not just its default global value, at init time, - so that updateCommentType() would not be needed at all.

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Aug 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@dmaluka

DoSetOptionNative() seems like an internal function to me, the name of it is also quite confusing against SetOptionNative() as well.

Would adding a function like SetOptionPersistence(bool persistent) which sets b.LocalSettings[option] be enough for now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As matter of fact, the line between internal and external functions here is pretty blurred. (Technically DoSetOptionNative() is external, it is already exported to plugins, although not on purpose but as a side effect of exporting it to other modules inside micro, which is a consequence of the bizarre design of micro's plugin system.)

Would adding a function like SetOptionPersistence(bool persistent) which sets b.LocalSettings[option]

Maybe... @JoeKar what do you think?

Copy link
Collaborator

@JoeKar JoeKar Aug 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is not nearly obvious why can't it be reset by a "filetype" change if we use SetOptionNative() (it took me some time to figure it out, for instance).

I thought you can remember, after the long review road of #3343. 😉

Would adding a function like SetOptionPersistence(bool persistent) which sets b.LocalSettings[option]

Maybe... @JoeKar what do you think?

So SetOptionNative() calls then SetOptionNativeMark(option string, nativeValue interface{}, mark(Local) bool).
The same then for consistent reason for SetGlobalOptionNative() -> setGlobalOptionNativeMark(option string, nativeValue interface{}, mark(Modified) bool)

The word persistent resp. persistence smells a bit of writing it persistent into the users configuration.

BTW: Wasn't the extension with a further parameter of these functions temporary part of #3343, but rejected due to the fact a of the introduction of a further parameter?

Anyway, if fine with that adjustment, in case it helps to improve the code/interfaces. Indeed we can then drop the introduced comment in the comment plugin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think what @Neko-Box-Coder meant is a separate function just for toggling the option's persistence state, independently of setting the option value.

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Aug 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

And also I forgot to mention is I would like to not break back compatibility as well for SetOptionNative() and SetGlobalOptionNative().

So ideally changing those 2 functions (name and parameters I guess) would be our last resort if possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, these two should stay as they are, but we can rename those two called from them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, why not simply:

diff --git a/internal/buffer/settings.go b/internal/buffer/settings.go
index 838df4a5..0a05a67e 100644
--- a/internal/buffer/settings.go
+++ b/internal/buffer/settings.go
@@ -45,6 +45,11 @@ func (b *Buffer) ReloadSettings(reloadFiletype bool) {
 	}
 }
 
+// DoSetOptionNative is a low-level function which just sets an option to a value
+// for this buffer, overriding the global setting. Unlike SetOption and SetOptionNative
+// it doesn't validate the option and doesn't mark it as overridden, so setting
+// an option via DoSetOptionNative doesn't prevent it from being reset to its
+// global value by the "reload" command or by changing the filetype.
 func (b *Buffer) DoSetOptionNative(option string, nativeValue interface{}) {
 	if reflect.DeepEqual(b.Settings[option], nativeValue) {
 		return
@@ -119,6 +124,8 @@ func (b *Buffer) DoSetOptionNative(option string, nativeValue interface{}) {
 	}
 }
 
+// SetOptionNative sets a given option to a value just for this buffer, overriding
+// the global setting.
 func (b *Buffer) SetOptionNative(option string, nativeValue interface{}) error {
 	if err := config.OptionIsValid(option, nativeValue); err != nil {
 		return err
@@ -130,7 +137,8 @@ func (b *Buffer) SetOptionNative(option string, nativeValue interface{}) error {
 	return nil
 }
 
-// SetOption sets a given option to a value just for this buffer
+// SetOption sets a given option to a value just for this buffer, overriding
+// the global setting. The value is a string the actual value is parsed from.
 func (b *Buffer) SetOption(option, value string) error {
 	if _, ok := b.Settings[option]; !ok {
 		return config.ErrInvalidOption
diff --git a/runtime/plugins/comment/comment.lua b/runtime/plugins/comment/comment.lua
index 4a016bfd..f9e85931 100644
--- a/runtime/plugins/comment/comment.lua
+++ b/runtime/plugins/comment/comment.lua
@@ -63,8 +63,6 @@ ft["zscript"] = "// %s"
 ft["zsh"] = "# %s"
 
 function updateCommentType(buf)
-    -- NOTE: Don't use SetOptionNative() to set "comment.type",
-    -- otherwise "comment.type" can't be reset by a "filetype" change.
     if buf.Settings["comment.type"] == "" then
         if buf.Settings["commenttype"] ~= nil then
             micro.InfoBar():Error("\"commenttype\" option has been renamed to \"comment.type\"",
@@ -72,9 +70,9 @@ function updateCommentType(buf)
         end
 
         if ft[buf.Settings["filetype"]] ~= nil then
-            buf.Settings["comment.type"] = ft[buf.Settings["filetype"]]
+            buf:DoSetOptionNative("comment.type", ft[buf.Settings["filetype"]])
         else
-            buf.Settings["comment.type"] = "# %s"
+            buf:DoSetOptionNative("comment.type", "# %s")
         end
     end
 end

Copy link
Contributor Author

@Neko-Box-Coder Neko-Box-Coder Aug 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's fine to add documentation I think but it would be difficult to rename or change the function signature afterwards if we decide to use it in the plugin (which other people will follow and do the same as well if needed)

I still stand by what I said before:

It's just I am not sure how to rename DoSetOptionNative() such that it is not confusing and doesn't require a comment explaining what the difference is.

Would it work if I add a proxy function called something like SetNonReloadableOptionNative() (or some other names) which does the same as SetOptionNative() but without setting the LocalSettings field.

Or even a step further where we make DoSetOptionNative() as private just like doSetGlobalOptionNative(). The only place it is using DoSetOptionNative() is internal/action/command.go:608 after all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think DoSetOptionNative is a particularly bad name (but you are welcome to suggest a better one). It aligns with what it does (and what is described in the documentation I've suggested above): just sets the option for this buffer and does nothing more.

Also I hope it is unlikely that any other plugin will actually want to use it. The comment plugin's use case is unusual in this regard.

Or even a step further where we make DoSetOptionNative() as private just like doSetGlobalOptionNative(). The only place it is using DoSetOptionNative() is internal/action/command.go:608 after all.

internal/action/command.go is in a different package. Which is why we made DoSetOptionNative() public in the first place. Try making it private and compiling micro.

if buf.Settings["comment.type"] == "" then
if ft[buf.Settings["filetype"]] ~= nil then
buf:SetOptionNative("commenttype", ft[buf.Settings["filetype"]])
buf.Settings["comment.type"] = ft[buf.Settings["filetype"]]
else
buf:SetOptionNative("commenttype", "# %s")
buf.Settings["comment.type"] = "# %s"
end

last_ft = buf.Settings["filetype"]
end
end

Expand All @@ -88,7 +86,7 @@ function commentLine(bp, lineN, indentLen)
updateCommentType(bp.Buf)

local line = bp.Buf:Line(lineN)
local commentType = bp.Buf.Settings["commenttype"]
local commentType = bp.Buf.Settings["comment.type"]
local sel = -bp.Cursor.CurSelection
local curpos = -bp.Cursor.Loc
local index = string.find(commentType, "%%s") - 1
Expand All @@ -114,7 +112,7 @@ function uncommentLine(bp, lineN, commentRegex)
updateCommentType(bp.Buf)

local line = bp.Buf:Line(lineN)
local commentType = bp.Buf.Settings["commenttype"]
local commentType = bp.Buf.Settings["comment.type"]
local sel = -bp.Cursor.CurSelection
local curpos = -bp.Cursor.Loc
local index = string.find(commentType, "%%s") - 1
Expand Down Expand Up @@ -178,7 +176,7 @@ end
function comment(bp, args)
updateCommentType(bp.Buf)

local commentType = bp.Buf.Settings["commenttype"]
local commentType = bp.Buf.Settings["comment.type"]
local commentRegex = "^%s*" .. commentType:gsub("%%","%%%%"):gsub("%$","%$"):gsub("%)","%)"):gsub("%(","%("):gsub("%?","%?"):gsub("%*", "%*"):gsub("%-", "%-"):gsub("%.", "%."):gsub("%+", "%+"):gsub("%]", "%]"):gsub("%[", "%["):gsub("%%%%s", "(.*)")

if bp.Cursor:HasSelection() then
Expand All @@ -204,6 +202,10 @@ function string.starts(String,Start)
return string.sub(String,1,string.len(Start))==Start
end

function preinit()
config.RegisterCommonOption("comment", "type", "")
end

function init()
config.MakeCommand("comment", comment, config.NoComplete)
config.TryBindKey("Alt-/", "lua:comment.comment", false)
Expand Down