DevHeads.net

Review Request 127102: Use fixed width for digital clock applet

Review request for kde-workspace and Plasma.

Repository: plasma-workspace

Description
Currently the width of the date label is not fixed but changes depending on the text. This causes the entire applet to change its width (if the time is the widest displayed item). This in turn can cause all other applets in the same panel to move whenever the displayed time changes.

This patch uses FontMetrics to iterate over all possible time strings (with different width) and chooses the widest of them as reference for the fixed width of the time label.

This way the width of the applet stays the same (unless the date is displayed and changes). The text remains centered though, which means that it can still move within the applet when the time changes.

Diffs
applets/digital-clock/package/contents/ui/DigitalClock.qml 95bb071

Diff: <a href="https://git.reviewboard.kde.org/r/127102/diff/" title="https://git.reviewboard.kde.org/r/127102/diff/">https://git.reviewboard.kde.org/r/127102/diff/</a>

Testing
Works with horizontal and vertical panel.
Also displaying different combinations of "seconds", "date" and "timezone" works.

Thanks,

Daniel Faust

Comments

Re: Review Request 127102: Use fixed width for digital clock app

By Daniel Faust at 02/19/2016 - 12:11

(Updated Feb. 19, 2016, 6:11 nachm.)

Review request for kde-workspace and Plasma.

Bugs: 347724
<a href="https://bugs.kde.org/show_bug.cgi?id=347724" title="https://bugs.kde.org/show_bug.cgi?id=347724">https://bugs.kde.org/show_bug.cgi?id=347724</a>

Repository: plasma-workspace

Description
Currently the width of the date label is not fixed but changes depending on the text. This causes the entire applet to change its width (if the time is the widest displayed item). This in turn can cause all other applets in the same panel to move whenever the displayed time changes.

This patch uses FontMetrics to iterate over all possible time strings (with different width) and chooses the widest of them as reference for the fixed width of the time label.

This way the width of the applet stays the same (unless the date is displayed and changes). The text remains centered though, which means that it can still move within the applet when the time changes.

Diffs (updated)
applets/digital-clock/package/contents/ui/DigitalClock.qml 95bb071

Diff: <a href="https://git.reviewboard.kde.org/r/127102/diff/" title="https://git.reviewboard.kde.org/r/127102/diff/">https://git.reviewboard.kde.org/r/127102/diff/</a>

Testing
Works with horizontal and vertical panel.
Also displaying different combinations of "seconds", "date" and "timezone" works.

Thanks,

Daniel Faust

Re: Review Request 127102: Use fixed width for digital clock app

By Daniel Faust at 03/08/2016 - 12:53

(Updated März 8, 2016, 6:53 nachm.)

Review request for kde-workspace and Plasma.

Changes
Fix width calculation if zero is the widest character

Bugs: 347724
<a href="https://bugs.kde.org/show_bug.cgi?id=347724" title="https://bugs.kde.org/show_bug.cgi?id=347724">https://bugs.kde.org/show_bug.cgi?id=347724</a>

Repository: plasma-workspace

Description
Currently the width of the date label is not fixed but changes depending on the text. This causes the entire applet to change its width (if the time is the widest displayed item). This in turn can cause all other applets in the same panel to move whenever the displayed time changes.

This patch uses FontMetrics to iterate over all possible time strings (with different width) and chooses the widest of them as reference for the fixed width of the time label.

This way the width of the applet stays the same (unless the date is displayed and changes). The text remains centered though, which means that it can still move within the applet when the time changes.

Diffs (updated)
applets/digital-clock/package/contents/ui/DigitalClock.qml 95bb071

Diff: <a href="https://git.reviewboard.kde.org/r/127102/diff/" title="https://git.reviewboard.kde.org/r/127102/diff/">https://git.reviewboard.kde.org/r/127102/diff/</a>

Testing
Works with horizontal and vertical panel.
Also displaying different combinations of "seconds", "date" and "timezone" works.

Thanks,

Daniel Faust

Re: Review Request 127102: Use fixed width for digital clock app

By Daniel Faust at 03/16/2016 - 10:35

(Updated März 16, 2016, 4:35 nachm.)

Review request for kde-workspace and Plasma.

Changes
Use single Date object

Bugs: 347724
<a href="https://bugs.kde.org/show_bug.cgi?id=347724" title="https://bugs.kde.org/show_bug.cgi?id=347724">https://bugs.kde.org/show_bug.cgi?id=347724</a>

Repository: plasma-workspace

Description
Currently the width of the date label is not fixed but changes depending on the text. This causes the entire applet to change its width (if the time is the widest displayed item). This in turn can cause all other applets in the same panel to move whenever the displayed time changes.

This patch uses FontMetrics to iterate over all possible time strings (with different width) and chooses the widest of them as reference for the fixed width of the time label.

This way the width of the applet stays the same (unless the date is displayed and changes). The text remains centered though, which means that it can still move within the applet when the time changes.

Diffs (updated)
applets/digital-clock/package/contents/ui/DigitalClock.qml 95bb071

Diff: <a href="https://git.reviewboard.kde.org/r/127102/diff/" title="https://git.reviewboard.kde.org/r/127102/diff/">https://git.reviewboard.kde.org/r/127102/diff/</a>

Testing
Works with horizontal and vertical panel.
Also displaying different combinations of "seconds", "date" and "timezone" works.

File Attachments (updated)
0001-Use-fixed-width-for-digital-clock-applet.patch
<a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/03/16/81b4a902-1454-4155-9fda-552b8acba1a8__0001-Use-fixed-width-for-digital-clock-applet.patch" title="https://git.reviewboard.kde.org/media/uploaded/files/2016/03/16/81b4a902-1454-4155-9fda-552b8acba1a8__0001-Use-fixed-width-for-digital-clock-applet.patch">https://git.reviewboard.kde.org/media/uploaded/files/2016/03/16/81b4a902...</a>

Thanks,

Daniel Faust

Re: Review Request 127102: Use fixed width for digital clock app

By Daniel Faust at 03/16/2016 - 10:56

(Updated March 16, 2016, 3:56 p.m.)

Status
This change has been marked as submitted.

Review request for kde-workspace and Plasma.

Changes
Submitted with commit e7f09ba1eb976c4f282145016d34fe87de515a25 by Daniel Faust to branch master.

Bugs: 347724
<a href="https://bugs.kde.org/show_bug.cgi?id=347724" title="https://bugs.kde.org/show_bug.cgi?id=347724">https://bugs.kde.org/show_bug.cgi?id=347724</a>

Repository: plasma-workspace

Description
Currently the width of the date label is not fixed but changes depending on the text. This causes the entire applet to change its width (if the time is the widest displayed item). This in turn can cause all other applets in the same panel to move whenever the displayed time changes.

This patch uses FontMetrics to iterate over all possible time strings (with different width) and chooses the widest of them as reference for the fixed width of the time label.

This way the width of the applet stays the same (unless the date is displayed and changes). The text remains centered though, which means that it can still move within the applet when the time changes.

Diffs
applets/digital-clock/package/contents/ui/DigitalClock.qml 95bb071

Diff: <a href="https://git.reviewboard.kde.org/r/127102/diff/" title="https://git.reviewboard.kde.org/r/127102/diff/">https://git.reviewboard.kde.org/r/127102/diff/</a>

Testing
Works with horizontal and vertical panel.
Also displaying different combinations of "seconds", "date" and "timezone" works.

File Attachments
0001-Use-fixed-width-for-digital-clock-applet.patch
<a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/03/16/81b4a902-1454-4155-9fda-552b8acba1a8__0001-Use-fixed-width-for-digital-clock-applet.patch" title="https://git.reviewboard.kde.org/media/uploaded/files/2016/03/16/81b4a902-1454-4155-9fda-552b8acba1a8__0001-Use-fixed-width-for-digital-clock-applet.patch">https://git.reviewboard.kde.org/media/uploaded/files/2016/03/16/81b4a902...</a>

Thanks,

Daniel Faust

Re: Review Request 127102: Use fixed width for digital clock app

By Martin Klapetek at 03/16/2016 - 10:48

Ship it!

Ship It!

- Martin Klapetek

On March 16, 2016, 4:35 p.m., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Martin Klapetek at 03/15/2016 - 11:48

I've tested it extensively and it works great. Thanks a lot!

There's one more issue - <a href="http://paste.opensuse.org/view/raw/f8ba5d0d" title="http://paste.opensuse.org/view/raw/f8ba5d0d">http://paste.opensuse.org/view/raw/f8ba5d0d</a> - the same font size should also be applied to the left date label (there are; the bottom should stay as is). Do you think you could include it as part of this patch?

applets/digital-clock/package/contents/ui/DigitalClock.qml (line 554)
<https://git.reviewboard.kde.org/r/127102/#comment63784>

spaces around operators

applets/digital-clock/package/contents/ui/DigitalClock.qml (lines 564 - 567)
<https://git.reviewboard.kde.org/r/127102/#comment63785>

Can't we just compare "A" and "P" width's and use that? Would spare creating two Date objects and two calls to Qt.formatTime

- Martin Klapetek

On March 8, 2016, 6:53 p.m., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Martin Klapetek at 03/16/2016 - 10:47

I didn't claim it is. But there's no reason to not write
new code in an efficient way ;)

About the call numbers, yes, I'm aware of that. I wanted
to go over the whole applet and fix it, but then I was
assigned a different project and time was lacking.

Patches welcome for that too, of course :)

- Martin

On March 16, 2016, 4:35 p.m., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Daniel Faust at 03/16/2016 - 10:35

As you wish. But keep in mind that this is not a performance bottleneck.
I added some debug outputs to see where setupLabels is called from and it gets called 10 times when initializing the applet, including 3 times in the onCompleted method.
Even if it is a little bit off topic, here is the log (indentation was done manually and is somewhat guessed):

```
onShowDateChanged
timeFormatCorrection
onShowSecondsChanged
timeFormatCorrection
setupLabels
00:00:00 NACHM.
setupLabels
00:00:00 NACHM.

onDateFormatChanged
setupLabels
00:00:00 NACHM.

onLastSelectedTimezoneChanged
timeFormatCorrection
setupLabels
00:00:00 NACHM.

onDisplayTimezoneAsCodeChanged
setupLabels
00:00:00 NACHM.

onUse24hFormatChanged
timeFormatCorrection
setupLabels
00:00:00

onStateChanged
setupLabels
00:00:00

onCompleted
onSelectedTimeZonesChanged
setupLabels
00:00:00
dateTimeChanged
timeFormatCorrection
setupLabels
00:00:00
timeFormatCorrection
setupLabels
00:00:00
```

- Daniel

On März 16, 2016, 4:35 nachm., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Martin Klapetek at 03/15/2016 - 16:41

Yeah, that'd be great. Thanks!

Ah, good. Haven't thought of that. Those germans...

(on the other hand, I wouldn't expect anyone in Germany to actually not use 24h clock format, but oh well)

One other thing - create just a single Date object and then call setHours(13) on it for the second format.

- Martin

On March 8, 2016, 6:53 p.m., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Daniel Faust at 03/15/2016 - 12:45

I changed it, but now I'm not so sure anymore if the fixed font size wasn't intentional.
It would be a separate issue anyway and not connected to this one, so if you want, I can upload the patch to a new review request.

No, because eg. in german the strings for am and pm are "vorm." and "nachm.".

Sure thing.

- Daniel

On März 8, 2016, 6:53 nachm., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By david at 02/17/2016 - 19:05

applets/digital-clock/package/contents/ui/DigitalClock.qml (line 554)
<https://git.reviewboard.kde.org/r/127102/#comment63073>

rather than looping, can we use FontMetric's maximumCharacterWidth

* numChars.

Then we could kill sizeHelper competely (FontMetric's didn't exist when this was written)

- David Edmundson

On Feb. 17, 2016, 4:23 p.m., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Daniel Faust at 02/26/2016 - 10:36

So what do you think about the changes I made (revision 2)?

- Daniel

On Feb. 19, 2016, 6:11 nachm., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By =?utf-8?Q?Thoma... at 02/19/2016 - 11:07

As long as the formatter accepts invalid times, that's certainly the most straight forward solution, yes.

- Thomas

On Feb. 17, 2016, 4:23 p.m., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Daniel Faust at 02/19/2016 - 09:30

Ok, what about this?
Find the widest glyph between 0 and 9.
Use a regex to replace [hmsz] from the format string with the widest number. (This will produce strings like "44:44")
Use Qt.formatTime once with an AM time and once with a PM time and choose the widest of the produced strings.
This code is fairly short and overestimates the total width only slightly.

- Daniel

On Feb. 17, 2016, 5:23 nachm., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Daniel Faust at 02/19/2016 - 08:24

That would work if it wasn't for 12 hour time formats.
For 24 hour formats you actually need [0,2], [0,3] and [0,9] for the hour and [0,5] and [0,9] for the minutes/seconds.
For 12 hour formats AM you need [0,1], [0,2] and [0,9] for the hour and [0,5] and [0,9] for the minutes/seconds.
For 12 hour formats PM you need [1,2], [3,9] and [0,3] for the hour and [0,5] and [0,9] for the minutes/seconds.
On top of that you need to account for format changes like hour=0, minute=0 -> "12:00 AM".

The code for this logic would be hard to maintain.

- Daniel

On Feb. 17, 2016, 5:23 nachm., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By =?utf-8?Q?Thoma... at 02/18/2016 - 18:32

Forget about the timeFormat - you iterate from 0-9 and figure the widest glyphs in [0,1], [0,5] and [0,9] - let's reasonably say "0,3 and 7".
Then you check the width for 00:37

(If you also need the date, you also need to check for the widest glyph in [0,3])

- Thomas

On Feb. 17, 2016, 4:23 p.m., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Daniel Faust at 02/18/2016 - 13:27

I'm not sure if I understand you, I just tried it with a single loop from 0-9 to construct the time strings:

for (var i=0; i<=9; i++) {
var str = Qt.formatTime(new Date(2000, 0, 1, i*10 + i, i*10 + i, i*10 + i), main.timeFormat);
console.log("hour: " + (i*10 + i) + ", minute: " + (i*10 + i) + " -> " + str);
}

qml: hour: 0, minute: 0 -> 00:00
qml: hour: 11, minute: 11 -> 11:11
qml: hour: 22, minute: 22 -> 22:22
qml: hour: 33, minute: 33 -> 09:33
qml: hour: 44, minute: 44 -> 20:44
qml: hour: 55, minute: 55 -> 07:55
qml: hour: 66, minute: 66 -> 19:07
qml: hour: 77, minute: 77 -> 06:18
qml: hour: 88, minute: 88 -> 17:29
qml: hour: 99, minute: 99 -> 04:40

- Daniel

On Feb. 17, 2016, 5:23 nachm., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Martin Klapetek at 02/18/2016 - 13:14

You don't actually need them as a time, you just need them as a placeholder. At which point it doesn't matter if it's a valid time or not. You can even format 12:12 with Qt.formatTime and then replace the numbers with your placeholder number.

In other words, the time format /is/ known in advance.

- Martin

On Feb. 17, 2016, 5:23 p.m., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Daniel Faust at 02/18/2016 - 13:08

Since I don't know the time format in advance, I have to construct different times and process them with Qt.formatTime.
That means, I have to test 0-9 for hours/minutes/seconds, 0-2 for tens of hours and 0-5 for tens of minutes/seconds. Otherwise the constructed times will be invalid.
This can be done with three loops and it would bring down the amount of advanceWidth and formatTime calls from 24+60=84 to 3+6+10=19.

- Daniel

On Feb. 17, 2016, 5:23 nachm., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Martin Klapetek at 02/18/2016 - 12:31

You still don't need this whole loop though, just find the biggest in 0-9 and use that for all the numbers (except year).

- Martin

On Feb. 17, 2016, 5:23 p.m., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Daniel Faust at 02/18/2016 - 07:01

As far as I understand maximumCharacterWidth returns the width of the widest character of the font - which can be ridiculously wide given that the font supports some wild unicode characters.
I did a quick test and maximumCharacterWidth returned about twice the width actually needed.

- Daniel

On Feb. 17, 2016, 5:23 nachm., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Marco Martin at 02/18/2016 - 06:12

hoping maximumCharacterWidth is reliable for all fonts, this loop really needs to go

- Marco

On Feb. 17, 2016, 4:23 p.m., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By david at 02/17/2016 - 18:59

Is this needed after: <a href="https://git.reviewboard.kde.org/r/127073/" title="https://git.reviewboard.kde.org/r/127073/">https://git.reviewboard.kde.org/r/127073/</a> ?

- David Edmundson

On Feb. 17, 2016, 4:23 p.m., Daniel Faust wrote:

Re: Review Request 127102: Use fixed width for digital clock app

By Marco Martin at 02/18/2016 - 06:11

probably

- Marco

On Feb. 17, 2016, 4:23 p.m., Daniel Faust wrote: