thanks for your comments, will look at it

So what I would suggest is to have a checkbox with something like "Scribble mode" inside the "Instant save" tab enabling this feature

"Does it save the Geany Scribble window then?" [user who didn't read the docs question]

Whilst I didn't get the "persistent temp files" either and I still don't know what the use-case is beyond persisting unnamed buffers and I don't understand what the reason for saving unnamed buffers is, if its unnamed how do I know whats in it to switch to it when they are all named "Untitled"? So I can't suggest anything better than the descriptive "save unnamed buffers".

Whilst I didn't get the "persistent temp files" either and I still don't know what the use-case is beyond persisting unnamed buffers and I don't understand what the reason for saving unnamed buffers is, if its unnamed how do I know whats in it to switch to it when they are all named "Untitled"?

They will get some file name (see the screencast, I personally would suggest names scribble_1, scribble_2, etc.)

So I can't suggest anything better than the descriptive "save unnamed buffers".

That's not the whole thing the patch does though - it really is some "scribble mode" with additional features:

1. On opening an unnamed buffer, it saves the file (same as instant save)
2. It auto-saves this buffer in user-specified intervals so the user doesn't have to care about saving it
3. When you close the buffer, you are asked if you really want to close it - when you do, the underlying file is deleted so, again, you don't have to care about files here. When you select that you don't want to close it, the open buffer is preserved - with a slight limitation. The limitation is that plugins can't interrupt file closing so what the plugin does it that it reopens the underlying file after it's closed (and of course it doesn't delete it in this case) - since this only happens when the user makes a mistake and tries to close the file by accident, I think it's fine to have it implemented this way.
4. When you use "Save as", the underlying "scribble" file is deleted automatically and only the file under which you saved it is preserved.

Now to why people (#905), including me, want to have this feature - the Scribble panel is extremely limited and the basic editor features don't work there (see #3558). Having a full-blown Scintilla editor used for Scribble with all editing options and syntax highlighting in a big window would be really nice. And this PR is quite a nice way how to implement it in a pretty much invisible and care-free way for users.

I know the scribble window is very limited, thats why I'm confused by you wanting to call this "scribble".

PS, I said I do not understand the use-case for this, I didn't say not to do it, just because I don't understand why you would want it doesn't mean not to do it. (if that was the case Geany wouldn't have a C filetype 😜 )

I know the scribble window is very limited, thats why I'm confused by you wanting to call this "scribble".

Well, it doesn't really have to be called this way, I'd just call it based on what it's supposed to do instead of how it's implemented (i.e. "persistent temp files"). Could be for instance "persistent notepad" and the individual tabs would be notepad_1, notepad_2, etc.

Ok, at least "notepad" is a term thats not used so far AFAICT so it won't get confused with something else.

One more thing. The plugin should pre-fill default_persistent_temp_files_dir_utf8 into the directory entry in the plugin configuration. When you delete the saveactions configuration file, this entry is empty by default.

Also, when this empty directory gets confirmed, the plugin shows a dialog with error, but this invalid value is stored into the config file anyway (I know this is how the Instantsave feature has it implemented now and you just copied it from there, but it shouldn't be possible to confirm the save action preferences with the invalid value and the preferences dialog shouldn't be closed in this case).

Finally, when you confirm the empty directory entry, the plugin crashes in

#1  0x0000ffffec113d54 in is_temp_saved_file
    (file_path_utf8=0xaaaaab716d80 "/home/parallels/projects/geany/doc/plugins.dox") at saveactions.c:336
#2  0x0000ffffec113eb4 in persistent_temp_files_update (data=<optimized out>)
    at saveactions.c:733

This should be fixed too.

One more thing. The plugin should pre-fill default_persistent_temp_files_dir_utf8 into the directory entry in the plugin configuration. When you delete the saveactions configuration file, this entry is empty by default.

Also, when this empty directory gets confirmed, the plugin shows a dialog with error, but this invalid value is stored into the config file anyway (I know this is how the Instantsave feature has it implemented now and you just copied it from there, but it shouldn't be possible to confirm the save action preferences with the invalid value and the preferences dialog shouldn't be closed in this case).

Finally, when you confirm the empty directory entry, the plugin crashes in

#1  0x0000ffffec113d54 in is_temp_saved_file
    (file_path_utf8=0xaaaaab716d80 "/home/parallels/projects/geany/doc/plugins.dox") at saveactions.c:336
#2  0x0000ffffec113eb4 in persistent_temp_files_update (data=<optimized out>)
    at saveactions.c:733

This should be fixed too.

not sure how any of these is possible - target_dir value in config file may stay empty while in-memory variable is filled with default value anyway.
And saving empty path into config should not be possible since we have multiple checks which allow only non-empty absolute path to valid existing directory to be saved to config file.
I wasnt able to reproduce any of these issues but anyway i added pre-filling of target_dir into a config file on plugin init, hope this helps.

ah found it, the issue appears not when you delete config file but when you dont have directory itself existing on disc, will try to do something with it

so i reworked settings behavior for this feature, now everything should be fine:

• when geany starts without existing temp files dir setting or settings file - it populates default dir path setting and creates folder on disk
• when geany starts with existing dir setting that is invalid (e.g. folder was deleted manually) - feature gets auto-disabled and status message sent.
• when user tries to save feature settings - dialog wont close until valid dir is set (or 'cancel' clicked).
• enabling feature with invalid dir path in a textbox is also not possible anymore.

also, i fulfilled old TODO of instantsave code i reused and added one of utils function to plugins API (checking if file is writable)

Overall, now everything seems working fine to me.
As it is expected - doing any changes in C may be hilariously tedious, and current code will anyway have to be re-wired to new UI (if we want it to change), so i guess we need to confirm an approach and then (if this happens) - try to finish everything.
e.g. this looks good to me #3911 (review)

added re-opening for all temp files as described here - #3911 (review)
Implementation appeared to be surprisingly tricky and there are some slightly rough edges, but at the end looks good to me.

@LiquidCake Thanks for the updates! I definitely like that the temp files are preserved across project opens/closes and it seems to work very well for me.

Some more observations when playing with the feature:

• when closing a temp file and choosing "Save", I'd suggest using some other default directory than the one where temp files are stored so users aren't tempted to store the files there (and be surprised by the consequences if they choose a gtemp_x file name)
• since all temp files are opened on session load, there could be one more option in the dialog asking what to do when closing a temp file - "Close". This would just close the temp file but keep it saved on the disk. I can imagine it could be useful when someone is not interested to see the temp file for some project but still wants to keep it for other projects. I'd also maybe rename "Don't save" to "Delete" so it's clear the contents of the tab is lost.
• it would be worth considering whether create_new_temp_file_name() shouldn't rather iterate over integers and test whether gtemp_i exists - this way, when e.g. gtemp_5 is open and all other temp files closed, gtemp_1-4 file names could be reused again for new tabs
• I'm not completely sure if utils_is_file_writable() should be part of Geany's API. Personally I'd rather sacrifice this check and not bloat Geany's API with another random function.

Added workaround to prevent temp files hijacking active tab on session change.

And about these:

@LiquidCake Thanks for the updates! I definitely like that the temp files are preserved across project opens/closes and it seems to work very well for me.

Some more observations when playing with the feature:

* when closing a temp file and choosing "Save", I'd suggest using some other default directory than the one where temp files are stored so users aren't tempted to store the files there (and be surprised by the consequences if they choose a `gtemp_x` file name)

I would also like this but we wont be able to do it on plugin level, directory is defined and set to save-as window in core code and also very early.
Would work if we could check some specific property of document at this point, see that it is temporary and prevent using existing file path as default value for save-as window. But i guess we wont be able to do it.
BTW - if user saves file with gtemp_ template name, or just 'saves as' temp file under same 'temp' name and path - no issues should happen, this file will just be treated by Geany as temp file again.

* since all temp files are opened on session load, there could be one more option in the dialog asking what to do when closing a temp file - "Close". This would just close the temp file but keep it saved on the disk. I can imagine it could be useful when someone is not interested to see the temp file for some project but still wants to keep it for other projects. I'd also maybe rename "Don't save" to "Delete" so it's clear the contents of the tab is lost.

To me such UX seems to allow user too many options to chose from, and also whole idea of allowing user to "keep" temp file on disk while not having it open in editor seems contradicting the essence of feature - user should see 'temp' tab as if it was an 'untitled' tab that just doesnt disappear after we close program. If user wants to save file and close tab - they should explicitly do it with save-as.
Also, if user could just close temp tab while keeping file - it will anyway re-appear in editor after any session change.
This is also why i just copied tab-closing dialog for temp tabs from regular unsaved tab closing dialog, without any changes - it should look and feel the same (ideally).
Unfortunately, there is one case we allow user to close temp tabs without deleting/saving them - it is when 'close all' option is clicked. But this is a necessary evil, not a feature - we cannot differentiate 'close all' from changing session to/from project.
So essentially - what you are proposing can be done, but i personally would avoid it.

* it would be worth considering whether `create_new_temp_file_name()` shouldn't rather iterate over integers and test whether `gtemp_i` exists - this way, when e.g. `gtemp_5` is open and all other temp files closed, `gtemp_1-4` file names could be reused again for new tabs

done

* I'm not completely sure if `utils_is_file_writable()` should be part of Geany's API. Personally I'd rather sacrifice this check and not bloat Geany's API with another random function.

removed

Added workaround to prevent temp files hijacking active tab on session change.

What is this and where is the commit?

when closing a temp file and choosing "Save", I'd suggest using some other default directory than the one where temp files are stored so users aren't tempted to store the files there (and be surprised by the consequences if they choose a gtemp_x file name)

I would also like this but we wont be able to do it on plugin level, directory is defined and set to save-as window in core code and also very early.

You could just avoid using dialogs_show_save_as() and implement it manually similarly to e.g. target_directory_button_clicked_cb() but for saving files in this case (note the changes related to #3861 - if you copy the code of target_directory_button_clicked_cb() and modify it, you can reuse file_chooser_run() and file_chooser_destroy()).

• since all temp files are opened on session load, there could be one more option in the dialog asking what to do when closing a temp file - "Close". This would just close the temp file but keep it saved on the disk. I can imagine it could be useful when someone is not interested to see the temp file for some project but still wants to keep it for other projects. I'd also maybe rename "Don't save" to "Delete" so it's clear the contents of the tab is lost.

To me such UX seems to allow user too many options ...

Fair enough, makes sense.

• it would be worth considering whether create_new_temp_file_name() shouldn't rather iterate over integers and test whether gtemp_i exists - this way, when e.g. gtemp_5 is open and all other temp files closed, gtemp_1-4 file names could be reused again for new tabs

Instead of using GDir, storing the result into a hash table, and then iterating over integers, wouldn't it be easier to do something like

gint i;

for (i = 1; i < 1000; i++)
{
    gchar *name = g_strdup_printf("%s%c%s%d", persistent_temp_files_target_dir, G_DIR_SEPARATOR, PERSISTENT_TEMP_FILE_NAME_PREFIX, i);
    gboolean file_exists = g_file_test(dirpath_locale, G_FILE_TEST_EXISTS);
    g_free(name;)
    if (!file_exists)
        break;
}

return g_strdup_printf("%s%d", PERSISTENT_TEMP_FILE_NAME_PREFIX, i);

Untested, possibly missing some details.

What is this and where is the commit?

1fd9ab7

At any session change, including initial startup - we first load session and then call load_all_temp_files_idle() which (re)loads all temp files and changes active tab to last loaded temp file. I added a workaround to re-activate originally active tab by re-opening it (i guess no other way to do it)

You could just avoid using dialogs_show_save_as() and implement it manually similarly to e.g. target_directory_button_clicked_cb() but for saving files in this case (note the changes related to #3861 - if you copy the code of target_directory_button_clicked_cb() and modify it, you can reuse file_chooser_run() and file_chooser_destroy()).

i could try, but this would anyway be used only when we try to close temp file tab and chose save-as. But when user just tries to save-as temp file using menu - it will still be opening on temp folder

Instead of using GDir, storing the result into a hash table, and then iterating over integers, wouldn't it be easier to do something like

lol you're right, changed logic to this one

At any session change, including initial startup - we first load session and then call load_all_temp_files_idle() which (re)loads all temp files and changes active tab to last loaded temp file. I added a workaround to re-activate originally active tab by re-opening it (i guess no other way to do it)

Ah, right, I overlooked it - I thought it only removed the API call.

i could try, but this would anyway be used only when we try to close temp file tab and chose save-as. But when user just tries to save-as temp file using menu - it will still be opening on temp folder

OK, I didn't think of this possibility - I guess it doesn't make sense to spend time on this then.

lol you're right, changed logic to this one

Better ;-)

so i think this feature is complete, if we are considering merging it - we should settle on config UI (either keep as is or change to point #1 from #3911 (review))
And probably rename everything to "persistent notepad" or something like that.

otherwise it is usable with manual geany build, helps me a lot

One more problem - when you launch Geany from the command-line by

geany some_file.txt

where some_file.txt doesn't exist yet, the file gets opened in a new tab but the "persistent temp file" becomes the active tab instead. I haven't checked the code what Geany does exactly but the file apparently gets opened at some later point.

In any case, I believe this could be mitigated if inside load_all_temp_files_into_editor() you only call document_open_file() when the temp file isn't opened in any tab (by going through open documents and checking their path). This would also eliminate some other problems like when "Switch to last used document after closing a tab" is selected in "Notebook tabs" inside preferences which leads to switching to "persistent temp files" when closing some other tabs.

One more request - could you rebase this PR on top of current Geany master (or merge master into this branch)? I'd like to give this feature some more testing but in parallel test the recently merged #3849 together with the LSP plugin.

One more problem - when you launch Geany from the command-line by

geany some_file.txt

where some_file.txt doesn't exist yet, the file gets opened in a new tab but the "persistent temp file" becomes the active tab instead. I haven't checked the code what Geany does exactly but the file apparently gets opened at some later point.

In any case, I believe this could be mitigated if inside load_all_temp_files_into_editor() you only call document_open_file() when the temp file isn't opened in any tab (by going through open documents and checking their path). This would also eliminate some other problems like when "Switch to last used document after closing a tab" is selected in "Notebook tabs" inside preferences which leads to switching to "persistent temp files" when closing some other tabs.

One more request - could you rebase this PR on top of current Geany master (or merge master into this branch)? I'd like to give this feature some more testing but in parallel test the recently merged #3849 together with the LSP plugin.

merged master, fixed unconditional reopening of files. This is indeed beneficial, though the core issue with keeping right tab active after opening non-existent file from console is related to fact that we cannot manually activate tab that does not yet have underlying file saved to disk (we can activate it only by re-opening file from disc right now).
So now everything works correctly, unless you put persistent temp file on disc manually and then open non-existent file from console - then focus will still be on a last-opened persistent file. Doesnt matter though

By the way I did run into a strange issue during Geany quitting when this feature is used in combination with the LSP plugin. Not a problem of this PR, I'll try to address it with #4069.

@techee i've reset branch and re-applied changes as 2 commits, i guess this is what we want

I've just diffed it against the previous version and apart from a fixed whitespace, there's no other difference in saveactions.c 👍

I'll wait until the next weekend and unless anybody has some more comments, I would merge it afterwards 🎉

If you feel bored, it would be good to address #4033 - possibly as a separate PR so possible review comments don't block this PR. I think you don't have to go into too much detail and just describe the general functionality of this feature in

Created PR for doc update #4077

I created 3 PRs with fixes to existing bugs i found during development.
They are in my local fork, will re-target or smth if we want to accept those PRs.

Autosave cannot be disabled
LiquidCake#4

Configured file type is not set to GeanyDocument
LiquidCake#3

File templates are not supported
LiquidCake#2

Last 2 are related to Instantsave and thus transferred to Pers. docs that were built looking on what Instantsave actually does atm

Autosave cannot be disabled

I think this patch should be added to this PR - it's pretty straightforward and fixes a problem introduced here.

Configured file type is not set to GeanyDocument

Does not affect this PR (it works for untitled documents) and was probably present before - I think you can open it once this PR is merged.

File templates are not supported

Alright, maybe I miss something but I think you can determine a non-file-backed documents just by checking if doc->real_path is NULL or not. But if you do, it would be best to "convert" such a document to "persistent" only when it gets modified, otherwise the result may be annoying (e.g., in the LSP plugin I show LSP server initialization handshake result in a new document. Unless modified, such documents can be closed in Geany without any warnings and you don't want to save such temporary documents).

In addition, I don't think you could change the "document-new" signal without affecting existing plugins. Also, plugins would have to be modified to use document_new_file_with_creation_type() consistently and it's kind of error-prone.

In any case, it can wait until this PR is merged.

Autosave cannot be disabled

I think this patch should be added to this PR - it's pretty straightforward and fixes a problem introduced here.

all 3 problems are in master branch, you can tell from how config window looks on videos, and PR for Autosave is directed to master (of my fork).
But we can add Autosave fix to this PR no problem

Configured file type is not set to GeanyDocument

Does not affect this PR (it works for untitled documents) and was probably present before - I think you can open it once this PR is merged.

right, it does not affect PUD unless templates start working, then it would in a minor way. But we could just merge changes from that PR into current one or create new one afterwards or just forget about it - as you prefer

File templates are not supported

Alright, maybe I miss something but I think you can determine a non-file-backed documents just by checking if doc->real_path is NULL or not. But if you do, it would be best to "convert" such a document to "persistent" only when it gets modified, otherwise the result may be annoying (e.g., in the LSP plugin I show LSP server initialization handshake result in a new document. Unless modified, such documents can be closed in Geany without any warnings and you don't want to save such temporary documents).

In addition, I don't think you could change the "document-new" signal without affecting existing plugins. Also, plugins would have to be modified to use document_new_file_with_creation_type() consistently and it's kind of error-prone.

In any case, it can wait until this PR is merged.

Here is how i see it:
Initially, Instantsave was working with files, created using template - for half a year, or never worked at all, who knows.
But this exists in code and in documentation:

Then, we have this commit - now filename is pre-generated for template-created document and is set to document struct.
(templates.c, on_new_with_file_template())

So from this point on, Instantsave is not working with such new files.
But - the original intent was for it to work with it as with normal files - instantly create backing file at configured folder, not to do it "after tab recieves changes". Despite this might be annoying.

And this is how it works now in my PR, once we let Instantsave pass beyond if (enable_instantsave && doc->file_name == NULL) again.
This could be fixed directly in master just for Instantsave, but my PR suggest changes that make PUD also work the same way.

If we dont fix this - we should at least remove mentions of supporting templates from geany.txt, in my PR #4077 i kept mentions and furthermore moved it to common part that describes both IS and PUD since their code in that place is common.
Or ofc we could skip all these, again as you prefer.

In addition, I don't think you could change the "document-new" signal without affecting existing plugins.

This is not cool, yes, alternatives would be worse - add field to GeanyDocument or just change template-spawned file name to something definitive like "untitled-tpl" and check for this kind of name here if (enable_instantsave && doc->file_name == NULL)
(as a hack).

Also, plugins would have to be modified to use document_new_file_with_creation_type() consistently and it's kind of error-prone.

Noone is oblidged to use it as old document_new_file() func still exists. If this doesnt brake ABI things should work for any plugin without re-compiling even. But here im not sure

i wont insist on doing anything as i dont personally need any of these to work and dont have that much spare time to keep working on it, but if we want/can complete this work and cut all tails - would be nice.

all 3 problems are in master branch, you can tell from how config window looks on videos, and PR for Autosave is directed to master (of my fork).
But we can add Autosave fix to this PR no problem

I don't care much, you can post it afterwards too.

right, it does not affect PUD unless templates start working, then it would in a minor way. But we could just merge changes from that PR into current one or create new one afterwards or just forget about it - as you prefer

I'm slightly lost in what you are proposing here - as I said, I think it can wait until this PR is merged.

So from this point on, Instantsave is not working with such new files.

I'm not questioning the fact that it doesn't work - I just don't think your implementation is the right one. You can't change the "document-new" signal - you break API towards plugins this way. Also, as I said, plugins would have to be modified to use document_new_file_with_creation_type() consistently and plugin authors would have to remember to do it correctly and I think this is the potential source of problems.

As I said, instead, the plugin itself should try to do it right by itself without any extra API. doc->real_path tells you whether there's a file backing the tab or not plus I think it should detect whether the file is modified or not before converting it to a persistent untitled document.

If we dont fix this - we should at least remove mentions of supporting templates from geany.txt, in my PR #4077 i kept mentions and furthermore moved it to common part that describes both IS and PUD since their code in that place is common.

This is an option too, depends which way you decide to go.

This is not cool, yes, alternatives would be worse - add field to GeanyDocument or just change template-spawned file name to something definitive like "untitled-tpl" and check for this kind of name here if (enable_instantsave && doc->file_name == NULL)

Again, I'd point you to doc->real_path (this isn't doc->file_name) - it tells you whether there's a file to which a tab is saved and based on this you can detect all "untitled" , template files, or any similar files created by plugins. No extra API needed IMO.

I don't care much, you can post it afterwards too.

Then i'll just create another PR after this one is merged

I'm slightly lost in what you are proposing here - as I said, I think it can wait until this PR is merged.

I proposed to add those changes right to this PR, but doesnt matter, i'll create another PR after this one is merged

I'm not questioning the fact that it doesn't work

ok got it about real_path, not sure about document_new_file_with_creation_type but anyway i guess we are not doing these changes as i wont be able to introduce new logic like detecting modifications etc.
So this means templates issues is out of question, closing that PR.

Which leads us to the last open question - since templates are not working - what to do with geany.txt from another PR - should i remove mentions of templates from Untitled Document Save section, or move it back to Instant Save or just keep as is?

ok got it about real_path, not sure about document_new_file_with_creation_type but anyway i guess we are not doing these changes as i wont be able to introduce new logic like detecting modifications etc.
So this means templates issues is out of question, closing that PR.

To me it doesn't sound too difficult so I'll try to do it myself once this PR is merged. But of course on the way I can realize it's not so easy as I imagine and may decide it's not worth it.

Which leads us to the last open question - since templates are not working - what to do with geany.txt from another PR - should i remove mentions of templates from Untitled Document Save section, or move it back to Instant Save or just keep as is?

Since this is a separate PR, I'd say let's wait if I manage to implement it in which case we can keep it; otherwise we can drop it.

I don't care much, you can post it afterwards too.

Then i'll just create another PR after this one is merged

I'm slightly lost in what you are proposing here - as I said, I think it can wait until this PR is merged.

I proposed to add those changes right to this PR, but doesnt matter, i'll create another PR after this one is merged

In the light of the above, I think it would actually be best if you added both of the fixes to this PR so I can start the work on the finished result. I think you can keep these as separate commits (no rebasing needed).

done

To me it doesn't sound too difficult so I'll try to do it myself once this PR is merged. But of course on the way I can realize it's not so easy as I imagine and may decide it's not worth it.

@LiquidCake I just tried this in

techee@534a655

Maybe I'm overlooking something but IMO it's as simple as that. Could you have a look at it and give it a try?

looks good to me
additionally there is another scenario of creation of non-empty document which is Document->Clone
it is not considered by this code but seems to work ok anyway

looks good to me

Good to hear. Would it be OK if I push the change to this PR so we don't have to open another one?

additionally there is another scenario of creation of non-empty document which is Document->Clone
it is not considered by this code but seems to work ok anyway

And other from the plugins - e.g. the VC plugin shows git history in a non-saved tab. But you can start editing this document and then you probably want it to be persistent with this feature (on the other hand, when you don't edit it, you just want to close it without any warning which is why I added the modification check).

Would it be OK if I push the change to this PR

sure

And other from the plugins

i see, then original (broken) logic from instantsave was not really enough but we didnt know because it was blocked

@LiquidCake The day has come, merging. 🎉

Thanks for all the work and your patience, really appreciated!

nice, thanks for you support