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.

Sign up for GitHub

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

Jump to bottom

Consolidate remembering window settings into single config #97333

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Consolidate remembering window settings into single config #97333

merged 1 commit into from
Oct 1, 2024

Conversation

bits-by-brandon
Copy link
Contributor

@bits-by-brandon bits-by-brandon commented Sep 22, 2024 •
edited
Loading

Fixes #97051 by consolidating the remember_window_size_and_position editor config into a new Auto option in interface/editor/editor_screen.

Screenshot 2024-09-23 at 11 14 46 AM

Tested locally on MacOS build

@AThousandShips AThousandShips changed the title Consolidates remembering window settings into single config Consolidate remembering window settings into single config Sep 22, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Sep 22, 2024
bits-by-brandon
bits-by-brandon commented Sep 22, 2024
View reviewed changes
main/main.cpp Outdated
@@ -4051,7 +4056,7 @@ int Main::start() {
sml->get_root()->set_embedding_subwindows(true);
}
restore_editor_window_layout = EditorSettings::get_singleton()->get_setting(
"interface/editor/remember_window_size_and_position");
"interface/editor/editor_screen") == -5;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I compare this variant to this signed int?

Copy link
Member

Choose a reason for hiding this comment

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

You cast it to an int, like int(EditorSettings::get_singleton()->get_setting("interface/editor/editor_screen")) or EditorSettings::get_singleton()->get_setting("interface/editor/editor_screen").operator int()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a total beginner with c++, is there a convention of using the raw values over something like an enum here?

Copy link
Member

Choose a reason for hiding this comment

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

You should use the enum to make it safer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I found the enums! lmk if i'm doing this right

@bits-by-brandon bits-by-brandon mentioned this pull request Sep 22, 2024
Closed
@bits-by-brandon bits-by-brandon marked this pull request as ready for review September 22, 2024 19:00
@bits-by-brandon bits-by-brandon requested review from a team as code owners September 22, 2024 19:00
Mickeon
Mickeon reviewed Sep 22, 2024
View reviewed changes
doc/classes/EditorSettings.xml Outdated Show resolved Hide resolved
doc/classes/EditorSettings.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon requested a review from KoBeWi September 22, 2024 20:27
@bits-by-brandon
Copy link
Contributor Author

I'm not sure what's causing the build fail GHA / 🐧 Linux / Editor w/ Mono (target=editor)

@bits-by-brandon bits-by-brandon requested a review from a team as a code owner September 23, 2024 12:55
@KoBeWi
Copy link
Member

KoBeWi commented Sep 23, 2024

The project manager's Auto option does not work. The window spawns at 0, 0 and moves to screen center, always on screen 1.

KoBeWi
KoBeWi reviewed Sep 23, 2024
View reviewed changes
servers/display_server.h Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Sep 23, 2024
View reviewed changes
main/main.cpp Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Sep 23, 2024
View reviewed changes
main/main.cpp Outdated Show resolved Hide resolved
@bits-by-brandon
Copy link
Contributor Author

bits-by-brandon commented Sep 23, 2024 •
edited
Loading

@KoBeWi

The project manager's Auto option does not work. The window spawns at 0, 0 and moves to screen center, always on screen 1.

Refactored to only apply settings in the editor and not to project manager, which was never implemented.
Also creates a separate enum to capture valid values of the editor config, which mirrors screens but also has the new possible "Auto" value

@bits-by-brandon
Copy link
Contributor Author

Any other considerations for this PR?

@adamscott
Copy link
Member

Any other considerations for this PR?

cc. @KoBeWi

KoBeWi
KoBeWi reviewed Sep 27, 2024
View reviewed changes
servers/display_server.h Outdated Show resolved Hide resolved
KoBeWi
KoBeWi reviewed Sep 27, 2024
View reviewed changes
doc/classes/EditorSettings.xml Outdated
@@ -772,7 +772,7 @@
Translations are provided by the community. If you spot a mistake, [url=$DOCS_URL/contributing/documentation/editor_and_docs_localization.html]contribute to editor translations on Weblate![/url]
</member>
<member name="interface/editor/editor_screen" type="int" setter="" getter="">
The preferred monitor to display the editor.
The preferred monitor to display the editor. If set to [b]Auto[/b], the editor will remember the last screen it was displayed on across restarts.
Copy link
Member

@KoBeWi KoBeWi Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
The preferred monitor to display the editor. If set to [b]Auto[/b], the editor will remember the last screen it was displayed on across restarts.
The preferred monitor to display the editor. If [b]Auto[/b], the editor will remember the last screen it was displayed on across restarts.

Also you could mention position and size, like in the deleted setting below.

KoBeWi
KoBeWi approved these changes Sep 27, 2024
View reviewed changes
Copy link
Member

@KoBeWi KoBeWi 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, though I think the new enum shouldn't be in DisplayServer, or at least be wrapped in TOOLS_ENABLED. It's editor-only.

@bits-by-brandon bits-by-brandon requested a review from a team as a code owner September 29, 2024 13:49
AThousandShips
AThousandShips reviewed Sep 29, 2024
View reviewed changes
editor/editor_settings.cpp Outdated
@@ -426,12 +426,17 @@ void EditorSettings::_load_defaults(Ref<ConfigFile> p_extra_config) {
EDITOR_SETTING_USAGE(Variant::INT, PROPERTY_HINT_ENUM, "interface/editor/display_scale", 0, display_scale_hint_string, PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_RESTART_IF_CHANGED | PROPERTY_USAGE_EDITOR_BASIC_SETTING)
EDITOR_SETTING_USAGE(Variant::FLOAT, PROPERTY_HINT_RANGE, "interface/editor/custom_display_scale", 1.0, "0.5,3,0.01", PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_RESTART_IF_CHANGED | PROPERTY_USAGE_EDITOR_BASIC_SETTING)

String ed_screen_hints = "Screen With Mouse Pointer:-4,Screen With Keyboard Focus:-3,Primary Screen:-2"; // Note: Main Window Screen:-1 is not used for the main window.
String ed_screen_hints = "Auto (Remembers last position):-4,Screen With Mouse Pointer:-3,Screen With Keyboard Focus:-2,Primary Screen:-1";
Copy link
Member

Choose a reason for hiding this comment

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

This will make existing settings break as if people picked Screen With Keyboard Focus they'll now have picked Screen With Mouse Pointer, any reason you can't keep those?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good point

@bits-by-brandon
Copy link
Contributor Author

@AThousandShips @KoBeWi Made changes!

@Repiteo Repiteo modified the milestones: 4.x, 4.4 Sep 30, 2024
@akien-mga akien-mga force-pushed the consolidate-window-remember-editor-settings branch from ad87f26 to e0957c2 Compare October 1, 2024 14:35
@akien-mga
Copy link
Member

I pushed an update to squash the commits together, as per our preferred PR workflow.

@akien-mga akien-mga merged commit 7d4e06e into godotengine:master Oct 1, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Geometror Geometror mentioned this pull request Oct 4, 2024
Open
@akien-mga akien-mga mentioned this pull request Jan 8, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips approved these changes

@KoBeWi KoBeWi KoBeWi approved these changes

@Mickeon Mickeon Awaiting requested review from Mickeon

Assignees
No one assigned
Labels
bug enhancement topic:editor
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

interface/editor/editor_screen setting has no effect once window position is remembered
7 participants
@bits-by-brandon @KoBeWi @adamscott @akien-mga @Mickeon @AThousandShips @Repiteo