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

Open all files in script editor #19225

Merged

Conversation

Paulb23
Copy link
Member

@Paulb23 Paulb23 commented May 28, 2018

This PR allows editing any file in the script editor.

Firstly, a new ResourceFormat called TextFile. The TextFile format can take any type of file (providing its utf_8).

As TextFile can take any type the resource_loader and resource_saver methods had to be changed. They will only load TextFile formats when explicitly asked for. This does however means that we have have to wait for the initial loading call to fail before trying again. This ensures that nothing else can load it. As loader order is "random".

To make editing all files possible, the hint type passed into InspectorDock::_load_resource is now passed all the way through the system. The script editor passes type Script. This is used to indicate that if the resource is not loaded we can safely try using TextFile. Allowing editing scene files within the ScriptEditor.

Secondly, I've Generalised ScriptEditorPlugin and ScriptEditorBase to take and edit any resource.

Thirdly, introduced a new TextEditor class extending ScriptEditorBase to edit the TextFile resources. Currently this is is fairly lightweight.

Lastly, I moved some of the text manipulation into CodeTexteditor reducing duplication from the ShaderTextEditor, ScriptTextEditor and would be TextEditor.

The changes from #19166 will have to be replicated into TextEditor.

closes #1390


I'm unsure about the resource_loader changes, weather the loader should internally check for Script or force the caller to handle the failed load?

The TextEditor class will also need a icon to use in the script list.

Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing!
It is funny that I was just starting to basically do the exact same thing. #19220
I came to almost the same conclusions but I would have implemented it with a little less code (I was lazy and was not sure how much this even would be used.)
So it looks really good to me and makes a lot of sense.
Also Thank you! for you timing so I didn't yet spend any time on it.

It is not really a review but more a bunch of questions which would make me super happy if I get an explanation.

Thank you! this pr is amazing

@@ -1661,11 +1687,13 @@ void ScriptEditor::_update_script_names() {
_update_script_colors();
}

bool ScriptEditor::edit(const Ref<Script> &p_script, int p_line, int p_col, bool p_grab_focus) {
bool ScriptEditor::edit(const RES &p_script, int p_line, int p_col, bool p_grab_focus) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would also rename the actual prop name to p_resource

@@ -116,7 +117,7 @@ class ScriptEditorBase : public Control {
};

typedef SyntaxHighlighter *(*CreateSyntaxHighlighterFunc)();
typedef ScriptEditorBase *(*CreateScriptEditorFunc)(const Ref<Script> &p_script);
typedef ScriptEditorBase *(*CreateScriptEditorFunc)(const RES &p_script);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same here. I would make it really obvious for ppl who work on it, that any resource "could be" edited. so calling the property p_resource makes sense to me?


#include "script_editor_plugin.h"

class TextEditor : public ScriptEditorBase {
Copy link
Contributor

@toger5 toger5 May 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this class is just phenomenal and makes soo much more sense.

}

bool ResourceFormatLoaderTextFile::handles_type(const String &p_type) const {
return (p_type == "TextFile");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still did not completely get the idea of handles_type Is passed when the editor is searching for a ui element to handle a specific resource? so I could edit a tscn with the type TextFile and with the type Scene and based on that the editor would behave differently?

I think I got it wrong because than it would make no sense why it is in ResourceFormatLoaderTextFile

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handles_type is used to select a resource loader.

When calling:
ResourceLoader::load("bla.txt")
it will select the first ResourceFormatLoader that supports the extension txt.

If However you call it like so:
ResourceLoader::load("bla.txt", "TextFile")
it will select the first ResourceFormatLoader that supports the extension txt and handles_type of TextFile.

So even if infinite ResourceFormatLoader support txt only ones that handles_type of TextFile will be selected. If none handle TextFile the resource will not be loaded.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay Thank you!
so it is basically load(file_on_disc, as_resource_type) (if possible ;) )

@Zylann
Copy link
Contributor

Zylann commented May 29, 2018

Does it mean we can also edit shaders there?

@akien-mga akien-mga added this to the 3.1 milestone May 29, 2018
@Paulb23 Paulb23 force-pushed the open_all_files_in_script_editor branch from b9de0b2 to c5f9c6f Compare May 29, 2018 18:16
@Paulb23
Copy link
Member Author

Paulb23 commented May 29, 2018

Renamed p_script to p_resource.

@Zylann Sort of, the shader editor grabs focus if it is open or has been opened.
Otherwise yes, though I can't recommend it as its like working in notepad.

@toger5
Copy link
Contributor

toger5 commented May 29, 2018

Moving it shouldn't be to hard if we decide that it is a nicer workflow.

But since you usually want to see your result it should stay at in the bottom panel.

@Geequlim
Copy link
Contributor

May related to #5311

@reduz
Copy link
Member

reduz commented Jul 4, 2018

I am against hacking ResourceLoader/Saver for this. Just harcode this support into the editor plugin, even if its considerably more code.

@Paulb23 Paulb23 force-pushed the open_all_files_in_script_editor branch from c5f9c6f to 724762c Compare July 15, 2018 14:45
@Paulb23
Copy link
Member Author

Paulb23 commented Jul 15, 2018

Integrated the changes from #19166 into text_editor.

Adjusted to no longer use ResourceLoader or ResourceSaver. Instead the ScriptEditor/ScriptEditorPlugin will handle all TextFile calls internally.

To allow Opening/Save As, it uses its own EditorFileDialog and will switch based on the type of file selected. If its a known Script type it will be processed as normal, else will be treated as a TextFile.

@Paulb23 Paulb23 force-pushed the open_all_files_in_script_editor branch from 724762c to a05ae24 Compare July 15, 2018 15:22
@Paulb23 Paulb23 force-pushed the open_all_files_in_script_editor branch from a05ae24 to 8cd2e48 Compare July 15, 2018 15:23
@Paulb23 Paulb23 force-pushed the open_all_files_in_script_editor branch from 8cd2e48 to 8ff7471 Compare July 22, 2018 10:56
@mhilbrunner
Copy link
Member

I just want to share this from our PR meeting where we agreed to merge this:

reduz> oh wow, it's really well done
Akien> Paul can work magic :)

I can only agree. Huge thanks for this!

@mhilbrunner mhilbrunner merged commit b92c432 into godotengine:master Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text editor should allow editing non-script files
7 participants