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.

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

Autosave locally #3342

Open
Bellavene opened this issue Jun 12, 2024 · 22 comments · May be fixed by #3343
Open

Autosave locally #3342

Bellavene opened this issue Jun 12, 2024 · 22 comments · May be fixed by #3343

Comments

@Bellavene
Copy link

Bellavene commented Jun 12, 2024

The tutorial and help options state this:

In the settings.json file you can also put set options locally by specifying
either a glob or a filetype. Here is an example which has `tabstospaces` on for
all files except Go files, and `tabsize` 4 for all files except Ruby files:

json
{
    "ft:go": {
        "tabstospaces": false
    },
    "ft:ruby": {
        "tabsize": 2
    },
    "tabstospaces": true,
    "tabsize": 4
}

Or similarly you can match with globs:

json
{
    "*.go": {
        "tabstospaces": false
    },
    "*.rb": {
        "tabsize": 2
    },
    "tabstospaces": true,
    "tabsize": 4
}

But it doesn't work with autosave option, starting micro with micro -autosave 1 doesn't work either... I see that this isn't working at least 4-5 years, why?

@Bellavene Bellavene changed the title Autosave Autosave locally Jun 12, 2024
@Andriamanitra
Copy link
Contributor

Enabling autosave works fine for me both from command line and settings.json. Can you provide more information (micro -version, operating system, how did you figure out it's "not working")?

@Bellavene
Copy link
Author

2.0.14-dev.201
macos 10.14.4

No need to quote my statement. I am not talking about global autosave, which works fine if toggled with settings.json, but still doesn't work from prompt. If you look closely, I am talking about local setting.

@Andriamanitra
Copy link
Contributor

I see the issue now: autosave currently only works as a global option, and we only have one global Autosave timer that triggers save on all open buffers regardless of their potentially different local autosave setting.

micro/cmd/micro/micro.go

Lines 421 to 424 in 650c0a8

case <-config.Autosave:
for _, b := range buffer.OpenBuffers {
b.AutoSave()
}

I can think of a few ways of fixing this:

  1. Give each buffer its own autosave timer (probably a bad idea?)
  2. Change the global timer to always trigger every second, but add checks for buffer-local autosave interval settings
  3. Completely rework the autosave system – the whole timer thing seems a bit odd to me as it triggers on a regular interval. Is there any reason to want that behavior instead of simply saving after the buffer is modified?

starting micro with micro -autosave 1 doesn't work either

This part I still can't reproduce though. How did you figure out it's not working?

@Bellavene
Copy link
Author

Bellavene commented Jun 13, 2024

I believe option 3 is the right way. And yes, I don't see the logic in autosave on every change that was made, autosave on exit sounds more logical, when we already have backups.
But why not bring back local execution? And why the tutorial still says about it, when it doesn't work?

This part I still can't reproduce though. How did you figure out it's not working?

Strange question, It doesn't auto save : ) I would be happy with the variable option only available, I need autosave only for CLI messaging apps anyway.

Screen.Recording.2024-06-13.at.08.59.16.mov

@JoeKar
Copy link
Collaborator

JoeKar commented Jun 13, 2024

That's very strange. I can see, that it isn't working for you, but currently I can't reproduce it in that way under Linux.
Do you have autosave set to true in your settings.json?
Because there is this downward compatible check:

// check if autosave is a boolean and convert it to float if so
if v, ok := parsedSettings["autosave"]; ok {
s, ok := v.(bool)
if ok {
if s {
parsedSettings["autosave"] = 8.0
} else {
parsedSettings["autosave"] = 0.0
}
}
}

Anyway I tested it with it being set to true and even then it wasn't waiting 8s.

@Bellavene
Copy link
Author

Bellavene commented Jun 13, 2024

That's very strange. I can see, that it isn't working for you, but currently I can't reproduce it in that way under Linux.
Do you have autosave set to true in your settings.json?

Nope, as I said before, I don't need global autosave. Yes, it is strange.

I found which setting in init.lua was causing dysfunctional autosave variable. Still can't imagine why this function is overriding the variable at start.

function onBufferOpen(buf)
    if buf.Settings["filetype"] == "unknown" then
        buf:SetOption("filetype", "zsh")
    end
end

@JoeKar
Copy link
Collaborator

JoeKar commented Jun 13, 2024

Aaah, ok...now it becomes clear:

} else if option == "filetype" {
config.InitRuntimeFiles(true)
err := config.ReadSettings()
if err != nil {
screen.TermMessage(err)
}
err = config.InitGlobalSettings()
if err != nil {
screen.TermMessage(err)
}
config.InitLocalSettings(b.Settings, b.Path)
b.UpdateRules()

The call to config.InitGlobalSettings() will actually revert the volatile settings.

@JoeKar JoeKar linked a pull request Jun 13, 2024 that will close this issue
@JoeKar
Copy link
Collaborator

JoeKar commented Jun 13, 2024

Feel free to apply the patch from the linked PR and test if it now fits to your expectation.

@Bellavene
Copy link
Author

Thanks

@Bellavene
Copy link
Author

Bellavene commented Jun 13, 2024

But still, autosave on exit would be better, I believe.

@JoeKar
Copy link
Collaborator

JoeKar commented Jun 14, 2024

It does exactly that in the moment autosave isn't overwritten by 0:

if config.GlobalSettings["autosave"].(float64) > 0 {
// autosave on means we automatically save when quitting
h.SaveCB("Quit", func() {
h.ForceQuit()
})

@Bellavene
Copy link
Author

Feel free to apply the patch from the linked PR and test if it now fits to your expectation.

Tested, nothing has changed. Variable still doesn't work if you change the file type for buffer in init.lua

@Bellavene Bellavene reopened this Jun 14, 2024
@JoeKar
Copy link
Collaborator

JoeKar commented Jun 15, 2024

  1. You don't need to close the issue as long as the PR is still open. It will be closed automatically in the moment the PR is merged.
  2. Did you really used the patch from the PR? I change the filetype while testing and it worked.

@dmaluka
Copy link
Collaborator

dmaluka commented Jun 16, 2024

@Andriamanitra :

I see the issue now: autosave currently only works as a global option, and we only have one global Autosave timer that triggers save on all open buffers regardless of their potentially different local autosave setting.

[...]

I can think of a few ways of fixing this:

  1. Give each buffer its own autosave timer (probably a bad idea?)

Why not?

  1. Change the global timer to always trigger every second, but add checks for buffer-local autosave interval settings

That means that 1 second would be the minimum granularity, which is a limitation of functionality.

  1. Completely rework the autosave system – the whole timer thing seems a bit odd to me as it triggers on a regular interval. Is there any reason to want that behavior instead of simply saving after the buffer is modified?

Because saving the file after inserting or removing every single character is a big overkill.

@dmaluka
Copy link
Collaborator

dmaluka commented Jun 16, 2024

@Bellavene :

And yes, I don't see the logic in autosave on every change that was made, autosave on exit sounds more logical, when we already have backups.

If all you need is autosave on exit, you can use something like this in init.lua, instead of using the autosave option:

function preQuit(bp)
    bp.Buf:Save()
end

...And you can make it customizable per buffer, e.g. with:

function preQuit(bp)
    if bp.Buf.Settings["initlua.saveonexit"] then
        bp.Buf:Save()
    end
end

function init()
    config.RegisterCommonOption("initlua", "saveonexit", false)
end

You can then use e.g.:

    "ft:go": {
        "initlua.saveonexit": true
    },

Note: if you also want to be able to change this initlua.saveonexit per buffer at runtime via the setlocal command, the above lua code won't allow that, but it will allow that if config.RegisterCommonOption() is not in init() but in preinit(). (I'd say it's a bug, config.RegisterCommonOption() should work seamlessly in both preinit() and init().)

@dmaluka
Copy link
Collaborator

dmaluka commented Jun 16, 2024

All that said, we may consider adding support for a special value for the autosave option, e.g. -1, for autosaving at exit only, without periodic autosaving on timer.

@Bellavene
Copy link
Author

Bellavene commented Jun 16, 2024

@Bellavene :

And yes, I don't see the logic in autosave on every change that was made, autosave on exit sounds more logical, when we already have backups.

If all you need is autosave on exit, you can use something like this in init.lua, instead of using the autosave option:

function preQuit(bp)
    bp.Buf:Save()
end

...And you can make it customizable per buffer, e.g. with:

function preQuit(bp)
    if bp.Buf.Settings["initlua.saveonexit"] then
        bp.Buf:Save()
    end
end

function init()
    config.RegisterCommonOption("initlua", "saveonexit", false)
end

You can then use e.g.:

    "ft:go": {
        "initlua.saveonexit": true
    },

Note: if you also want to be able to change this initlua.saveonexit per buffer at runtime via the setlocal command, the above lua code won't allow that, but it will allow that if config.RegisterCommonOption() is not in init() but in preinit(). (I'd say it's a bug, config.RegisterCommonOption() should work seamlessly in both preinit() and init().)

Thank you. I knew there must be a simple function for that.

But how it can be changed, so it autosaves not by file type, but only at certain file/files? Tried this one with no luck:

    "tmp*.txt": {
        "initlua.saveonexit": true,
    }

Update: Nevermind, instead of partial name, I have set it to the full path with name and it works! Thank you again.

@Andriamanitra
Copy link
Contributor

@Andriamanitra :

I see the issue now: autosave currently only works as a global option, and we only have one global Autosave timer that triggers save on all open buffers regardless of their potentially different local autosave setting.
[...]
I can think of a few ways of fixing this:

  1. Give each buffer its own autosave timer (probably a bad idea?)

Why not?

Constantly having a bunch of timers running & saving potentially dozens of files most of which aren't modified seems pretty wasteful. The timer behavior is also just weird: if you have a 10 second timer the save might happen either instantly after a change or 10 seconds after a change.

  1. Change the global timer to always trigger every second, but add checks for buffer-local autosave interval settings

That means that 1 second would be the minimum granularity, which is a limitation of functionality.

I struggle to think of a use case for more granularity. The current minimum granularity is 1 second and I haven't seen any complaints about it:

time.Sleep(time.Duration(a) * time.Second)

  1. Completely rework the autosave system – the whole timer thing seems a bit odd to me as it triggers on a regular interval. Is there any reason to want that behavior instead of simply saving after the buffer is modified?

Because saving the file after inserting or removing every single character is a big overkill.

It would still be debounced with a short timer. This is generally how other editors do it, and how I (and probably most users?) would expect autosave to work.

@dmaluka
Copy link
Collaborator

dmaluka commented Jun 16, 2024

Constantly having a bunch of timers running & saving potentially dozens of files most of which aren't modified seems pretty wasteful.

If micro is saving a file every time without even checking if it was modified, that is already a huge waste.

And that is what is actually happening (as I've just found), lol.

The current minimum granularity is 1 second

Indeed. I thought it was a floating-point number.

It would still be debounced with a short timer. This is generally how other editors do it, and how I (and probably most users?) would expect autosave to work.

Ok, I have no strong opinion what is better, a debounce timer or a periodic timer. I don't use autosave myself, so I don't know what would a user expect. (I suppose most micro users don't users don't use it either, which is why no one has complained/noticed that it is needlessly saving an unmodified file in the background all the time, for example.)

Still, if I wanted to use it, I imagine I wouldn't care how many seconds after a change would the save happen, - for the same reason that I wouldn't like it to happen after every change. I would just want it to happen from time to time (and probably would prefer to be able to configure how often it happens). So it isn't obvious to me what are the merits of using a debounce timer instead.

@dmaluka
Copy link
Collaborator

dmaluka commented Jun 16, 2024

And that is what is actually happening (as I've just found), lol.

Quick fix: #3356

@JoeKar
Copy link
Collaborator

JoeKar commented Jun 17, 2024

The current minimum granularity is 1 second

Indeed. I thought it was a floating-point number.

We converted the user input from string to float64 and then via SetAutoTime to int (why ever), instead of doing the float conversion at one line time.Duration(a * float64(time.Second)), with which we could have a granularity of 1ns (which I've doubt we need).
So autotime should be a float64 and we can drop the conversion to int.

Adding a timer per buffer sounds a bit too much to me. From my perspective the "ticker" idea sounds like a good compromise to have it somehow specific per buffer, but still with one general timer of what ever granularity. The save shall then take place every time the tick count has been consumed, while it's reset after every change and save.
But it's just my opinion and I don't use autosave too.

@JoeKar
Copy link
Collaborator

JoeKar commented Jun 19, 2024

@Bellavene
Can you test #3343 once again?
Please describe the tested scenario in case it fails or doesn't match your expectation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants