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

Expose reload_scripts as reload_open_files #83267

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KANAjetzt
Copy link
Contributor

@KANAjetzt KANAjetzt commented Oct 13, 2023

Expose the reload_scripts method to match the current state of the 3.x branch.

As described in #83118, it is possible to replicate this method with GDScript:

static func reload_script(text_edit: TextEdit, source_code: String) -> void:
	var column := text_edit.get_caret_column()
	var row := text_edit.get_caret_line()
	var scroll_position_h := text_edit.get_h_scroll_bar().value
	var scroll_position_v := text_edit.get_v_scroll_bar().value

	text_edit.text = source_code
	text_edit.set_caret_column(column)
	text_edit.set_caret_line(row)
	text_edit.scroll_horizontal = scroll_position_h
	text_edit.scroll_vertical = scroll_position_v

	text_edit.tag_saved_version()

and set the text_editor/behavior/files/auto_reload_scripts_on_external_change setting to true.

Personally, I would prefer calling EditorInterface.get_script_editor().reload_scripts(), but I might not be aware of possible pitfalls caused by exposing this method.

This would also improve the porting of existing 3.5 plugins.

closes godotengine/godot-proposals#8111

@KANAjetzt KANAjetzt requested review from a team as code owners October 13, 2023 13:41
@AThousandShips
Copy link
Member

AThousandShips commented Oct 13, 2023

Please remove the special characters from the commit message and make it match the title of the PR

Also please open a proper proposal as asked on the issue, having a proper proposal where the proper arguments for this feature is important to gauge if this is needed

editor/plugins/script_editor_plugin.cpp Outdated Show resolved Hide resolved
doc/classes/ScriptEditor.xml Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

AThousandShips commented Oct 13, 2023

@twaritwaikar do you recall if exposing this (i.e. making public) in 3.x was meant to be used in scripting, or was it renamed since it was made public on the c++ side?

This was done in #53900 but only for the 3.x side, so the fact that this was not done in 4.x to me implies that it was not intended to be exposed as such

Edit: these changes were ported to 4.x in #62157 where this was not exposed

@Qubus0
Copy link

Qubus0 commented Oct 13, 2023

This is useful (and used) for all plugins that modify any (active) scripts.
I'm not sure why it would not be intended to be exposed. Would there be any downside to having it exposed or is it a case of needing a certain degree of usefulness to be justified?

@AThousandShips
Copy link
Member

Exposing internal methods, especially in the editor, binds the development to maintain compatibility and stability, so that's why I asked for a proposal to gauge the need and justification for this

If it can be worked around, how needed it is, etc.

Because exposing a method isn't a trivial thing that comes with possible issues and a burden of maintenance

@KANAjetzt KANAjetzt force-pushed the feat_expose_reload_scripts branch from 2f93ffe to 3a245a9 Compare October 14, 2023 10:33
@KANAjetzt
Copy link
Contributor Author

What can I do to make the Linux check pass?

@AThousandShips
Copy link
Member

See the error message, you need to reorder the entries of the documentation, they need to be ge sorted

@KANAjetzt
Copy link
Contributor Author

KANAjetzt commented Oct 17, 2023

Sorry for that unnecessary check, I hope I got it now 😅
The doctool got stuck when trying to delete the cache while VSCode was running.

@Paulb23
Copy link
Member

Paulb23 commented Oct 29, 2023

Don't see a solid reason against exposing this for plugins, but I'm thinking we should start moving away from the script naming in this file a little bit as reload_scripts doesn't just reload Script, but any open file i.e TextFile and JSON.

So we should bind it as reload_open_files or something along those lines.

@KANAjetzt KANAjetzt force-pushed the feat_expose_reload_scripts branch 2 times, most recently from 467d9ea to 5e07dcb Compare October 30, 2023 10:07
@KANAjetzt KANAjetzt changed the title Expose ScriptEditor.reload_scripts() Expose reload_scripts as reload_open_files Oct 30, 2023
@KANAjetzt
Copy link
Contributor Author

Thanks, @Paulb23 - I went ahead and changed it to your suggestion. Is there something to do now on the project converter side?

@KANAjetzt KANAjetzt force-pushed the feat_expose_reload_scripts branch 2 times, most recently from 04d8004 to 7631d87 Compare March 18, 2024 16:05
@KANAjetzt
Copy link
Contributor Author

I added the function to the rename map, I hope we can get this merged for 4.3 👀

doc/classes/ScriptEditor.xml Outdated Show resolved Hide resolved
@KANAjetzt KANAjetzt force-pushed the feat_expose_reload_scripts branch from e369043 to 2687a6c Compare April 27, 2024 20:41
Expose the reload_scripts method to match the current state of the 3.x branch.
Bind it as `reload_open_files` in contrast to `reload_scripts` in 3.x to more accurately reflect its function.

closes godotengine/godot-proposals#8111

clarifies the description of the method's internals

Co-authored-by: Chris Cranford <[email protected]>
@KANAjetzt KANAjetzt force-pushed the feat_expose_reload_scripts branch from 2687a6c to 8c32133 Compare September 8, 2024 20:17
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.

Expose ScriptEditor.reload_scripts()
5 participants