[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request 118272: batterymonitor visual cleanups
From: "Kai Uwe Broulik" <kde () privat ! broulik ! de>
Date: 2014-06-02 8:59:11
Message-ID: 20140602085911.24281.95028 () probe ! kde ! org
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118272/#review58934
-----------------------------------------------------------
There's been a bug report about inconsistent indicator/slider lengths: Bug 335540
- Kai Uwe Broulik
On May 23, 2014, 7:21 p.m., Sebastian Kügler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118272/
> -----------------------------------------------------------
>
> (Updated May 23, 2014, 7:21 p.m.)
>
>
> Review request for Plasma and Kai Uwe Broulik.
>
>
> Repository: plasma-workspace
>
>
> Description
> -------
>
> Clean up Battery Monitor
>
> This patch simplifies the battery's ui, and brings spacing in line with
> Plasma 5 standards.
>
> In detail:
>
> - removing the selection item on batteries: it's already visible by the
> text shown that the battery is selected. It also doesn't need the
> selection semantics across the ui, because -- for what?
> - remove separator lines, in Plasma 5, we use spacing for this case
> - align extra battery info to the slider, improves logical grouping
> - Improve HighDPI by removing hard-coded layout hints, use units.gridUnit
> throughout
> - Fix batteryitem's status: it would show the wrong icon and time label,
> because AC Adapter can be empty. Checking charging is semantically
> correct here, since it uses the charge state, not the adapter state.
> - remove a bunch of SVGs that were used internally to get margins -- use
> gridUnit for layout internal margins instead
> - fix slider's right alignment, this would jump based on the percentage
> label's width, which varies per item. We know the rough length of the
> percentage label from the context, and can align the labels to that.
>
> Code is also in kde-workspace[sebas/batterycleanup].
>
>
> Diffs
> -----
>
> applets/batterymonitor/contents/ui/BatteryItem.qml 431aa9a
> applets/batterymonitor/contents/ui/BrightnessItem.qml a91ccde
> applets/batterymonitor/contents/ui/CompactRepresentation.qml 8d7b26e
> applets/batterymonitor/contents/ui/PopupDialog.qml 0444199
> applets/batterymonitor/contents/ui/PowerManagementItem.qml 530fce3
> applets/batterymonitor/contents/ui/batterymonitor.qml 7536f95
>
> Diff: https://git.reviewboard.kde.org/r/118272/diff/
>
>
> Testing
> -------
>
> Tested with plasmoidviewer and in the shell, plugging ac in and out, tried \
> different combinations of values.
>
> File Attachments
> ----------------
>
> before (collapsed)
> https://git.reviewboard.kde.org/media/uploaded/files/2014/05/22/abc1d750-6eb7-4728-a5f2-e9f8566b5b44__battery-before-collapsed.png
> before (expanded)
> https://git.reviewboard.kde.org/media/uploaded/files/2014/05/22/2a0b9ad3-03e8-4f07-a158-ceb0d3d0e23c__battery-before-expanded.png
> after (collapsed)
> https://git.reviewboard.kde.org/media/uploaded/files/2014/05/22/4439093c-345c-44af-afa1-a05cb0973990__battery-after-collapsed.png
> after (expanded)
> https://git.reviewboard.kde.org/media/uploaded/files/2014/05/22/b18cf61f-0ff1-4e91-93c2-ebd6b1dab38d__battery-after-expanded.png
>
>
> Thanks,
>
> Sebastian Kügler
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/118272/">https://git.reviewboard.kde.org/r/118272/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">There's been a bug \
report about inconsistent indicator/slider lengths: Bug 335540</pre> <br />
<p>- Kai Uwe Broulik</p>
<br />
<p>On May 23rd, 2014, 7:21 p.m. UTC, Sebastian Kügler wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Plasma and Kai Uwe Broulik.</div>
<div>By Sebastian Kügler.</div>
<p style="color: grey;"><i>Updated May 23, 2014, 7:21 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-workspace
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Clean up Battery Monitor
This patch simplifies the battery's ui, and brings spacing in line with
Plasma 5 standards.
In detail:
- removing the selection item on batteries: it's already visible by the
text shown that the battery is selected. It also doesn't need the
selection semantics across the ui, because -- for what?
- remove separator lines, in Plasma 5, we use spacing for this case
- align extra battery info to the slider, improves logical grouping
- Improve HighDPI by removing hard-coded layout hints, use units.gridUnit
throughout
- Fix batteryitem's status: it would show the wrong icon and time label,
because AC Adapter can be empty. Checking charging is semantically
correct here, since it uses the charge state, not the adapter state.
- remove a bunch of SVGs that were used internally to get margins -- use
gridUnit for layout internal margins instead
- fix slider's right alignment, this would jump based on the percentage
label's width, which varies per item. We know the rough length of the
percentage label from the context, and can align the labels to that.
Code is also in kde-workspace[sebas/batterycleanup].</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Tested with plasmoidviewer and in the shell, plugging ac in and out, \
tried different combinations of values.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>applets/batterymonitor/contents/ui/BatteryItem.qml <span style="color: \
grey">(431aa9a)</span></li>
<li>applets/batterymonitor/contents/ui/BrightnessItem.qml <span style="color: \
grey">(a91ccde)</span></li>
<li>applets/batterymonitor/contents/ui/CompactRepresentation.qml <span style="color: \
grey">(8d7b26e)</span></li>
<li>applets/batterymonitor/contents/ui/PopupDialog.qml <span style="color: \
grey">(0444199)</span></li>
<li>applets/batterymonitor/contents/ui/PowerManagementItem.qml <span style="color: \
grey">(530fce3)</span></li>
<li>applets/batterymonitor/contents/ui/batterymonitor.qml <span style="color: \
grey">(7536f95)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118272/diff/" style="margin-left: \
3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments \
</h1>
<ul>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/05/22/abc1d750-6eb7-4728-a5f2-e9f8566b5b44__battery-before-collapsed.png">before \
(collapsed)</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/05/22/2a0b9ad3-03e8-4f07-a158-ceb0d3d0e23c__battery-before-expanded.png">before \
(expanded)</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/05/22/4439093c-345c-44af-afa1-a05cb0973990__battery-after-collapsed.png">after \
(collapsed)</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/05/22/b18cf61f-0ff1-4e91-93c2-ebd6b1dab38d__battery-after-expanded.png">after \
(expanded)</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic