| romangg |
| Plasma | |
| VDG |
BUG: 375969
This patch makes the digital clock use a smaller (fixed) font when used in a horizontal form factor.
Vertical is unchanged because it's already too small, and the user can resize the widget when it's on the desktop.
Here's what it looks like by default:
Other sizes:
Other fonts:
Play with the widget settings in different panels/desktop
| No Linters Available |
| No Unit Test Coverage |
i don't think i like the idea of adding yet another setting on this already rather fragile code
| applets/digital-clock/package/contents/ui/DigitalClock.qml | ||
|---|---|---|
| 473 | We use font.pixelSize to avoid the size "jumping" when it fits to a smaller panel the size can fit. https://bugs.kde.org/show_bug.cgi?id=370156 Use font.pixelSize: plasmoid.configuration.fontSize * units.devicePixelRatio | |
@Zren Thanks! I've updated the rev
@mart I can see some parts of the code look a little too hacky but the new setting is nothing fancy, and I've been using for about 3 weeks without issues. It also fixes these two bugs: https://bugs.kde.org/show_bug.cgi?id=191889 and https://bugs.kde.org/show_bug.cgi?id=323223
PING
Hey guys, the beta is starting next week. Any chance we can still land this or https://phabricator.kde.org/D6813 for 5.11?
i don't think i like the idea of adding yet another setting on this already rather fragile code
I think this explains why you haven't had a review, this existing code is already sketchy as-is :/
But we should be trying to fix that not just all of us ignoring it. Sorry about that.
So this clock has 4 states:
Currently:
1 & 2 choose the font size to fill the space vertically indefinitely
3 chooses the font size that fits the space horizontally up to 3x normal font size
4 is... frankly unreadable.
With your patch:
in states 1 and 2 you're explicitly setting the time zone label to a config value, and implicitly the date and TZ label to 0.7 of that.
I could understand logic that limits the maximiumSize to a user defined value, but I don't want to be in a position where the user resizes a panel..then has to open all the config options of each widget and adjust things. Would keeping the fit mode but setting the maximumPixelSize to the config value work for you?
States 3 and 4 are unchanged:
I don't really want to have a config option that simply does nothing. Is there a reason it's not changed?
I think this explains why you haven't had a review, this existing code is already sketchy as-is :/
But we should be trying to fix that not just all of us ignoring it. Sorry about that.
No problem, I can understand the dread of piling stuff on code that needs refactoring.
So this clock has 4 states:
- horizontal
- horizontal small (date and tz appear alongside clock)
- vertical
- other ()
I don't think I've ever seen state 4. How do you trigger it?
With your patch:
in states 1 and 2 you're explicitly setting the time zone label to a config value, and implicitly the date and TZ label to 0.7 of that.I could understand logic that limits the maximiumSize to a user defined value, but I don't want to be in a position where the user resizes a panel..then has to open all the config options of each widget and adjust things. Would keeping the fit mode but setting the maximumPixelSize to the config value work for you?
That's a better solution! We could use a float value, and present it more as a percentage instead of a font size value.
The only thing to look out for is a minimum size limit, so the widget doesn't vanish when the user resizes the panel/widget.
States 3 and 4 are unchanged:
I don't really want to have a config option that simply does nothing. Is there a reason it's not changed?
The main idea of this setting was to be able to make the clock smaller than the panel size. Vertical panels are usually thin, and the font size for digital clocks turns out to be fairly small already. So I didn't think it was really useful to set a font size there. It wouldn't hurt either though.
So, I've been fighting with this for a few weekends already and I just can't get it to work reliably with a relative scale. The spacing goes all wonk and sometimes the labels' font size don't change the way you expect them to (probably because of the maze of bindings).
I got frustrated and started writing a quick proof-of-concept rewrite, and it's working pretty well so far. Instead of using a grid + 3 labels (and some "helper" elements), the new code uses a single label. All the magic happens in the label's text. You can check the code here:
https://github.com/diegogangl/digital_clock_r/blob/master/package/contents/ui/ClockLabel.qml
There's a lot missing of course, but I could finish/polish it and submit a patch. Custom date formats could also be doable with this code. What do you guys think? Would you be interested in a rewrite patch?
Given how often this codebase's unmaintainability comes up, I think that would be very welcome!
If its good, sure.
If its simpler because it doesn't do all the things, then no.
It's important to know why some things are done the way they were. Otherwise we just go round in circles fixing the reintroducing bugs.
Like there's the implicit size hint being fixed so that if you use a font where "1" is thinner than "0" the clock doesn't jump contents about.
Yours doesn't seem to have that.
The other horrific monster in the current code is using the user's locale but disabling seconds.
Thanks for the replies guys.
@davidedmundson Good point! I'll keep a close eye on the current code while I work on this.
It also might be worth stepping back and asking why people want a size adjustment setting in the first place. I'm willing to bet that most of the time, it's because they think the default size is too big (because it is really big--way bigger than any of the other text in the panel). Perhaps if we address that by reducing the default size a bit, the demand for a user-changeable setting would decrease.
I think this describes the main problem for most users (including me) very good:
KDE Forum: Padding in plasma panels
It look out of place and jumps right into the eyes.
I have three screens, the main screen display the time with secons and the date, while the other two just show the time /default settings) and are really big.
Exactly: people in that thread think the default size is too big. I think if we just make the default size smaller, the complaints will go away.
Actually I posted that on the forums before deciding to make this patch :P
Yeah, we can do that. However IMO the tricky part isn't having a setting, it's making the text (and/or label heights) scale relative to the size of the panel. Label heights and font settings depend on each other (and the helper(s)) in different ways depending on the form factor. Maybe I can bring the formulas from the rewrite code to set a fontSize, then check to see where it breaks and clamp it (hopefully it won't break the rest of the layout).
An alternative would be to set a fixed font size, like say the default desktop font size or a multiplier of it. It could still keep the fontSizeMode to fit and be relative-ish, the font size would go shrink until the minimum allowed if it didn't have enough space.
Reducing the scope of this patch a bit, it now sets the clock fontsize to a fixed value. This prevents all the layout issues from relative scales. The code changes now are minimal, so it won't add pile more complexity on the code.
I'll continue working on the rewrite when I have some free time again, and submit a patch when/if I have feature parity with the current clock.
Why did you change the 0.56 * main.height value in case a date is shown? Reduces the clock size in two line mode and in this mode it was not too big.
Also the clock with height units.iconSizes.small is a bit too small in comparision to the other icons left and right. I don't know why this is (I think there is some automatic spacing in fonts). I would say it is perfect with height units.iconSizes.small + units.smallSpacing.
main.height is relative to the panel size. It was kind of weird that the font size would go from a fixed size to a relative one when enabling dates. I tried to keep it somewhat close to the size of the one line mode, but still legible.
Good idea, it's much closer. It's off by a little, probably because of all the hoops needed to calculate the font size and the difference between the x-height and the numbers height in the font itself.
To be honest I think the clock should scale with the panel size. When only presenting the time it should be in all panel sizes smaller than currently as you rightfully want to change, but it shouldn't just always be of size units.iconSizes.small, i.e. the size of the tray icons (which should also scale a bit until a certain threshold and then jump to 2 lines mode, since everything else scales in a panel).
TLDR: Please do a scaling time font, which is smaller than currently but scales with panel size, i.e. not just always the same small size. Thanks in advance.
Use relative scaling. The 0.71 factor looks funny but it aligns with the icons the best.
Also reverted the change for clock+date.
Looks good to me now. The 0.71 factor is of course rather arbitrary, but I think just multiplying with an arbitrary fixed factor is in the context of the current Digital Clock code the best thing to do. Before commiting please also add a small comment, that this value was only chosen through testing, such that the size was fine. This way somebody, who later reads the code, does not think that there is some deep meaning to this magic number.
Do you have commit rights? Shall I push for you (pls tell me then name and email address so I can acknowledge you as the author)?
I don't have commit rights (AFAIK :P), please push it.
Name: Diego Gangl
Email: diego@sinestesia.co
I think we can make another applet called Small Digital Clock, which can be an alternative of Digital Clock. Just like we have different alternatives for launcher and task bar.