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

Change default Save Script shortcut (reverted) #79337

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jul 11, 2023

Fixesnot #78672 🤪 EDIT: Unlinked, see comment below.

Why this works:

  • when you are outside Script Editor, Ctrl+S will save the scene
  • when you are inside Script Editor, Ctrl+S will save the script

Also:

  • by default scenes are saved when running the project
  • when exiting the editor you will get quit confirmation if a scene is unsaved

Thus there isn't (much?) risk of data loss that could be caused by saving the scene less often.

I tested this solution for a day (by changing my shortcut in editor settings xd) and aside from the fact that I'm used to the old shortcut, it works quite well.
Alternatively we could add a second shortcut for saving scenes (e.g. Ctrl+Shift+S), but I think it's not necessary.

@KoBeWi KoBeWi added this to the 4.2 milestone Jul 11, 2023
@KoBeWi KoBeWi requested a review from a team as a code owner July 11, 2023 12:39
@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 11, 2023
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Makes sense to me. The only reason why this setting differed from the Save Scene one is because before 4.0, we didn't support shortcut contexts.

Now that we do, it makes sense to reuse Ctrl+S for both, which is probably what most users expect (we all save the scene needlessly whenever we want to save our script changes).

@YuriSizov
Copy link
Contributor

I would like to mention that the linked issue is not entirely fixed by this. Or at least the underlying design problem with how shortcuts are assigned to controls, which allows them to steal focus like that. This PR addresses one particular case, but even after we merge it the issue would still be reproduceable in other ways and situations.

We should differentiate between shortcuts associated with a control and shortcuts associated with the entire application. Even if "Save current scene" is attached to some element in some part of the UI, it is definitely not expected by the user that pressing the shortcut combination from another context would focus that element or its parent. It's seen as a "global" action, not a contextual action, so context switching from triggering it would be a nuisance at best.

But this change here is still a good improvement that wasn't possible in Godot 3. I would just keep the issue open for now. And also avoid cherry-picking into a patch release, since it changes default shortcuts and can cause conflicts.

@YuriSizov YuriSizov merged commit 770b7e9 into godotengine:master Jul 12, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@KoBeWi KoBeWi deleted the best_fix_ever branch July 12, 2023 15:28
@jordanlis
Copy link

YuriSizov so his is a "quickwin" that will allow to save script with ctrl alt s but is it planned that saving with ctrl + s in the script editor won't focus on the main window ?

I totally agree with your comment btw, hence the question

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 16, 2023

@jordanlis You can already save with Ctrl+Alt+S without focusing the main window. This PR just changes the shortcut to Ctrl+S, so while you are inside script editor, by default Ctrl+S will save the script instead of the scene.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@YuriSizov
Copy link
Contributor

Skipping from cherry-picking as mentioned before.

@akien-mga
Copy link
Member

For the record, reverted by #84931 as that change turned out not to be consensual.

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.

4 participants