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

Improve save handling for built-in scripts #54653

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 6, 2021

This PR improves the way built-in scripts saved state/saving is handled, to bring them pretty on par with normal scripts.

Summary:

  • unified the way of getting script name. Script editors have get_name() method for that, but built-in scripts had some weird special case that omitted this method (even though the method itself also had a case for built-in scripts lol). I unified that
  • with the above, the "saved" status of built-in scripts is now properly tracked. Before they were always internally "unsaved" (and closing a modified built-in script always opened the save dialog) and never displayed their status, now unsaved built-in scripts properly display (*)
  • whenever scene is saved in editor, all built-in scripts that belong to this scene will be marked as saved
  • when you save a built-in script (e.g. using Ctrl+Alt+S shortcut), its scene will be saved instead of trying to save the resource and opening a "Save As" dialog

lvknwpP6jc

@KoBeWi KoBeWi added enhancement topic:editor usability cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Nov 6, 2021
@KoBeWi KoBeWi added this to the 4.0 milestone Nov 6, 2021
@KoBeWi KoBeWi requested review from a team as code owners November 6, 2021 01:42
@KoBeWi KoBeWi force-pushed the built_in_scripts_deserved_that branch from 606ab52 to 39dea65 Compare November 6, 2021 02:32
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

Looks good.

Would be good to apply the get_name changed to text_editor as well.

Not sure if we should also change the FILE_SAVE_ALL action to take into account built-in scripts.

Comment on lines -992 to -995
if (script->is_built_in()) {
continue; //internal script, who cares
}

Copy link
Member

Choose a reason for hiding this comment

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

Second thoughts, rather then having a separate method and signal can we not handle it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The _res_saved_callback isn't called for built-in resources. I removed this check, but it was useless anyways.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 7, 2021

Would be good to apply the get_name changed to text_editor as well.

Are built-in TextFiles even a thing? Also #28607 strikes again.

Not sure if we should also change the FILE_SAVE_ALL action to take into account built-in scripts.

It would be easier if saving wasn't handled in multiple places :/

save_all_scripts is called during save_external_data and built-in script isn't really an external data, so I didn't handle it. But it probably makes sense for Save All option in menu...

@KoBeWi KoBeWi force-pushed the built_in_scripts_deserved_that branch from 39dea65 to 9275b48 Compare November 7, 2021 22:40
@KoBeWi KoBeWi force-pushed the built_in_scripts_deserved_that branch from 9275b48 to 134e4d1 Compare November 7, 2021 22:51
@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 7, 2021

So it turns out that saving scene when saving script brings many problems (who would've thought...). I had to move the scene_saved signal.

@akien-mga akien-mga merged commit e3f3fc5 into godotengine:master Nov 9, 2021
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the built_in_scripts_deserved_that branch November 9, 2021 20:55
@akien-mga
Copy link
Member

Would need a dedicated cherry-pick PR for 3.x.

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.

3 participants