Bug 449868 - Bar charts have no gaps between bars anymore
Summary: Bar charts have no gaps between bars anymore
Status: RESOLVED FIXED
Alias: None
Product: plasmashell
Classification: Plasma
Component: System Monitor widgets (show other bugs)
Version: 5.24.0
Platform: Arch Linux Linux
: NOR minor
Target Milestone: 1.0
Assignee: Plasma Bugs List
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2022-02-09 15:13 UTC by john.fano
Modified: 2022-02-28 17:00 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.24.3
Sentry Crash Report:


Attachments
bar chart gap example (28.24 KB, image/png)
2022-02-10 11:40 UTC, john.fano
Details

Note You need to log in before you can comment on or make changes to this bug.
Description john.fano 2022-02-09 15:13:59 UTC
SUMMARY
Bar charts for the system monitor panel widget seems to have lost the small gap between the bars with the 5.24 update, which makes it difficult to see the distinct bars on the panel, especially when the colors are similar and/or the levels are similar.

STEPS TO REPRODUCE
1.  Add System Monitor widget to panel
2.  Add sensors
3.  Set the chart to Bar Chart

OBSERVED RESULT
The bar chart for the System Monitor widget has the bars all run together and possibly overlapping slightly, although it's hard to tell at times if they are overlapping or if it's just my brain playing tricks.

EXPECTED RESULT
There used to be a small gap between the bars so they were visually distinctive.  This is especially noticeable with the Individual Core Usage widget.

SOFTWARE/OS VERSIONS
Linux: Arch Linux (5.16.7 kernel)
(available in About System)
KDE Plasma Version: 5.24.0
KDE Frameworks Version: 5.90.0
Qt Version: 5.15.2
Graphics Platform:  X11

ADDITIONAL INFORMATION
The bar charts are functioning correctly as far as the data they are reporting, just visually it all runs together now.  I've updated 3 machines from 5.23.5 so far with the same results.  On one machine I even removed the widget and added it back, which had no effect.
Comment 1 galder 2022-02-09 20:46:23 UTC
Hello,
could you please provide a screenshot?

Regards
Comment 2 galder 2022-02-09 20:50:29 UTC
needs info
Comment 3 john.fano 2022-02-10 11:40:51 UTC
Created attachment 146530 [details]
bar chart gap example
Comment 4 galder 2022-02-10 11:53:46 UTC
Thanks for the screenshot.
I see it clearly now. Maybe it is on propose.
Lets see what the developers think about.

Regards
Comment 5 galder 2022-02-10 11:57:25 UTC
reproducible with "individual core usage" widget
Comment 6 john.fano 2022-02-10 12:06:42 UTC
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!
Comment 7 Nate Graham 2022-02-11 16:55:15 UTC
This was an intentional change (see Bug 444585), but maybe it was the wrong change?
Comment 8 Arjen Hiemstra 2022-02-11 17:01:05 UTC
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.
Comment 9 Nate Graham 2022-02-11 17:01:58 UTC
Could we maybe add a single pixel of whitespace between the bars, rather than none?
Comment 10 john.fano 2022-02-14 17:32:57 UTC
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!
Comment 11 Nate Graham 2022-02-15 15:42:54 UTC
> 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.
Comment 12 john.fano 2022-02-16 12:44:08 UTC
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 :-)
Comment 13 Nate Graham 2022-02-16 15:57:30 UTC
Sounds good, I can help if you get stuck!
Comment 14 john.fano 2022-02-23 13:24:48 UTC
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!
Comment 15 Nate Graham 2022-02-23 15:05:35 UTC
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.
Comment 16 Bug Janitor Service 2022-02-24 14:16:15 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/libksysguard/-/merge_requests/220
Comment 17 Nate Graham 2022-02-28 16:57:47 UTC
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
Comment 18 Nate Graham 2022-02-28 17:00:30 UTC
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