fix: honor fontFamily, remove unused primaryColor and fontSize#11
fix: honor fontFamily, remove unused primaryColor and fontSize#11johnkferguson wants to merge 1 commit into
Conversation
- Drop primaryColor and fontSize from theme.conf, flake.nix, README. Both were declared but never read by any QML; setting them was a no-op. - Wire config.fontFamily into Main.qml via two derived properties with fallback to the bundled FontLoaders. - Set the default fontFamily to "Google Sans Flex Freeze" (the actual family name embedded in assets/fonts/FlexRounded-*.ttf).
johnkferguson
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic font family selection by adding properties that fall back to default fonts if no configuration is provided. It also cleans up the configuration by removing unused properties like primaryColor and fontSize from theme.conf, flake.nix, and the documentation. The review feedback suggests consolidating the redundant activeFontRegular and activeFontBold properties into a single activeFontFamily property to simplify the logic and improve maintainability, as the underlying font family is the same for both weights.
| property string activeFontRegular: (config.fontFamily && config.fontFamily.length > 0) ? config.fontFamily : fontRegular.name | ||
| property string activeFontBold: (config.fontFamily && config.fontFamily.length > 0) ? config.fontFamily : fontBold.name |
There was a problem hiding this comment.
The properties activeFontRegular and activeFontBold are redundant because the bundled fonts share the same family name (Google Sans Flex Freeze). You can consolidate them into a single property, activeFontFamily, and simplify the logic using the logical OR operator. This improves maintainability and reduces code duplication.
property string activeFontFamily: config.fontFamily || fontRegular.name
| color: container.extractedAccent | ||
| font.pixelSize: 22 | ||
| font.family: fontRegular.name | ||
| font.family: activeFontRegular |
There was a problem hiding this comment.
Update to use the consolidated activeFontFamily property.
font.family: activeFontFamily
| backgroundSource: config.background | ||
| baseAccent: container.extractedAccent | ||
| fontFamily: fontRegular.name | ||
| fontFamily: container.activeFontRegular |
There was a problem hiding this comment.
Update to use the consolidated activeFontFamily property. Also, the container. prefix is unnecessary here as the property is visible in the scope.
fontFamily: activeFontFamily
| color: container.extractedAccent | ||
| font.pixelSize: 48 | ||
| font.family: fontBold.name | ||
| font.family: activeFontBold |
There was a problem hiding this comment.
Update to use the consolidated activeFontFamily property. Qt will automatically select the bold weight of the family because font.weight: Font.Bold is set.
font.family: activeFontFamily
| font.pixelSize: 24 | ||
| font.weight: Font.Bold | ||
| font.family: fontRegular.name | ||
| font.family: activeFontRegular |
There was a problem hiding this comment.
Update to use the consolidated activeFontFamily property.
font.family: activeFontFamily
| color: container.extractedAccent | ||
| font.pixelSize: 14 | ||
| font.family: fontRegular.name | ||
| font.family: activeFontRegular |
There was a problem hiding this comment.
Update to use the consolidated activeFontFamily property.
font.family: activeFontFamily
| color: isCurrent ? baseColor : "white" | ||
| font.pixelSize: 12 | ||
| font.family: fontBold.name | ||
| font.family: activeFontBold |
There was a problem hiding this comment.
Update to use the consolidated activeFontFamily property. Qt will automatically select the bold weight of the family because font.weight: Font.Bold is set.
font.family: activeFontFamily
| color: isCurrent ? "white" : (hovered ? "#DDDDDD" : "#AAAAAA") | ||
| font.pixelSize: 15 | ||
| font.family: fontRegular.name | ||
| font.family: activeFontRegular |
There was a problem hiding this comment.
Update to use the consolidated activeFontFamily property.
font.family: activeFontFamily
| color: isCurrent ? "white" : "#AAAAAA" | ||
| font.pixelSize: 14 | ||
| font.family: fontRegular.name | ||
| font.family: activeFontRegular |
There was a problem hiding this comment.
Update to use the consolidated activeFontFamily property.
font.family: activeFontFamily
|
I was wondering why my font family wasn't applying when using the flake, but this fixed the issue. There are some spots where the font isn't applied, though (e.g., the "Press any key to unlock" text at the bottom). On a side note, different fonts sometimes cause the clock hours/minutes to overlap. I think the culprit is the Clock.qml component which has its Column spacing hardcoded to -130. Works fine with the default font, since I think it already has large spacing. With JetBrains Mono With Arial (easier to see overlap) I played around with adding a fontSpacing option (perhaps not the best name...), which works so you can set a custom spacing while keeping the default -130 if not defined (andreshungbz@4cf3334). Waiting first to see if this PR is merged, though. |
2 participants
Noticed while customizing the theme that
theme.confdeclaresprimaryColor,fontFamily, andfontSize, and the flake accepts all three as overrides — but none of them are actually read by any QML. Setting them does nothing.This PR:
primaryColorandfontSize. They're not referenced anywhere in the QML, so they were silent no-ops.config.fontFamilyintoMain.qmlvia two derived properties that fall back to the bundled FontLoaders when unset.fontFamilytoGoogle Sans Flex Freeze— the actual family name embedded inassets/fonts/FlexRounded-*.ttf(verified withfc-query). The previous defaultFlexRoundedwas a string Qt couldn't resolve, but it didn't matter since the value was never read.Small breaking change for the flake API: passing
primaryColororfontSizeto.override { ... }now raises a Nix eval error instead of being silently ignored. No rendered behavior changes either way.