Fixing comment plugin not using user settings when overriding default setting

[Neko-Box-Coder]

If you are overriding the default comment format say
("//%s" instead of "// %s")

"*.cpp": {
    "commenttype": "//%s"
}

This will get ignored when switching between different file types and the default will be used again.

And also cleaned up unnecessary (and flawed) logic since we should always honor the buffer settings anyway.

[dmaluka]

"*.cpp": {
    "commenttype": "//%s"
}

You probably meant the following?

"ft:c++": {
    "commenttype": "//%s"
}

since with "*.cpp" the setting is associated with the file name, not with the file type, so it should not change after set filetype.

...So, with this PR and the above "ft:c++" setting, when I open foo.go and then run set filetype c++, the setting indeed takes effect, however, after that, when I run set filetype go, it doesn't restore the original setting.

[Neko-Box-Coder]

since with "*.cpp" the setting is associated with the file name, not with the file type, so it should not change after set filetype.

It does change currently when without this PR. It overwrites the setting from the user which is set in the beginning when opening the buffer.

...So, with this PR and the above "ft:c++" setting, when I open foo.go and then run set filetype c++, the setting indeed takes effect, however, after that, when I run set filetype go, it doesn't restore the original setting.

The problem here is it registers the comment type when you do a comment.

So yes... although your example is a bit unlikely, I get what you are saying.
A more likely scenario but still introduce the problem in the same way would be:

1. the user opens a file that micro doesn't recognize or creates a new buffer or micro recognize the file type wrongly
2. then the user invoke the comment action, which registers the "wrong" comment type
3. then the user sets the correct file type and invokes the comment action, which will still continue to be wrong.

[Neko-Box-Coder]

Actually, even for custom filetypes that are not "overriding" the default, this bug would still manifest because of the if statement

micro/runtime/plugins/comment/comment.lua

Lines 67 to 75 in 21bb61c

	if buf.Settings["commenttype"] == nil or (last_ft ~= buf.Settings["filetype"] and last_ft ~= nil) then
	if ft[buf.Settings["filetype"]] ~= nil then
	buf.Settings["commenttype"] = ft[buf.Settings["filetype"]]
	else
	buf.Settings["commenttype"] = "# %s"
	end
	last_ft = buf.Settings["filetype"]
	end

[JoeKar]

Just a thought:
Maybe RegisterCommonOptionPlug resp. within the plugin RegisterCommonOption is an option?
But again, the compatibility to commenttype would brake, since it's then called comment.commenttype or comment.type.

Upon filetype changes this option can then be cleared and the proper reaction can take place.

[Neko-Box-Coder]

Okay, how about now @dmaluka @JoeKar

I have added a setting called commentfiletype (although I am leaning towards calling it internal_commentfiletype to make it clear it is for internal use) which just records what file type commenttype is for.

It will detect file type change and apply the corresponding comment format correctly.

The new change will also update the comment format table according to the user settings for any new buffers.
So for example if you override the go comment to be

"ft:go": {
    "commenttype": "/* %s */"
}

for whatever reason, it will update the table so that this will be used next time.

So if you have the above json and

1. open a .go file
2. invoke comment (so that it registers it)
3. change the file type to something else like setlocal filetype json
4. invoke comment again
5. then set the filetype back to go and invoke the comment

The user settings will be applied instead of the default one.

The only niche use case this will break is if you want a specific commenttype for only 1 buffer and not apply to other buffers with the same file type, this will not work.

However, the original implementation doesn't work for this niche use case anyway since the moment you switch to a different buffer, your commenttype for that specific buffer will be gone. So it's not like this is breaking backward compatibility if it was never working 😂

[JoeKar]

Due to the merge of #3343 a rebase is necessary.

I have added a setting called commentfiletype (although I am leaning towards calling it internal_commentfiletype to make it clear it is for internal use) which just records what file type commenttype is for.

See my #3424 (comment) above. RegisterCommonOption() will create/register a plugin specific option including default value, which can be used for further tracking and should be touched now (at least in theory) in case of file type changes by micro itself.

[JoeKar]

The only niche use case this will break is if you want a specific commenttype for only 1 buffer and not apply to other buffers with the same file type, this will not work.

This should do the trick for all the use cases (base is current master):

diff --git a/runtime/plugins/comment/comment.lua b/runtime/plugins/comment/comment.lua
index f86da945..50d63ee5 100644
--- a/runtime/plugins/comment/comment.lua
+++ b/runtime/plugins/comment/comment.lua
@@ -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.
+    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
 
@@ -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
@@ -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
@@ -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
@@ -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)

[Neko-Box-Coder]

@JoeKar
Thanks a lot. Yeah your approach is much better. It is working as expected now.
I also added an error message when the old option is being used to remind the user to update their settings.

[dmaluka]

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.

[Neko-Box-Coder]

@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?

[dmaluka]

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?

[JoeKar]

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.

[dmaluka]

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.

[Neko-Box-Coder]

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.

[JoeKar]

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

[dmaluka]

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

[Neko-Box-Coder]

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.

[dmaluka]

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.

[dmaluka]

After changing commenttype to comment.type in settings.json and running the reload command, micro still stubbornly shows this error.