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

Add TextFile support across the editor #53025

Merged
merged 1 commit into from
Sep 30, 2021

Conversation

Paulb23
Copy link
Member

@Paulb23 Paulb23 commented Sep 24, 2021

Starting to look at improving TextFile support in the editor.

This PR adds a new editor setting docks/filesystem/textfile_extensions that accepts a csv list of extensions e.g txt,cfg,json.

Any file with those extensions will be visible in the FileSystem dock with the TextFile icon, all normal operations work i.e open, delete, rename etc. Right clicking the FileSystem dock will allow you to create a TextFile. Similarly, the inspector can now handle TextFiles

The main file dialogs will have the extensions listed in docks/filesystem/textfile_extensions as filters. On top of this, TextFiles will now appear the in the general QuickOpen menu.

The system favours the ResourceLoader, so if tscn is in the extension list, it will have no affect. In addition, TextFiles will not appear in the exporting process or be exported, so will have to use the export filters as usual.

Finally, they are not classed as import_extensions, so will not litter the project with .import files.

Related godotengine/godot-proposals/issues/677

closes #21787
closes #24187


Unsure where the setting belongs, did think about moving to ProjectSettings as each project may use different file types?

@Calinou
Copy link
Member

Calinou commented Sep 24, 2021

Should we add a default list of TextFile extensions? This would improve the FileSystem dock usability out of the box.

I'd suggest something like txt,md,cfg,ini,log,json,yml,yaml,toml.

@akien-mga
Copy link
Member

I'm not so fond of having to special case TextFile everywhere so that it's not handled like other resources. Couldn't we find a way to allow editing text files without the need for a proxy TextFile resource which we actually don't want to be treated as a resource?

@Paulb23
Copy link
Member Author

Paulb23 commented Sep 30, 2021

The short answer is not really.

The longer answer is, if we did remove TextFiles as a resource, we'd have more edge cases, as everything is expecting a resource and assumes it to be handled by the Resource[Loader|Saver]. E.g Currently, with this the QuickOpen menu works without any changes.

Given that, the other solution is to modify Resource[Loader|Saver]. This will mean touching core which reduz was originally against. And by doing so it would become re-exposed to users, which has caused enough issues during 3.x.

The only true way to remove it, is to officially support TextFiles and not make it editor only (Will probably still need changes to Resource[Loader|Saver]).

@reduz
Copy link
Member

reduz commented Sep 30, 2021

Some issues with this PR

  • Godot uses .cfg extension for some files like export settings. If we make this editable, they will be visible in the filesystem dock, which is likely not great.
  • In master there is a JSON object which inherits refcounted, question is whether it may not be better to simply make it a Resource instead that you can load. Of course if you do this it will not be a text file.

@reduz
Copy link
Member

reduz commented Sep 30, 2021

Aditionally, you maay want to treat JSON as a special case because it can be parsed and throw errors too (Godot JSON object supports this, if parsing is broken it tells you where exactly)

@Paulb23
Copy link
Member Author

Paulb23 commented Sep 30, 2021

I think for now, we keep JSON as a plain text file. Then for 4.1-4.2 we can add improved support for editing JSON, as ScriptEditorPlugin / ScriptEditorBase is due a refactor (in part for adding multi-window support).

@reduz
Copy link
Member

reduz commented Sep 30, 2021

@Paulb23 sounds good, as long as its nothing that will require large compat break we can kick to 4.1 or other minor versions.
We would likely need to then fix the export settings so it uses a different extension, like gdcfg

@Paulb23 Paulb23 requested a review from a team as a code owner September 30, 2021 19:29
@Paulb23
Copy link
Member Author

Paulb23 commented Sep 30, 2021

Updated to change export_presets.cfg to export_presets.gdcfg, included some renaming code, so should not break compatibility.

@akien-mga
Copy link
Member

We would likely need to then fix the export settings so it uses a different extension, like gdcfg

Personally I don't think that's necessary. It's yet another unique extension for a single file, for a supposed problem which I'm not sure will actually be a problem.

It's not particularly useful to open export_presets.cfg in the script editor, but it doesn't hurt either. As pointed out, it can be done anyway from the File menu.

If we want to hide it from the FS dock, I'd special case it there, but in general I'd prefer to merge this PR first, and then we can assess if there's any text file that we don't want to expose for easy opening as TextFile.

So I'd leave out da35bc3 (can be kept in case it's wanted later on) to merge this PR otherwise as is.

@Paulb23
Copy link
Member Author

Paulb23 commented Sep 30, 2021

If we want to hide it from the FS dock, I'd special case it there, but in general I'd prefer to merge this PR first, and then we can assess if there's any text file that we don't want to expose for easy opening as TextFile.

So I'd leave out da35bc3 (can be kept in case it's wanted later on) to merge this PR otherwise as is.

Sounds like a plan, removed the commit.

@akien-mga akien-mga merged commit 3e1b630 into godotengine:master Sep 30, 2021
@akien-mga
Copy link
Member

Thanks!

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