Rework `filetype` change, `reload` command and `autosave` #3343

JoeKar · 2024-06-13T20:15:37Z

the change of the filetype could cause unexpected behavior depending on global options being reset, while a volatile setting has been used.
the reload command could cause the same unexpected behavior and additionally doesn't really reload the user settings
autosave is now aborted in case his value has changed to something smaller or equal to 0.

dmaluka · 2024-06-16T13:37:50Z

This is not the only issue here. The reload command or setting filetype indeed changes the autosave value, but as I can see, this value change has no effect: the file is still autosaved according to the old setting. (This issue is orthogonal to the issue of whether the setting is a volatile setting and should not be changed by reload in the first place.)

So it seems that instead of hacking InitGlobalSettings() we should rework the reload behavior: it should not call InitGlobalSettings() but rather call SetGlobalOptionNative() for every option that changed (at least), to make the changes take effect.

And as for the filetype setting, do we really need to re-read and reinitialize global settings when changing the filetype? Shouldn't we really just call InitLocalSettings() then? (I see that was added in 3ef0267 to fix the issue Bug: when manually running set filetype <type>, micro should reload appropriate settings from the config file for that type, but it doesn't #2712, but that issue was not really about updating settings because settings.json changed, it was just about updating per-filetype settings because the filetype changed, right?)

After such 2 changes, InitGlobalSettings() would be only called at micro startup, before initializing volatile settings, so we wouldn't need to change it.

dmaluka · 2024-06-16T13:38:25Z

This issue is orthogonal to the issue of whether the setting is a volatile setting and should not be changed by reload in the first place.

BTW maybe we actually do want reload to change volatile settings as well?

JoeKar · 2024-06-17T10:55:27Z

[...] but as I can see, this value change has no effect: the file is still autosaved according to the old setting.

I saw something different, otherwise I wouldn't have created the PR.
Anyway, I agree that taking care of this in the two mentioned scenarios seems to be the better approach.

BTW maybe we actually do want reload to change volatile settings as well?

Hm, depends on what the community and or us really expects from that command to do.
At least for now it's somehow undocumented/undefined what shall happen in the scenario the user applied a local resp. volatile option and uses reload afterwards in the same session.

dmaluka · 2024-06-17T19:17:23Z

I saw something different

Are you sure? I've checked again that with autosave and other options that are specially handled by SetGlobalOptionNative() and buffer.SetOptionNative() (e.g. hlsearch, mouse), if I run e.g. micro -mouse false and then run reload in micro, then show mouse shows that mouse is set to true, however the mouse is still disabled.

Moreover, I see it's even worse: in the basic case when I change an option value in settings.json by editing settings.json (while micro is running) and then run reload, the option value doesn't change at all.

internal/buffer/settings.go

dmaluka · 2024-06-17T19:24:55Z

runtime/help/commands.md

+   In case `micro` was started with a `-<option>` being set to something other
+   than the default value and other than set within the `settings.json`
+   then this volatile option will be reverted to the default or to the value
+   defined in `settings.json`.


The reload description is expanded also in PR #3240 (still not merged, but IMHO it is ready for merge).
This change would make it even more lengthy and confusing.

...Now it actually seems a bit more reasonable to me if reload will not change volatile settings. Its job is to update those settings that were loaded from settings.json (since settings.json changed), arguably it should not update those settings that were not loaded from settings.json in the first place.

This change would make it even more lengthy and confusing.

Ok, drop it (locally) already.

...Now it actually seems a bit more reasonable to me if reload will not change volatile settings.

Yes, that's an other way around. Sounds feasible and leaves the changed volatiles alone.

Sounds feasible and leaves the changed volatiles alone.

At least as long as they aren't changed by the settings.json too.

JoeKar · 2024-06-18T17:53:02Z

Are you sure?

Yep, tried it now once again with the current state (set filetype without config.InitGlobalSettings()) against the master...

current:

$ micro -autosave 20 test # filetype unknown
modify
set filetype ini
Ctrl-Q
-> no prompt (y/n/esc)
-> modification stored

master:

$ micro -autosave 20 test # filetype unknown
modify
set filetype ini
Ctrl-Q
-> prompt (y/n/esc)

dmaluka · 2024-06-18T21:26:35Z

Yep [...]

Ah... you checked that save without prompt on exit gets disabled after set filetype ini, but you didn't check if autosave every 20 seconds gets disabled after set filetype ini. It doesn't get disabled. Try it.

JoeKar · 2024-06-19T17:05:59Z

master:

$ micro -autosave 20 test # filetype unknown
show autosave -> 20
set filetype ini
show autosave -> 0
autosaved after 20s

current:

$ micro -autosave 20 test # filetype unknown
show autosave -> 20
set filetype ini
show autosave -> 20
autosaved after 20s

Ok, you're right...saw it now. Totally weird.
autosave still needs a fix to be disabled, after it was started initially.

internal/action/command.go

internal/config/autosave.go

JoeKar · 2024-07-15T19:45:08Z

Hopefully I didn't miss something after my absence.
In case of please poke as usual.

internal/action/command.go

dmaluka · 2024-07-15T22:11:57Z

internal/buffer/settings.go

-			screen.TermMessage(err)
-		}
+		b.Settings = config.DefaultCommonSettings()
+		b.Settings[option] = nativeValue


Why so tricky? The option will be set in b.Settings by b.SetOptionNative() anyway, so why not just:

for k := range config.DefaultCommonSettings() { ... }

Also it would be nice to do this in a separate commit: one commit to remove the undesired stuff for globals and unneeded parsing, and another commit to add the missing stuff for locals.

dmaluka · 2024-07-15T22:51:18Z

internal/config/autosave.go

+			select {
+			case a = <-autotime:
+				if a > 0 {
+					elapsed = time.After(time.Duration(a * float64(time.Second)))


This seems to work correctly from the autosave perspective, however it seems a bit unclean: it doesn't stop the previous elapsed timer, so it will still fire after some time. Although it won't cause an unwanted autosave (thanks to the fact that the elapsed variable is now assigned to a different channel, so we will not wait on the old channel in next iterations of the loop), this also probably means that, since no one will read from the old channel when the old timer fires, the old timer will block forever on sending to the old channel, so it will not be garbage-collected even after it fires.

JoeKar · 2024-07-21T12:17:53Z

Just this one left for v2.0.14, what do you think?

internal/action/command.go

dmaluka · 2024-07-21T11:22:51Z

internal/action/command.go

+			SetGlobalOptionNative(k, parsedSettings[k])
+			continue
+		}
+		if _, ok := oldParsedSettings[k]; ok {


Do we need oldParsedSettings? If the setting is not present in the new parsed settings, we should set it to the default in any case?

If it is to avoid unneeded setting it to the default if it's already default, then it looks inconsistent with the above case when it is present in parsedSettings: in that case we don't check if the new parsed setting is different from the old one or not.

I was sure, that it was needed in a case, but can't remember and reproduce it so far.
So long store short:
I will remove it and directly convert the second path into } else {.

dmaluka · 2024-07-21T12:51:56Z

internal/buffer/settings.go

 	} else if option == "filetype" {
-		config.InitLocalSettings(b.Settings, b.Path)
+		parsedSettings := config.ParsedSettings()
+		parsedSettings[option] = nativeValue


This seems unneeded: we already ensure skipping filetype below?

I confirm, that it looks hacky, but didn't find a better solution so far.
We need to start with clean parsed settings (local within this function) and call InitLocalSettings(), which then operates at this given map. But filetype must be set to parse the local settings (filetype dependent) correctly within this given map, otherwise it would ignore the ft: XYZ settings.
Since we don't want to clear it again to unknown or cause recursive calls it's ignored in the following loop.

Please correct me, in case I oversee something.

But filetype must be set to parse the local settings

Ah, indeed. Looks good then.

I'd maybe just use explicit parsedSettings["filetype"] instead of parsedSettings[option], to make it a bit more clear.

Maybe also rename parsedSettings to just settings?

internal/action/command.go

dmaluka · 2024-07-21T20:11:56Z

internal/buffer/settings.go

 	} else if option == "encoding" {
+		_, err := htmlindex.Get(b.Settings["encoding"].(string))
+		if err != nil {
+			b.Settings["encoding"] = "utf-8"


I believe we should just return error in this case, not override previously set (valid) encoding.

So why not just return err in this case?

internal/action/command.go

dmaluka · 2024-07-22T00:16:42Z

internal/config/settings.go

+			if equal {
+				return nil
+			}
+		}


Instead of all this we can just use a local boolean variable which we set to true if we modify or delete anything in parsedSettings in this function?

...But I actually have doubts whether it's a good idea to prevent unnecessary writes so generically. Think of such a scenario, for example:

The user starts micro -> settings.json is read, option foo is set to true.

The user edits settings.json, changing foo to false (but doesn't run reload).

The user runs set foo true -> micro doesn't update settings.json (so the permanent foo setting remains false, i.e. the set command doesn't behave as it should).

Since micro unfortunately encourages both ways of setting settings (manual editing of settings.json and the set command), we should aim to ensure that both work well, together? So we shouldn't assume that settings.json hasn't changed since the last time micro read it, in the general case? We can only assume that in the special reload case?

Instead of all this we can just use a local boolean variable which we set to true if we modify or delete anything in parsedSettings in this function?

Argh, yes. Your example shows, that it's the wrong approach anyway.

So we shouldn't assume that settings.json hasn't changed since the last time micro read it, in the general case?

Most probably we've then still some use cases left, where settings.json is overwritten without needs, but we should address this then in a different issue/PR.

We can only assume that in the special reload case?

This definitely.

internal/buffer/settings.go

internal/action/command.go

Validate the parsed options directly after read and inform about errors.

Additionally pricise the documentation of the remaining DefaultCommonSettings() and DefaultAllSettings() functions.

- on `reload` - on `filetype` change

It doesn't need to loop over the DefaultCommonSettings() again, since they're already included in the default settings and set via SetGlobalOptionNative().

...and use `SetOptionNative()` instead.

dmaluka · 2024-08-18T19:52:35Z

internal/config/autosave.go

-				break
+			select {
+			case a = <-autotime:
+				if t != nil && !t.Stop() && len(elapsed) > 0 {


Why do we check elapsed only if t.Stop() returned false?

According to the docs...

Stop prevents the Timer from firing. It returns true if the call stops the timer, false if the timer has already expired or been stopped.

...so in case it returns true it shouldn't lead to any elapsed timer yet.
For sanity we could decouple it from the return of Stop() and just clear elapsed ~~in case it isn't empty~~ till it is empty.

...so in case it returns true it shouldn't lead to any elapsed timer yet.

Aha, I see. If t.Stop() returned true, it is guaranteed that the timer has not fired, so the channel is guaranteed to be empty.

BTW I've noticed the doc also says this:

For a chan-based timer created with NewTimer(d), as of Go 1.23, any receive from t.C after Stop has returned is guaranteed to block rather than receive a stale time value from before the Stop; if the program has not received from t.C already and the timer is running, Stop is guaranteed to return true. Before Go 1.23, the only safe way to use Stop was insert an extra <-t.C if Stop returned false to drain a potential stale value.

BTW, I see we are using 1.19, maybe it is a bit too old? (1.23, on the other hand, could be a bit too new, it was only released a week ago.)

~~in case it isn't empty~~ till it is empty.

For the record, it seems guaranteed that elapsed will not contain more than 1 element (i.e. there is no difference between if len(elapsed) > 0 and for len(elapsed) > 0 in this case)?

BTW, I see we are using 1.19, maybe it is a bit too old? (1.23, on the other hand, could be a bit too new, it was only released a week ago.)

Yes, 1.23 or even 1.22 could lead to problems in the moment deprecated functions have been removed from the standard library.

For the record, it seems guaranteed that elapsed will not contain more than 1 element (i.e. there is no difference between if len(elapsed) > 0 and for len(elapsed) > 0 in this case)?

Ok, but since it needs to be checked anyway it doesn't hurt to do it in a loop.

Setting options directly in (h.)Buf.Settings without calling SetOption() or SetOptionNative() is generally not the best idea, since it may not trigger the needed side effects. In particular, after zyedidia#3343, directly setting `diffgutter` and `ruler` causes them not being tracked as locally overridden per buffer, so if we run the `reload` command, it unexpectedly replaces them with the default ones.

JoeKar force-pushed the fix/volatile-after-reinit branch from 479f0d9 to eee2f0c Compare June 17, 2024 15:48

JoeKar changed the title ~~config: Keep volatile settings in case a reinitialization takes place~~ Jun 17, 2024

dmaluka reviewed Jun 17, 2024

View reviewed changes

JoeKar force-pushed the fix/volatile-after-reinit branch from eee2f0c to 5216507 Compare June 18, 2024 20:57

JoeKar changed the title ~~buffer/settings: Don't reinitialize global settings on filetype change~~ Jun 18, 2024

JoeKar changed the title ~~Rework filetype change and reload command~~ Jun 19, 2024

JoeKar force-pushed the fix/volatile-after-reinit branch from e6beb6e to 9d84b25 Compare June 19, 2024 19:27

JoeKar mentioned this pull request Jun 19, 2024

Autosave locally #3342

Closed

dmaluka reviewed Jun 22, 2024

View reviewed changes

internal/action/command.go Show resolved Hide resolved

dmaluka reviewed Jun 22, 2024

View reviewed changes

internal/config/autosave.go Outdated Show resolved Hide resolved

JoeKar force-pushed the fix/volatile-after-reinit branch from 9d84b25 to a60ab16 Compare July 15, 2024 19:43

dmaluka reviewed Jul 15, 2024

View reviewed changes

JoeKar force-pushed the fix/volatile-after-reinit branch from a60ab16 to 5bcc352 Compare July 16, 2024 20:49

JoeKar added this to the v2.0.14 milestone Jul 19, 2024

dmaluka reviewed Jul 21, 2024

View reviewed changes

JoeKar force-pushed the fix/volatile-after-reinit branch from 5bcc352 to 66ace26 Compare July 21, 2024 19:39

dmaluka reviewed Jul 21, 2024

View reviewed changes

JoeKar force-pushed the fix/volatile-after-reinit branch from 66ace26 to faf9c58 Compare July 21, 2024 21:33

dmaluka reviewed Jul 22, 2024

View reviewed changes

JoeKar force-pushed the fix/volatile-after-reinit branch from faf9c58 to 1940ef8 Compare July 22, 2024 20:02

dmaluka reviewed Jul 23, 2024

View reviewed changes

internal/action/command.go Outdated Show resolved Hide resolved

internal/action/command.go Outdated Show resolved Hide resolved

internal/action/command.go Outdated Show resolved Hide resolved

JoeKar added 10 commits August 18, 2024 21:10

config: Refactor parsed option handling

4663927

Validate the parsed options directly after read and inform about errors.

config/settings: Remove duplicating DefaultGlobalSettings()

f661b64

Additionally pricise the documentation of the remaining DefaultCommonSettings() and DefaultAllSettings() functions.

action/command: Prevent overwriting settings set locally or by plugin

0552959

- on `reload` - on `filetype` change

action/command: On reload prevent overwriting local settings

724e294

action/command: Refactor reload & the filetype change

a3071b1

buffer/buffer: Store fileformat in LocalSettings on auto detection

10511c9

action/command: Simplify ResetCmd

0542765

It doesn't need to loop over the DefaultCommonSettings() again, since they're already included in the default settings and set via SetGlobalOptionNative().

plugins/comment: Don't write to b.Settings directly

a678d42

...and use `SetOptionNative()` instead.

config: Remove unused GetAutoTime()

832c7de

config: Use float64 within the autosave processing

4170df8

JoeKar force-pushed the fix/volatile-after-reinit branch 2 times, most recently from 48e1b96 to fff3b18 Compare August 18, 2024 19:26

dmaluka reviewed Aug 18, 2024

View reviewed changes

JoeKar added 3 commits August 18, 2024 22:28

config: Rework autosave to be rearmed upon change

8b31dc7

config: Don't truncate float64 to int

33b7c9d

config: Correct typo in validatePositiveValue()

b80ea93

JoeKar force-pushed the fix/volatile-after-reinit branch from fff3b18 to b80ea93 Compare August 18, 2024 20:29

dmaluka approved these changes Aug 18, 2024

View reviewed changes

JoeKar merged commit 7dc78b7 into zyedidia:master Aug 19, 2024

JoeKar deleted the fix/volatile-after-reinit branch August 19, 2024 17:59

This was referenced Aug 19, 2024

syntax/json: Add comment support to the syntax highlighting #3434

Merged

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

Merged

dmaluka mentioned this pull request Aug 27, 2024

New release needed ? #3444

Closed

JoeKar mentioned this pull request Nov 24, 2024

Filetypes duplicates when you set filetype #3186

Closed

JoeKar mentioned this pull request Jan 23, 2025

Fixing settings not being applied when saving as a new file #3625

Merged

dmaluka mentioned this pull request May 25, 2025

Fix non-working colorscheme plugins #3761

Merged

Rework filetype change, reload command and autosave #3343

Rework filetype change, reload command and autosave #3343

Uh oh!

Conversation

JoeKar commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dmaluka commented Jun 16, 2024

dmaluka commented Jun 16, 2024

JoeKar commented Jun 17, 2024

dmaluka commented Jun 17, 2024

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeKar commented Jun 18, 2024

dmaluka commented Jun 18, 2024

JoeKar commented Jun 19, 2024

Uh oh!

Uh oh!

JoeKar commented Jul 15, 2024

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeKar commented Jul 21, 2024

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

JoeKar Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

2 participants

Rework `filetype` change, `reload` command and `autosave` #3343

Rework `filetype` change, `reload` command and `autosave` #3343

JoeKar commented Jun 13, 2024 •

edited

Loading

JoeKar Aug 18, 2024 •

edited

Loading