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

Fix editor screenshots menu button's tooltip #92836

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

WhalesState
Copy link
Contributor

@WhalesState WhalesState commented Jun 6, 2024

No description provided.

@AThousandShips
Copy link
Member

This breaks at least workflow compatibility, I'd argue this requires a proposal to discuss if this is a good idea in the first place

@WhalesState
Copy link
Contributor Author

WhalesState commented Jun 6, 2024

This breaks at least workflow compatibility, I'd argue this requires a proposal to discuss if this is a good idea in the first place

We can just save them separately in the editor data folder, the issue is that it's saving to the project data folder, while only running games should save there, not the editor.

image

@AThousandShips
Copy link
Member

My bad you didn't mention this was not the intended behavior, based on the tooltips

However it still needs to be discussed and breaks workflows that rely on the position

@WhalesState
Copy link
Contributor Author

Ok, i will just fix the path without making a separate folder for them.

@AThousandShips
Copy link
Member

The separate folder isn't the point for that part, you still are moving them to an entirely different path that's what breaks workflows

@AThousandShips AThousandShips requested a review from a team June 6, 2024 11:51
@AThousandShips
Copy link
Member

For context this was added in:

Where the placement was decided and the tooltip was added, so I suspect the choice of tooltip and path was intentional and the "Editor Data/Settings" part isn't a confusion over that, so I marked this back as an enhancement as it was never intended to work any other way from the start, so should be taken with more care IMO

@AThousandShips
Copy link
Member

AThousandShips commented Jun 6, 2024

The more I think about this the more I'm against this change, the current behavior makes it easy to handle screenshots from different projects, the main use of these screenshots are for tutorials, this way you can organize them easily if you're working on different tutorials at once, for example if you spend some time working on the example data etc. and you screenshot each step and you do all the stages (especially if you, for example, record it as a video as well) and then you write up and use the screenshots at the end of the day

Now with these changes the screenshots will be in one grouped folder with no indication which is from which project

I'd say the proper change here is to fix the tooltip, as it was confused in what it said, whereas the location for the screenshot was intended from the start, it also changes a specific detail that has been in the engine since before 4.0, and will now differ between this and past versions (this is IMO not a valid change to cherry pick)

So I'd suggest opening a proposal, and I'd suggest the following:

  • Make it configurable
  • Expose a screenshot path in the editor paths
  • Have it be a project specific setting but based on an editor setting by default
  • Keep the current default for this setting

For this PR I'd say to just fix the tooltip

@WhalesState
Copy link
Contributor Author

WhalesState commented Jun 6, 2024

At least we know that such confusion exists.

I have a question, shouldn't we considered this as deprecated in 4.x? since it's only capturing the main Editor window and not the whole screen. It can't include opened menus, or other sub-windows, and the shortcut doesn't work if another window is focused ie. popups.

@AThousandShips
Copy link
Member

I have a question, shouldn't we considered this as deprecated in 4.x? since it's only capturing the main Editor window and not the whole screen. It can't include opened menus, or other sub-windows, and the shortcut doesn't work if another window is focused ie. popups.

I'd say that's something to also discuss in a proposal

@AThousandShips AThousandShips changed the title Fix editor screenshots save path. Fix editor screenshot tooltip Jun 6, 2024
@AThousandShips AThousandShips added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jun 6, 2024
@AThousandShips AThousandShips modified the milestones: 4.x, 4.3 Jun 6, 2024
@WhalesState WhalesState changed the title Fix editor screenshot tooltip Fix editor screenshots menu button's tooltip Jun 6, 2024
@WhalesState WhalesState marked this pull request as ready for review June 6, 2024 13:54
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM but needs some editor people reviews as well for specifics

editor/editor_node.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

IMO the location where screenshots are saved is unexpected, so we should consider changing it. I'm not sure what's the best location but the project-specific user:// folder isn't it.

But for now it makes sense to document this better for 4.3, and evaluate changing it for a future release.

@WhalesState
Copy link
Contributor Author

WhalesState commented Jun 10, 2024

IMO, the best location is in editor data folder, we can create a new folder "Screenshots" and we save the screenshots there like ProjectName + "_screenshot_" + Time, this will make it easier to sort them.

@akien-mga akien-mga merged commit 5241d30 into godotengine:master Jun 10, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@WhalesState WhalesState deleted the screenshot branch June 10, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants