DevHeads.net

Review Request 127393: digital-clock: Fix font size of date label in small panels

Review request for kde-workspace.

Repository: plasma-workspace

Description
Currently the digital clock applet uses a fixed font size for the date label when it's placed in a narrow horizontal panel.
Example: <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>

With this patch the same font size is used as for the time label.

As mentioned at <a href="https://git.reviewboard.kde.org/r/127102/" title="https://git.reviewboard.kde.org/r/127102/">https://git.reviewboard.kde.org/r/127102/</a> I'm not sure if the current design is a bug or intentional.
On the one hand having a smaller font size reduces the width of the applet, on the other hand having the same font size is more consistent.
I would prefer a consistent look however.

This patch creates one problem though. Currently the height of the date-time-separator is set to the height of the (fixed) date label font size.
With this patch I set the separator height to 70% of the applet height.

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

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

Testing

Thanks,

Daniel Faust

Comments

Re: Review Request 127393: digital-clock: Fix font size of date

By Daniel Faust at 03/19/2016 - 09:16

(Updated März 19, 2016, 2:16 nachm.)

Review request for kde-workspace.

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

Repository: plasma-workspace

Description
Currently the digital clock applet uses a fixed font size for the date label when it's placed in a narrow horizontal panel.
Example: <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>

With this patch the same font size is used as for the time label.

As mentioned at <a href="https://git.reviewboard.kde.org/r/127102/" title="https://git.reviewboard.kde.org/r/127102/">https://git.reviewboard.kde.org/r/127102/</a> I'm not sure if the current design is a bug or intentional.
On the one hand having a smaller font size reduces the width of the applet, on the other hand having the same font size is more consistent.
I would prefer a consistent look however.

This patch creates one problem though. Currently the height of the date-time-separator is set to the height of the (fixed) date label font size.
With this patch I set the separator height to 70% of the applet height.

Diffs (updated)
applets/digital-clock/package/contents/ui/DigitalClock.qml 02d55a9

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

Testing

Thanks,

Daniel Faust

Re: Review Request 127393: digital-clock: Fix font size of date

By Daniel Faust at 04/09/2016 - 06:12

(Updated April 9, 2016, 12:12 nachm.)

Status
This change has been discarded.

Review request for kde-workspace.

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

Repository: plasma-workspace

Description
Currently the digital clock applet uses a fixed font size for the date label when it's placed in a narrow horizontal panel.
Example: <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>

With this patch the same font size is used as for the time label.

As mentioned at <a href="https://git.reviewboard.kde.org/r/127102/" title="https://git.reviewboard.kde.org/r/127102/">https://git.reviewboard.kde.org/r/127102/</a> I'm not sure if the current design is a bug or intentional.
On the one hand having a smaller font size reduces the width of the applet, on the other hand having the same font size is more consistent.
I would prefer a consistent look however.

This patch creates one problem though. Currently the height of the date-time-separator is set to the height of the (fixed) date label font size.
With this patch I set the separator height to 70% of the applet height.

Diffs
applets/digital-clock/package/contents/ui/DigitalClock.qml 02d55a9

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

Testing

Thanks,

Daniel Faust

Re: Review Request 127393: digital-clock: Fix font size of date

By Martin Klapetek at 03/21/2016 - 11:08

Ship it!

Thanks!

- Martin Klapetek

On March 19, 2016, 2:16 p.m., Daniel Faust wrote:

Re: Review Request 127393: digital-clock: Fix font size of date

By Daniel Faust at 04/09/2016 - 06:12

Now that Plasma 5.6 has arrived in opensuse Tumbleweed, I was finally able to finish my re-write. It uses the default font size for all labels in narrow panels.
I'm pretty sure that was always the intended way plus I think it looks better than using a scaled font.

- Daniel

On März 19, 2016, 2:16 nachm., Daniel Faust wrote:

Re: Review Request 127393: digital-clock: Fix font size of date

By Daniel Faust at 03/21/2016 - 11:47

I already started to rewrite the clock applet. I got pretty far actually, and this issue arose again.
I think it was always intended to use the default font size in narrow panels, but since font.pointSize and font.pixelSize were both set,
setting pointSize to the default font size never worked.

So now my tendecy is to use the default font size and post my changes once I'm done.

- Daniel

On März 19, 2016, 2:16 nachm., Daniel Faust wrote:

Re: Review Request 127393: digital-clock: Fix font size of date

By Martin Klapetek at 03/16/2016 - 16:31

Fix it, then Ship it!

Thanks!

For future reference, the proper group to add to Plasma/workspace reviews is "plasma".

applets/digital-clock/package/contents/ui/DigitalClock.qml (lines 382 - 383)
<https://git.reviewboard.kde.org/r/127393/#comment63848>

You can't set both, it will print an error that you can't and one is ignored

Also leave this in the font { } format please

- Martin Klapetek

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

Re: Review Request 127393: digital-clock: Fix font size of date

By Daniel Faust at 03/19/2016 - 09:17

Yeah, you're right.

- Daniel

On März 19, 2016, 2:16 nachm., Daniel Faust wrote:

Re: Review Request 127393: digital-clock: Fix font size of date

By Martin Klapetek at 03/17/2016 - 13:46

Well changing code just for the sake of change is not
really worth it as it makes looking up history of that
line, the actual change, needlessly harder.

- Martin

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

Re: Review Request 127393: digital-clock: Fix font size of date

By Daniel Faust at 03/17/2016 - 12:32

Yes, I copy'n'pasted it. How about I throw in an additional commit to fix it everywhere in the file?

- Daniel

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

Re: Review Request 127393: digital-clock: Fix font size of date

By Martin Klapetek at 03/16/2016 - 13:19

This.

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

Have you tried dateLabelLeft.paintedHeight?

- Martin Klapetek

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

Re: Review Request 127393: digital-clock: Fix font size of date

By Martin Klapetek at 03/16/2016 - 16:30

Hah, interesting. Ok, let's go with your patch then.

- Martin

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

Re: Review Request 127393: digital-clock: Fix font size of date

By Daniel Faust at 03/16/2016 - 14:26

This is the result: <a href="http://paste.opensuse.org/view/raw/0b724b7b" title="http://paste.opensuse.org/view/raw/0b724b7b">http://paste.opensuse.org/view/raw/0b724b7b</a>

- Daniel

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

Re: Review Request 127393: digital-clock: Fix font size of date

By Martin Klapetek at 03/16/2016 - 14:19

That's exactly what .paintedHeight property is for. Just try it.

- Martin

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

Re: Review Request 127393: digital-clock: Fix font size of date

By Daniel Faust at 03/16/2016 - 14:11

I had a second look at your screen shot, I thought that the separator had the exact height as the rendered text, but it really has the height of the label.
After my changes I thought that the separator was too high, so I scaled it down to 70%. But it seems that leaving it at 100% is like it was before.
Since it seems too complicated to calculate the rendered text height, leaving it at 100% height might be the better choice.

- Daniel

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

Re: Review Request 127393: digital-clock: Fix font size of date

By Martin Klapetek at 03/16/2016 - 13:45

paintedHeight is the actual height that the painted text has in pixels.

- Martin

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

Re: Review Request 127393: digital-clock: Fix font size of date

By Daniel Faust at 03/16/2016 - 13:28

I think that would be the same as dateLabelLeft.height, but the separator is meant to have the same height as the font, not the label.
I would need something like "paintedFontPixelSize".
Alternatively I could calculate the font.pixelSize as a percentage of the label height and use that as a scaling factor.

- Daniel

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