Skip to content

feat(settings): improve battery status layout - #3103

Open
hthienloc wants to merge 7 commits into
AvengeMedia:masterfrom
hthienloc:feat/battery-status-layout
Open

feat(settings): improve battery status layout#3103
hthienloc wants to merge 7 commits into
AvengeMedia:masterfrom
hthienloc:feat/battery-status-layout

Conversation

@hthienloc

@hthienloc hthienloc commented Aug 18, 2026

Copy link
Copy Markdown
Member

Type of change

  • Refactor / internal cleanup

Screenshots / video

dms_capture_1787184832770 dms_capture_1787184923595

@hthienloc
hthienloc marked this pull request as draft August 18, 2026 02:36
@hthienloc

hthienloc commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

@bbedward @purian23 My initial intention is to redesign the battery info tab in the settings, but there is a question I want to clarify before going any further:

Should we repeat the information that's already available in the battery widget?

Because I'd like to remove the duplicate information and replace it with something more useful, like battery history, so users can track their battery level over an extended period of time. But the edge case is some user might not use the battery widget so this is still holding some use case.

@purian23

Copy link
Copy Markdown
Collaborator

Hey @hthienloc, I'm fairly open on this one with no strong opinions either way. It should repeat maybe the basics of info, but otherwise open to the options you listed for sure. Thanks!

@hthienloc
hthienloc marked this pull request as ready for review August 20, 2026 00:17
@hthienloc

Copy link
Copy Markdown
Member Author

/claude review

spacing: Theme.spacingXXS

StyledText {
text: I18n.tr("Estimated time")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These re-cased/reworded labels create brand-new untranslated terms and orphan already-translated ones. quickshell/translations/en.json has Estimated Time (line 8809), Battery Health (3262) and AC Adapter (Plugged In) (964), each referenced only from this file — so this PR drops every existing locale's translation for them in exchange for identical strings that differ only in casing/wording. CONTRIBUTING asks to reuse existing terms.

Please restore the exact existing terms:

  • line 147: I18n.tr("Estimated Time")
  • line 177: I18n.tr("Battery Health")
  • line 105: I18n.tr("AC Adapter (Plugged In)")
Suggested change
text: I18n.tr("Estimated time")
text: I18n.tr("Estimated Time")

width: parent.width
iconName: "battery_charging_full"
title: I18n.tr("Status")
settingKey: "batteryStatusCard"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Dropping title: here breaks the settings-search label for this card. extract_settings_index.py falls back to the block's first text: when there's no title (find_settings_components, lines 368-377), so it picked up the no-battery fallback string: the regenerated index now lists batteryStatusCard as "Power" (and with no icon) instead of "Status". Users searching settings will see the battery card titled "Power" under the "Power & Security" tab, which reads as a different setting.

If the header-less card is intentional, keep the searchable title but hide the header — or at minimum reorder so the first text: in the block is a meaningful label rather than the !batteryAvailable fallback.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude review

Layout change looks sound; two i18n/search-index issues worth fixing before merge.

  • New term variants orphan existing translations (Estimated time/Battery health/Plugged In (AC) vs already-translated Estimated Time/Battery Health/AC Adapter (Plugged In)), quickshell/Modules/Settings/BatteryTab.qml:147
  • Removing title: makes the settings-search index label the card "Power" (the no-battery fallback string) instead of "Status", quickshell/Modules/Settings/BatteryTab.qml:58

Checked: effective diff is BatteryTab.qml + settings_search_index.json (the BatteryService.qml commits are already on master); verified Theme.withAlpha/surfaceTextMedium/spacingXXS/fontSizeXLarge, DankIcon.implicitWidth == size, BatteryService.isLowBattery/batteryAvailable, no binding loops in the new Row/Column nesting, and Math.max(0, parent.width - 100) stays within bounds at the 150% max font scale. Model: claude-opus-5.

@bbedward

Copy link
Copy Markdown
Collaborator

/claude review

Comment on lines +88 to +95
text: BatteryService.batteryAvailable ? `${BatteryService.batteryLevel}%` : I18n.tr("Power")
font.pixelSize: Theme.fontSizeXLarge
font.weight: Font.Bold
color: root.batteryStatusColor
}

StyledText {
text: BatteryService.batteryAvailable ? BatteryService.batteryStatus : I18n.tr("Management")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The no-battery fallback splits the phrase "Power Management" across two separate I18n.tr() calls ("Power" on line 88, "Management" on line 95), rendered side by side in the same Row. Those are two unrelated existing terms in en.json (a nav label and a generic noun), and word order + wording can't be fixed by a translator, so localized builds get things like "Energía Gestión". Sentence fragments shouldn't be assembled from separate terms.

BatteryService.batteryStatus already returns a properly translated "No battery" for this case, so the simplest fix is to drop the fragment and let the existing status string carry it — e.g. keep the icon/percentage slot empty (or use one whole new term) and use BatteryService.batteryStatus unconditionally on line 95.

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Claude review

Both findings from the previous round are fixed; one new i18n issue.

  • No-battery fallback splits "Power Management" into two separate I18n.tr() fragments that translators cannot reorder, quickshell/Modules/Settings/BatteryTab.qml:88-95

Checked: previous round resolved — title: I18n.tr("Status") is restored (search index entry is back to label "Status", and SettingsSearchService falls back to item.icon || "settings" so the dropped iconName is harmless), and the labels now reuse the existing Estimated Time / Battery Health / AC Adapter (Plugged In) terms. Also verified formatEstimatedTime/getBatteryIcon/isLowBattery/batteryAvailable exist on BatteryService and their property reads are captured by the bindings; Theme.withAlpha/surfaceTextMedium/spacingXXS/fontSizeXLarge/iconSizeLarge all resolve; DankIcon.implicitWidth == Math.round(size) so the icon-column width subtraction is exact; no binding loops in the new Row/Column nesting (every width traces back to an explicit one); the no-battery path is reachable (Battery tab is not gated on batteryAvailable) and batteryHealth/formatTimeRemaining return non-empty fallbacks there; Math.max(0, parent.width - 100) stays in bounds at the 150% font scale. Model: claude-opus-5.

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.

3 participants