Userland: Port some applications to the GML compiler #25637
Conversation
github-actions
bot
added
the
👀 pr-needs-review
RatcheT2497
force-pushed
the
various-gml-compilation-ports
branch
2 times, most recently
from
06eb4fe to
1b593da
Compare
RatcheT2497
force-pushed
the
various-gml-compilation-ports
branch
from
1b593da to
0e87322
Compare
| @@ -174,11 +164,11 @@ void ThemesSettingsWidget::apply_settings() | |||
| auto color_scheme_path = color_scheme_path_or_error.release_value(); | |||
|
|
|||
| if (!m_color_scheme_is_file_based && find_descendant_of_type_named<GUI::CheckBox>("custom_color_scheme_checkbox")->is_checked()) { | |||
| auto set_theme_result = GUI::ConnectionToWindowServer::the().set_system_theme(m_selected_theme->path, m_selected_theme->name, m_background_settings_changed, "Custom"sv); | |||
There was a problem hiding this comment.
Why is m_background_settings_changed hardcoded to false now?
There was a problem hiding this comment.
I hardcoded it because it was always set to false at the very start of the method unconditionally, which (at least as far as I know) rendered it pretty much moot. It was a reference that BackgroundSettingsWidget set at various points and there currently doesn't seem to be a standardized way of passing variables to compiled widgets either, but it wouldn't make much of a difference.
EDIT: For what it's worth, I tested it a little after removing the forced reset and it didn't seem to do much of anything, so I'm not really sure what's it meant to do...
There was a problem hiding this comment.
Removing it seems fine compared to the current state of the code, but could you factor that change out into a separate commit ahead of time? If something breaks then whoever is trying to track down the bug will have an easy time determining whether it was caused by this change or by the GML conversion.
There was a problem hiding this comment.
cc @implicitfield, since the line forcing the flag to false was added in #17893 (and it looks like it can't be anything but accidental).
There was a problem hiding this comment.
I think that was just a rebase mishap; some of those changes there had been sitting around locally for quite some time before they were actually PR'd. m_background_settings_changed was originally set to false at the end of apply_settings, but apparently I ended up moving it to the top of that function instead.
There was a problem hiding this comment.
Would it still be fine to factor out the change into a separate commit and properly fix it later once there's GMLCompiler infrastructure for it?
There was a problem hiding this comment.
That is presumably fine, yes. It's currently broken anyway (and has been since March of 2023), so there should be no issue in delaying the investigation and proper fix until later.
There was a problem hiding this comment.
There is a slight problem - I don't think I can make the split commit atomic. The boolean that I'm removing weaves together the BackgroundSettingsWidget and ThemesSettingsWidget, right in the main file, causing a compilation error unless removed or fixed via adding some infrastructure to the GML compiler :/
There was a problem hiding this comment.
Shouldn't it work just fine if you remove the boolean everywhere in the very first commit?
| @@ -174,11 +164,11 @@ void ThemesSettingsWidget::apply_settings() | |||
| auto color_scheme_path = color_scheme_path_or_error.release_value(); | |||
|
|
|||
| if (!m_color_scheme_is_file_based && find_descendant_of_type_named<GUI::CheckBox>("custom_color_scheme_checkbox")->is_checked()) { | |||
| auto set_theme_result = GUI::ConnectionToWindowServer::the().set_system_theme(m_selected_theme->path, m_selected_theme->name, m_background_settings_changed, "Custom"sv); | |||
There was a problem hiding this comment.
Removing it seems fine compared to the current state of the code, but could you factor that change out into a separate commit ahead of time? If something breaks then whoever is trying to track down the bug will have an easy time determining whether it was caused by this change or by the GML conversion.
| @@ -174,11 +164,11 @@ void ThemesSettingsWidget::apply_settings() | |||
| auto color_scheme_path = color_scheme_path_or_error.release_value(); | |||
|
|
|||
| if (!m_color_scheme_is_file_based && find_descendant_of_type_named<GUI::CheckBox>("custom_color_scheme_checkbox")->is_checked()) { | |||
| auto set_theme_result = GUI::ConnectionToWindowServer::the().set_system_theme(m_selected_theme->path, m_selected_theme->name, m_background_settings_changed, "Custom"sv); | |||
There was a problem hiding this comment.
cc @implicitfield, since the line forcing the flag to false was added in #17893 (and it looks like it can't be anything but accidental).
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
|
This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions! |
github-actions
bot
removed
the
👀 pr-needs-review
This PR contributes towards #20518
The ported applications are: ThemeEditor, DisplaySettings and ClockSettings.