broulik added a comment.
There's also a lot of code duplication making the diff hard to read. Please
group if statements together where it makes sense, e.g. avpod things like
if (foo) {
return plasmoid.title;
} else if (foo) {
return plasmoid.title;
or
if (foo) {
if (bar) {
baz();
}
} else {
if (bar) {
baz();
}
}
INLINE COMMENTS
> batterymonitor.qml:85
> + if (powermanagementDisabled) {
> + return i18n("Power management is disabled")
> + }
When power management is disabled the subtext shows no useful information now.
Powermanagement disabled means disabled screen powermanagement (i.e. keep
screen always on), has nothing to do with the battery percentage, which is
still useful to have.
> batterymonitor.qml:95
> if (remainingTime > 0) {
> - return i18nc("%1 is remaining time, %2 is percentage", "%1
> Remaining (%2%)",
> -
> KCoreAddons.Format.formatDuration(remainingTime,
> KCoreAddons.FormatTypes.HideSeconds),
> - pmSource.data["Battery"]["Percent"])
> - } else {
> - return i18n("%1% Battery Remaining",
> pmSource.data["Battery"]["Percent"]);
> + return i18n("%1 until fully charged",
> KCoreAddons.Format.formatDuration(remainingTime,
> KCoreAddons.FormatTypes.HideSeconds))
> + }
Keep the `i18nc` explaining that %1 is remaining time
> batterymonitor.qml:102
> }
> }
> +
What in the `else` case? Perhaps at least explicitly `return "";`
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D24070
To: mthw, ngraham, #vdg, #plasma, broulik, ndavis
Cc: ndavis, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh,
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
