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 save_all_scenes method to EditorInterface #77537

Merged

Conversation

henriquelalves
Copy link
Contributor

@henriquelalves henriquelalves commented May 27, 2023

This PR exposes save_all_scenes method to the EditorInterface class. It's a very simple change, but I don't think there were other ways to do this in the editor.

I added the documentation for this method and changed, but I wanted to propose changing save_scene to save_edited_scene (and save_scene_as to save_edited_scene_as). It's a breaking change, but the previous method names gives the wrong impression on what they do, since it's not possible to use them to save an arbitrary editor scene. Is it okay to add this on this PR, or should I ignore this?

PS: My use-case is a very simple addon with pre-configured resolutions for different mobile devices. I need to change the resolution in the inspector quickly, without the hassle of opening Project Settings multiple times, and I need the addon to reload every open scene to see the changes. Problem is, when the scenes reload, any modification not saved is lost, so I need to save them first. Before this PR, I was saving all scenes by running play and stop via code, because by default the editor saves all scenes before playing the game - but that certainly wasn't efficient nor the best way to do this.

@henriquelalves henriquelalves requested a review from a team as a code owner May 27, 2023 02:46
@Chaosus Chaosus added this to the 4.1 milestone May 27, 2023
@YuriSizov
Copy link
Contributor

YuriSizov commented May 29, 2023

You need to fix the documentation. You've probably edited it manually, but the order is incorrect. Make sure to use the doctool instead of editing by hand, and then fill out the description. See this doc.

I agree that the names of the existing methods give a wrong impression, but we don't consider such breaking changes anymore, since Godot 4 is stable. That said, I think your problem can be solved if we were to expose a proper save_scene method that takes a scene resource path as an argument. Then you can use existing API to iterate over open scenes and save them one after another. But exposing existing "save all" method is fine too.

@henriquelalves
Copy link
Contributor Author

Done!
I just added a bit on the save_scene and save_scene_as to clarify their functionality.

@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 2, 2023
Copy link
Contributor

@Eoin-ONeill-Yokai Eoin-ONeill-Yokai left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for submitting it! It looks like some people in the repository put the "(squash)" notice at the end of the squashed commit titles though I'm admittedly not sure how rigid we are about this as a commit formatting standard.

@YuriSizov
Copy link
Contributor

It looks like some people in the repository put the "(squash)" notice at the end of the squashed commit titles

That would be incorrect.

@Eoin-ONeill-Yokai
Copy link
Contributor

It looks like some people in the repository put the "(squash)" notice at the end of the squashed commit titles

That would be incorrect.

Ok thanks for the clarification!

@AThousandShips
Copy link
Member

AThousandShips commented Jun 2, 2023

This is caused by using squash instead of fixup, it's confusing when we say to "squash" but see the PR workflow instructions (meant that it can be confusing and think it's only squash that should be used)

@YuriSizov
Copy link
Contributor

YuriSizov commented Jun 2, 2023

@AThousandShips Both actions are squashing commits. It's just one appends the commit message from the squashed commit, and the other drops it. But let's not derail this PR review too much, OP is completely fine here :) But it will have to wait a bit for the 4.2 merge window.

Update documentation

(squash) fix docs

(squash) Improve docs
@YuriSizov
Copy link
Contributor

Thanks for the rebase :) I'm adding this PR to the queue so we don't forget about it.

@YuriSizov YuriSizov requested a review from KoBeWi July 26, 2023 15:46
@YuriSizov YuriSizov changed the title Add save_all_scenes method to Editor Interface Expose save_all_scenes method to EditorInterface Jul 26, 2023
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

LGTM. Important note that this will stop the currently playing scene.

@YuriSizov YuriSizov merged commit 41a7f6b into godotengine:master Jul 26, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

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.

None yet

5 participants