| Summary: | Bar charts have no gaps between bars anymore | ||
|---|---|---|---|
| Product: | [Plasma] plasmashell | Reporter: | john.fano |
| Component: | System Monitor widgets | Assignee: | Plasma Bugs List <plasma-bugs> |
| Status: | RESOLVED FIXED | ||
| Severity: | minor | CC: | ahiemstra, lemmyg, nate, notmart |
| Priority: | NOR | Keywords: | regression |
| Version: | 5.24.0 | ||
| Target Milestone: | 1.0 | ||
| Platform: | Arch Linux | ||
| OS: | Linux | ||
| See Also: | https://bugs.kde.org/show_bug.cgi?id=444585 | ||
| Latest Commit: | https://invent.kde.org/plasma/libksysguard/commit/309e7aa8a6ce58569c49713247a507210c41976c | Version Fixed In: | 5.24.3 |
| Sentry Crash Report: | |||
| Attachments: | bar chart gap example | ||
|
Description
john.fano
2022-02-09 15:13:59 UTC
Hello, could you please provide a screenshot? Regards needs info Created attachment 146530 [details]
bar chart gap example
Thanks for the screenshot. I see it clearly now. Maybe it is on propose. Lets see what the developers think about. Regards reproducible with "individual core usage" widget You're welcome. Just to clarify, even though this is the core usage widget, the gap is now missing from ANY bar chart even when using it to monitor RAM, swap, disk space, etc... The core usage is livable since the graph bounces all around, but those more static bars are much harder to read now at a quick glance. The gap really helped that instant visual feedback of the graph. Thanks! This was an intentional change (see Bug 444585), but maybe it was the wrong change? It was an intentional fix for 5.24. The simple fact is that if there's not enough space it's better to show bars than whitespace. However, I did consider making this configurable, but that needed to wait for 5.25 since it'd involve new strings. Could we maybe add a single pixel of whitespace between the bars, rather than none? Thanks for providing the link to the other bug report. I understand why this change was made now, however, I did some playing on my 5.23.5 and 5.24.0 VMs and I think that the patch to fix the 444585 bug may also a bug. The calculation will always be zero (0).
The spacing calculation was a simple round integer calculation. Chart width divided by 20. I didn't dig through the code, but I was able to infer based on the example calculations below that the bar width on the panel view is 9px and the bar width in the pop-up chart is 17px.
Have a look at these examples, which I confirmed the results by measuring the pixels in GIMP:
Under 5.23.5 with the old spacing calculation:
8 sensors:
- Panel view - 34px / 20 = 1.7 rounds to 2, yielding a bar width of 7px
- Pop-up view - 112px / 20 = 5.6 rounds to 6, yielding a bar width of 11px
20 sensors:
- Panel view - 173px / 20 = 8.65 rounds to 9, yielding a bar width of 0px
- Pop-up view - 320px / 20 = 16px, which is exactly what I measure with GIMP, yielding a bar width of 1px
Under 5.24.0:
25 sensors
- Panel view - (225px / 25) * 0.05 = 0.454 floor of 0, yielding a bar width of 9px
- Pop-up view - (418px / 25) * 0.05 = 0.836 floor of 0, yielding a bar width of 17px
28 sensors:
- Panel view - (254px / 28) * 0.05 = 0.455 floor of 0, yielding a bar width of 9px
- Pop-up view - (472px / 28) * 0.05 = 0.843 floor of 0, yielding a bar width of 17px
So as the chart width scales up with the number of sensors, the floor calculation for the spacing will always be 0. Even if that was switched to round instead of floor, it would always be a 1 so it's moot to have either calculation.
Ultimately it would be nice to have the spacing as a user configurable value for those with high core/thread count CPUs, but I understand that would take GUI changes to make that available. A quick and simple solution is to set the spacing to a fixed 2px size rather than using either calculation which will yield a very quick and easy to read graph that scales well in both views.
Thanks!
> A quick and simple solution is to set the spacing to a fixed 2px size rather than using either
> calculation which will yield a very quick and easy to read graph that scales well in both views.
That makes sense to me. Would you be interested in submitting a merge request to do this?
Side note: don't actually use 2px, use Math.floor(Kirigami.Units.smallSpacing) which will give you the same thing, but also adjust to the user's font size if they choose a huge font.
I'm the only sysadmin at work and I use Python for system automation, so I've never done a merge request before or worked with QML, but I am always up for learning and trying something new, so I will give it a go :-) Sounds good, I can help if you get stuck! Nate, I have a fix that is working well at different font sizes, resolutions and DPI's. The Kirigami.Units.smallSpacing seems to be the smallest size I could find in the documentation. Is that accurate or is there a smaller unit? That one at it's default size yielded a spacing that seemed unnatural and wasteful on panel real estate. I ended up dividing that in half and that is where I am at now. It's a small enough spacing to not waste panel space, yet the bar chart is very quickly readable with both high motion and static bars. I just wanted to make sure I wasn't missing a smaller unit in the documentation before I submit my merge request. Thanks! That's the smallest unit, but if you find that it's still too big, it's acceptable to do `Math.floor(Kirigami.Units.smallSpacing)` Always round down when you divide a standard unit. A possibly relevant merge request was started @ https://invent.kde.org/plasma/libksysguard/-/merge_requests/220 Git commit 1b36ab8c5884a21f52f6cae69dddef8071e0c290 by Nate Graham, on behalf of John Fano. Committed on 28/02/2022 at 16:57. Pushed by ngraham into branch 'master'. Fixed computational bug for bar chart spacing The current calculation method for the bar chart spacing would always result in a spacing size of zero (0). This fix implements a fixed size bar spacing scaling with the font size. It allows the chart to scale well at any resolution, DPI and font size as well as with any number of items charted. FIXED-IN: 5.24.3 M +1 -1 faces/facepackages/barchart/contents/ui/BarChart.qml https://invent.kde.org/plasma/libksysguard/commit/1b36ab8c5884a21f52f6cae69dddef8071e0c290 Git commit 309e7aa8a6ce58569c49713247a507210c41976c by Nate Graham, on behalf of John Fano. Committed on 28/02/2022 at 17:00. Pushed by ngraham into branch 'Plasma/5.24'. Fixed computational bug for bar chart spacing The current calculation method for the bar chart spacing would always result in a spacing size of zero (0). This fix implements a fixed size bar spacing scaling with the font size. It allows the chart to scale well at any resolution, DPI and font size as well as with any number of items charted. FIXED-IN: 5.24.3 (cherry picked from commit 1b36ab8c5884a21f52f6cae69dddef8071e0c290) M +1 -1 faces/facepackages/barchart/contents/ui/BarChart.qml https://invent.kde.org/plasma/libksysguard/commit/309e7aa8a6ce58569c49713247a507210c41976c |