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

Add an Advanced Options toggle to the editor export preset #88419

Merged

Conversation

m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Feb 16, 2024

Similar to the Project Settings, add an Advanced Options toggle to the editor export presets to allow hiding advanced options and simplify the UI.

Follow up of the discussion with @dsnopek on #88297 (comment).
This PR sets up the infra and hides Android advanced options; other platform maintainers can leverage the same logic to hide options they deem advanced for their platform.

Screenshot_20240216_141141_Godot Editor 4 (debug)

Screen_Recording_20240216_141113_Godot.Editor.4.debug.mp4

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks, this looks awesome!

The code looks good - just two little comments. Seemed to work great in my testing too. :-)

return;
}
advanced_options_enabled = p_enabled;
EditorExport::singleton->save_presets();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to save the presets when this is changed?

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 followed the pattern used for runnable; from reading the code, we persist metadata about a preset everytime it's modified, so I did the same for this new field.

editor/export/editor_export.cpp Outdated Show resolved Hide resolved
@m4gr3d m4gr3d force-pushed the add_export_preset_advanced_options_toggle branch 2 times, most recently from c6135dc to 846652e Compare February 16, 2024 23:43
Copy link
Contributor

@dsnopek dsnopek 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 to me! I just retested it, and it's especially nice on top of the changes from PR #88297 because this exposes the new advanced Gradle template fields :-)

@Calinou
Copy link
Member

Calinou commented Feb 17, 2024

I think Runnable should be placed on top of Advanced Options, instead of the other way around. This way, Advanced Options will be closer to the view it affects (the list of export options).

@Mickeon
Copy link
Contributor

Mickeon commented Feb 17, 2024

It does peeve me that it occupies a whole new line instead of being alongside the "Runnable" toggle. I understand why that is, but it does look odd.

@m4gr3d m4gr3d force-pushed the add_export_preset_advanced_options_toggle branch 2 times, most recently from d87cd5d to 5c4f845 Compare February 18, 2024 18:35
@m4gr3d
Copy link
Contributor Author

m4gr3d commented Feb 18, 2024

I think Runnable should be placed on top of Advanced Options, instead of the other way around. This way, Advanced Options will be closer to the view it affects (the list of export options).

@Mickeon I played around with keeping them on the same line. Let me know what you think

Screenshot 2024-02-18 103638

@Mickeon
Copy link
Contributor

Mickeon commented Feb 18, 2024

I do like it myself, but it may break a few editor UX rules. @Calinou may know best. At worst it is reverted.

@Calinou
Copy link
Member

Calinou commented Feb 18, 2024

Putting them side-by-side is ideal to me 🙂

I was concerned you could confuse the two buttons together (since the checkbutton icon that appears on the left of Runnable could be confused to be for that setting). However, I'd say the different text color when the button is checked makes it unambiguous enough.

If there's a strong need, we can have an Separator node placed between the two buttons, but I think this is good to go as-is for now.

@m4gr3d m4gr3d force-pushed the add_export_preset_advanced_options_toggle branch from 5c4f845 to 6ef3154 Compare February 18, 2024 18:44
@akien-mga akien-mga merged commit ec0adfd into godotengine:master Feb 18, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@m4gr3d m4gr3d deleted the add_export_preset_advanced_options_toggle branch February 18, 2024 23:16
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.

5 participants