-
-
Notifications
You must be signed in to change notification settings - Fork 102
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
Conversation
@Marukesu This seems like a good idea especially as |
261f28c
to
8d8821d
Compare
@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 |
src/Widgets/SettingsPopover.vala
Outdated
halign = Gtk.Align.CENTER | ||
}; | ||
|
||
if (css_provider == null) { |
There was a problem hiding this comment.
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.
Despite the coding technicalities there do not seem to be any regressions in operation. |
There was a problem hiding this 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.
@Marukesu Another regression with this is that the font scale is not set individually on each tab. |
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. |
Yes two smaller PRs would be easier to review. |
f9f76be
to
695150b
Compare
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]>
Signed-off-by: Gustavo Marques <[email protected]>
695150b
to
02a34fe
Compare
There was a problem hiding this 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.
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.