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 SettingsPopover Class #699

Merged
merged 2 commits into from
Jul 25, 2023
Merged

Add SettingsPopover Class #699

merged 2 commits into from
Jul 25, 2023

Conversation

Marukesu
Copy link
Contributor

move the settings menu popover, and related functions to they own class.

the settings object is now, also used to keep the state between the widgets synchronized, and the actions provided by it make the new class mostly self-contained, only needing a single signal to indicate the window that the theme dialog should be shown.

@jeremypw
Copy link
Collaborator

@Marukesu This seems like a good idea especially as MainWindow.vala is larger than I would like. Could you fix the conflicts please?

@Marukesu Marukesu force-pushed the maru/settings-popover branch 2 times, most recently from 261f28c to 8d8821d Compare June 25, 2023 17:36
@Marukesu
Copy link
Contributor Author

@jeremypw, sorry for the delay in rebasing this, should be good to review.

don't know what caused the CI error after the rebase, but with the --print-errorlogs option, it should be more easy to know what test case failed in the future.

halign = Gtk.Align.CENTER
};

if (css_provider == null) {
Copy link
Collaborator

@jeremypw jeremypw Jun 26, 2023

Choose a reason for hiding this comment

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

This gives two new compile warnings with valac 0.56.7

../src/Widgets/SettingsPopover.vala:180.13-180.32: warning: Use of possibly unassigned parameter `css_provider'
  180 |         if (css_provider == null) {
      |             ^~~~~~~~~~~~~~~~~~~~   
../src/Widgets/SettingsPopover.vala:186.9-186.90: warning: Use of possibly unassigned parameter `css_provider'
  186 |         style_context.add_provider (css_provider, Gtk.STYLE_PROVIDER_PRIORITY_APPLICATION);

Do you know if later versions of Vala will support it? I tend to avoid this pattern though it is useful.

@jeremypw
Copy link
Collaborator

Despite the coding technicalities there do not seem to be any regressions in operation.

Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

Found a regression: Changes in font size are not restored after closing and reopening the app.

@jeremypw
Copy link
Collaborator

@Marukesu Another regression with this is that the font scale is not set individually on each tab.

@Marukesu
Copy link
Contributor Author

I'm tempted to revert the ActionGroup change for now, the widget won't be as self-contained as i would like, but would still make this a good start in reducing the MainWindow scope.

@jeremypw
Copy link
Collaborator

Yes two smaller PRs would be easier to review.

move the settings menu popover, and related functions to they own class.
the new class is mostly self-contained, only needing a signal to ask the
window to show the theme dialog should be shown.

Signed-off-by: Gustavo Marques <[email protected]>
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

Code looks good, no regressions or new terminal warnings seen.

@jeremypw jeremypw merged commit 0f6c1cd into master Jul 25, 2023
4 checks passed
@jeremypw jeremypw deleted the maru/settings-popover branch July 25, 2023 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants