Save scene together with script/shader when Ctrl+S is pressed. #84623
Conversation
|
@AThousandShips this is meant for 4.2 actually, It's to avoid workflow breaking from 4.1 to 4.2. |
|
Then is it exclusive with: Assumed 4.3 as it seemed like a more involved change and we're late in the cycle for such a change, with no milestone assigned to start with |
|
#84614 should still be correct and important to merge, but indeed this PR should be tested with the changes from 84614 to confirm that it still does what it's meant to. |
akien-mga
changed the title
akien-mga
added
the
cherrypick:4.2
|
I think this approach can make sense, but I prefer not to rush it now just before the 4.2 stable release. The current behavior isn't too bad in 4.2, and we can cherry-pick improvements for 4.2.x after having reviewed and tested them properly. |
| EditorNode::get_singleton()->save_resource(resource); | ||
|
|
||
| if (resource->get_path().is_resource_file()) { | ||
| // If resource file exists, this means that the shader will not trigger a save-as popup-up. |
There was a problem hiding this comment.
Note that Save As popup is caused by Shader Editor not handling built-in scripts properly. #84630 will resolve this.
EDIT:
The PR was merged, so the comment/code needs adjustment.
| Ref<Shortcut> save_shortcut = ED_GET_SHORTCUT("script_editor/save"); | ||
| Ref<Shortcut> scene_save_shortcut = ED_GET_SHORTCUT("editor/save_scene"); | ||
| if (save_shortcut.is_valid() && scene_save_shortcut.is_valid()) { | ||
| if (save_shortcut->is_match(scene_save_shortcut)) { |
There was a problem hiding this comment.
This condition will be true even if you press unrelated event. E.g. if your Save Scene shortcut has Ctrl+S, script editor has Ctrl+S and Alt+S, and then you press Alt+S, the scene will be saved even if the event you pressed was unrelated.
There was a problem hiding this comment.
Yeah, but this is very unlikely to happen and but I don't think there is any other way to do this, as you can't currently check in an easy way if a shortcut is being pressed.
|
@akien-mga I strongly disagree that the current behavior is acceptable. Every time I work and do shaders or edit scripts I am hit by this. This is a regression from 4.1 and IMO it should be fixed before 4.2 stable. |
reduz
force-pushed
the
script-save-also-save-scene
branch
from
b026062
to
284dc9a
Compare
Fixes godotengine#84601. The scene will also silently be saved (just like before), since many users are not necessarily aware that the script or shader editor editors are focused at the time of saving. Now, however, if something causes the save to fail it will not be notified to the user, in case the user to avoid the annoyance caused by godotengine#84039.
reduz
force-pushed
the
script-save-also-save-scene
branch
from
284dc9a
to
4a34c24
Compare
@robbertzzz Both script and shader editors have the same indication as scenes, a little asterisk after the name of an unsaved resource. It's in the sidebar to the left.
It is an editor setting, you can change shortcuts to anything you want, including the old behavior where all shortcuts were different. |
The script editor doesn't for me in the latest beta.
Then why does CTRL+S not save both at the same time? |
Please open an issue. |
To have the old behavior you need to change the shortcut for the script and the shader editors to be something else. Then Ctrl-S will always save the scene, and as a result of that — save some related edited resources. This is the old behavior as available in Godot 4.1 and prior. |
But it's extremely unintuitive that having CTRL+S set as saving the scene does not save the scene. For usability sake, it should. If I have CTRL+S set to save both the scene and the script, I expect it to do just that when I press CTRL+S. Anything else is bad UX. |
Seems to be user error, it didn't happen before I undid my random changes but looks like that's because the parser doesn't update immediately. |
Well, pressing Delete in Script/Shader Editor does not delete the currently selected node. I don't see why save shortcut should be special. |
Delete is not a shortcut. The main problem for me though is that saving the scene does not save just the scene. It saves any unsaved resources. It's inconsistent, and consistency is a key element of UX design that is being ignored with this approach. Besides all that, the current implementation differs from the previous engine version in an unclear way. The only reason I'm here is because I was looking for a bug report on saving not always working. You can definitely expect lots of complaints if this editor behaviour suddenly changes for all users, because it's a key feature that literally all of us use. |
It is though Edit:
Now let's focus on something constructive |
|
Delete is a shortcut that can be rebinded in editor settings. If you want a better example, Ctrl+A. Godot 3 has an outstanding issue that trying to select all text often brings up Add Node dialog, because they have the same shortcut. In Godot 4 shortcut contexts are properly separated, so such bugs do not happen anymore, and that applies to Save too. |
|
My understanding is, that there was a behavior change:
I would also prefer that Ctrl-S is treated as an application-global shortcut, that saves |
It only saved one scene, and that scene saved its modified resources. It didn't save everything. Presenting it as such is misleading, we don't have a way to save everything at this moment. |
|
Then my understanding was wrong. Thanks for the correction. |
|
For the record, we've reverted the script and shader save shortcuts to their 4.1 bindings, so that Save Scene still takes precedence everywhere for Ctrl+S (#84931). Since we didn't have consensus on the shortcut change in the end, going back to the previous status quo is usually how we deal with it. We can keep evaluating what this PR offers to implement to see it would improve the UX for all use cases, or if we should take another approach. |
@AThousandShips Shortcuts are, by definition, a quick-hand way to do something that's hidden behind a menu otherwise. Delete is a function key that deletes stuff. In some cases it can double as a shortcut, but inside the code editor it only serves its intended purpose: deleting text that is in front of the cursor. Within that context it's a function key, not a shortcut, and therefore not very valuable to this discussion. The later provided example of CTRL+A by @KoBeWi is what I would consider not-so-great UX. The same shortcut suddenly has a second purpose that is not commonly associated with it, leading to external inconsistency (a key pillar of user-centred design). But regardless, we can talk about examples of some shortcuts being context-specific for ages, fact of the matter is that CTRL+S typically saves whatever is currently open. If we take any IDE as an example, that's the current script you're editing. In case of Godot, you'll have both a script and a scene open at the same time within the same window so external consistency dictates that both of those should be saved. To take it one step further, other game engines that have a built-in code editor also save everything when hitting CTRL+S (GameMaker Studio, GDevelop), so again for external consistency's sake Godot should be no different. I could see how having the code editor popped out into a separate window changes this discussion: at that point only the script is being edited in the active window (similar to a split-screen IDE; each "screen" acts as a separate entity) and there it makes more sense if only that script gets saved. I'd be all for a contextual save option being linked to the active |
That's not how everyone sees it. I agree on the premise that you expect to save what you have open, but in my mind, if I have a script open, then I don't have a scene open. The script takes precedence, it takes the whole view. There is no way to interact with the "current" scene, any more than there is for any other scene tab currently in the background So I want to save what I have open, and that's the script. But instead the unexpected happens, the scene is also saved for some reason. This has been pretty annoying for years because there was no way around it. You had to learn a secondary set of shortcuts and press awkward key combinations to get the desired result. Godot 4 resolves the limitation. So thankfully, even with the defaults reverted it is now possible to set the same shortcut to be contextual and actually save what is open, and not what is somewhere in the background. |
The script does not take the whole view though, it only takes up the centre portion of the screen. You can still interact with the scene while editing a script.
What is annoying about things saving? It's annoying that they don't save.
Which makes sense, because the script only takes up part of the view so it shouldn't override the rest of the UI.
It was never a problem, just use a different shortcut. |
|
@robbertzzz The point of my comment was to explain to you that not everyone sees the world the same way you do. People build different mental models of their work environment. Where you see the scene as always open, others see what is presented in the center view as the currently edited entity. And expect actions to behave accordingly. Telling others that the way they experience things is wrong is not productive or helpful.
I was talking about the technical limitation where we could not use the same shortcut in different contexts safely. You can find many reports from users trying to duplicate a line in the code editor and triggering this same action in the shader editor instead, if both of them are open. This was a big problem which was resolved in 4 thanks to shortcut contexts. |
Can you please leave the accusatory language out? I'm trying to explain, as someone with a degree in this stuff, what the typical way is to approach problems like this. I'm trying to show, with proper reasoning, why something isn't how proper usability design is typically done. Yet the only thing I'm hearing back are personal opinions of individual users. Everybody has different opinions on things, there is never a 100% right answer like you said. That's why usability design instead focuses on concepts like external consistency to define what the generally most-accepted solution is. That way, instead of just a useless back-and-forth between disagreeing users, you have a proper reasoning to fall back on. I'm trying to give that reasoning for my point of view, I'd like to hear yours (which should not be that some individuals don't like it, except if there's statistical proof that a majority doesn't like it).
That's a different problem with a similar cause and not relevant to the discussion of how CTRL+S should behave. |
|
"Just copy what others do" is not a valid approach to user experience and software design, which discards our problem space and experiences of our users. This does not lead to accessible tools, only to stale ideas that cater to the same crowd while ignoring others as statistically insignificant. And pointing out that you're spending time telling people how wrong they are to expect certain behavior from their software is necessary because that's what you're doing. This doesn't create a productive conversation when you tell others that their experiences don't matter because you have a degree and some stats. In any case, neither of us are adding anything to this discussion, so I suggest we stop it. The current version of Godot 4.2 allows for both behaviors to exist, so there is nothing worth splitting hairs over here. The remaining merits of this PR are beyond your points anyway, have nothing to do with the Ctrl-S behavior, and should be discussed further. |
Seriously, more accusations of things I didn't do after I asked you to stop? You're right, this discussion is going nowhere if you can't even keep it professional. As a core member of the team you should be open to feedback rather than trying to shut it down through personal attacks. I'm not dismissing any individual's views, I'm saying that in the larger picture they don't make a lot of sense from the point of usability design. As I said, "there is never a 100% right answer", and that's why anecdotal arguments don't make for the best discussion on usability. Again, if you have any arguments that are not "me and two others don't like it", I'd love to hear them, because that's the kind of evidence-based discussion I've been trying to have here.
External consistency, "copying" others, is one of the key pillars to UX and software design. It's also a key element of game design; most games are combinations of other games and they often blatantly say so in press releases. Random examples: Stardew Valley is a spiritual successor to Harvest Moon and tries to be an improvement of that game; Cities: Skylines tried to bring SimCity to a modern era. In their day-to-day jobs game designers (and other UX designers) spend a lot of their time looking at references to see how they approached things. A great example specific to games is the extremely popular and widely used Game UI Database. It allows you to quickly explore trends between different games, find UI for games in a similar genre as yours, etc. It's used extensively by the UI designers at the studio I work at. And if you still don't believe me, here are some examples of external consistency:
I hope you get my point by now: external consistency is everywhere and each of these examples would lead to terrible usability if they did not follow this rule of external consistency. This concept is one of Jakob Nielsen's widely accepted usability heuristics, this is a great read on them. The original papers are here and here.
I'm still trying to add things to this discussion, but feel free to step out if you have no counterpoints to anything I said and instead choose to keep personally attacking me.
This PR literally addresses CTRL+S behaviour, and this is the right place to have this discussion according to @akien-mga as mentioned in #84601 . |
|
What you call anecdotal evidence is me not wanting to speak for others. But if you want me to, fine.
Dozens if not hundreds of people used 4.2 dev snapshots and betas, and only "me and two others" didn't like the change of the shortcut behavior. There are literally only a couple of reports, and that's at the time when we had a huge influx of Unity refugees trying Godot for the first time. The only reason we are having this discussion right now is because reduz personally didn't like the change. Besides him, only a couple of people were bothered by it during the last 4 months. This is all the evidence I need to know that this change is not as controversial as it appears to be from your point of view. In fact, the biggest bother that people experienced was due to an unrelated bug in the shortcut implementation in the shader editor. Which was fixed. If not for this bug, I'm not sure Juan or other people would've even noticed that something is "off". Because it would just work, naturally.
Asking you to be open-minded about people who don't think exactly like you is hardly me shutting down the discussion. Brushing off personal experiences as statistically insignificant is dismissing other views. We aren't building the engine by stats, we are building it for people. For people choosing Godot for what it brings to the table, including in the area of user experience. What other applications do is for the most part irrelevant and should only be referenced, not used as a guidebook. There are many applications that do not agree on standard shortcuts for most mundane and widely available actions. Like duplicating a line in a text editor. A million different things plays into it, starting from the thing most developers don't even consider: keyboard layouts and different languages. User comfort can differ drastically when you step outside of the US English layout and consider some typical shortcuts. So everyone ends up solving it in their own way, and users end up rebinding keys to their liking. Which is a totally normal thing to do in a professional app with a varied subject area. We are making a tool that should work in the hands of many differently skilled professionals, from different fields and crafts. Where different UI paradigms are used. And we have an editor where multiple things can be edited at the same time, from multiple windows, with multiple conditions being true at the same time. It's easy for you to argue for a solution that you like by stating that external consistency matters. But this approach fails to recognize that people don't think the same way, and complex applications don't always work the same way. We don't have a universal save function. We only have contexts. And what contexts people see differs. And it's part of Godot's unique UI. And this unique UI with its unique considerations must be accounted for. You can't just mindlessly copy others and think that this offers the best user experience for your users. It may make the transition from other tools easier, but that isn't the main goal of a proficiency oriented app. The learning curve may be steeper but the outcome may be way better if you stop thinking within external constraints and start to recognize patterns that form within your medium. After all, we are not breaking the expectation of what Ctrl-S does in principle. We are adjusting what it does specifically. It still saves things, with or without the recent changes. But what things, how it saves them, that's open to interpretation. And it all depends on the app itself, not external factors.
What this PR addresses right now is that we don't show error when you save a script with an empty scene open. That's not relevant to the general discussion about what Ctrl-S should do. |
A personal opinion is anecdotal evidence. That's not what I call it, that's what it is.
So.. you agree with me?
You weren't asking me anything, you were accusing me of things I didn't do.
And how do you gather opinions and define which opinions are relevant, if not through numbers? You build it for people through the stats of those people, or at least that's what I hope you do because that's how proper UX design evaluation is conducted.
I'm not saying that what others do is a guidebook. I'm saying external consistency is a great starting point. After building features based on such heuristics the first thing you should do is test it with a sample of your target audience. A heuristic is by definition not deterministic.
I agree, which is why I used code editors and other game engines as an example rather than something completely unrelated.
Again, I agree, this is what I've been saying all along. External consistency was a starting point, not an endpoint. I tried to come with an overarching reasoning because I felt it showed well why I think things should work a certain way. The majority might disagree, but we don't have those statistics, so instead I'm using common arguments for design decisions to argue my view. For the third time, feel free to come with counterarguments to these within the actual discussion about save behaviour.
I never said to mindlessly copy others, those are your words. I gave my reasoning for my opinion on why things should work a certain way, as I've now mentioned multiple times.
That's, again, why I took inspiration from tools in the same medium.
It depends on what people expect, not the app itself or external factors. However, other software is a great reference for predicting what people are used to.
Sure, but that's not the discussion from which people are sent here. This is a discussion to have with Remi. This is an extremely fruitless discussion since you're only willing to talk about the overarching meta. Unless you want to come with actual reasons why you disagree with my view on behaviour of the editor I suggest we stop having this conversation. |
No, you've misread. I'm saying that almost nobody was bothered by the change that you were asking to revert. |
|
Closing as the original changes that inspired this PR were reverted. It makes no longer sense to keep it open. |
AThousandShips
added
archived
and removed
cherrypick:4.2
Fixes #84601.
If both "Script -> Save Script" and "Scene -> Save Scene" use the same shortcut, the scene will also silently be saved (just like before), since many users are not necessarily aware that the script or shader editor editors are focused at the time of saving. Now, however, if something causes the save to fail it will not be notified to the user, to avoid the annoyance caused by #84039.