Use browser width to scale cpu and bandwidth graphs on device overview #2537

Merged
merged 2 commits into from Jan 6, 2016

Projects

None yet

6 participants

@Alucardfh

Hi ,

Following what was done in PR #2510 this is an attempt to have the graphs in the device overview to scale according to the size of the browser.

Current :
deviceoverview

Changes :
deviceoverviewchanges

@f0o
Member
f0o commented Dec 2, 2015

๐Ÿ‘ works here on upstream chrome and huge resolution (3840x1080) as well as on a smartphone with upstream firerox

@Rosiak
Contributor
Rosiak commented Dec 2, 2015

Tagging @paulgear for testing, in relation to the latest issues.

@f0o
Member
f0o commented Dec 2, 2015

Good point, taggin @vectr0n as well for the same reasons

@laf
Member
laf commented Dec 8, 2015

๐Ÿ‘ from me.

@laf
Member
laf commented Dec 12, 2015

@librenms/reviewers anyone else?

@Rosiak
Contributor
Rosiak commented Dec 12, 2015

๐Ÿ‘
No issues here.
Tested on firefox, chrome and safari.

@f0o
Member
f0o commented Dec 13, 2015

Merge in 48h unless somebody throws in a blocker :)

@paulgear
Member

Tested this on my system. It is still giving blurry graphs. The reason for this is that when you ask graph.php for a certain resolution, it gives you a graph somewhat larger than that, because the width & height don't include the X & Y axes. e.g. if I go to http://librenms.localhost/graph.php?width=841&height=280&device=1&type=device_bits&legend=no I get a picture that is 922 x 319, not 841 x 280. Adding this patch to generate_lazy_graph_tag() should sort it out:

diff --git a/html/includes/functions.inc.php b/html/includes/functions.inc.php
index b303255..e1f388e 100644
--- a/html/includes/functions.inc.php
+++ b/html/includes/functions.inc.php
@@ -476,6 +476,7 @@ function generate_lazy_graph_tag($args) {
         }
         $urlargs[] = $key."=".urlencode($arg);
     }
+    $urlargs[] = "absolute=1";

     if ($config['enable_lazy_load'] === true) {
         return '<img class="lazy graphs" width="'.$w.'" height="'.$h.'" data-original="graph.php?' . implode('&amp;',$urlargs).'" border="0" />';
@f0o
Member
f0o commented Dec 15, 2015

๐Ÿ‘ on @paulgear's remark

@paulgear
Member

I did a little playing with this earlier, and it might need a little more than just absolute=1, because that makes a mess when you turn on legends. I haven't looked at the code closely, but is there a way we can ask it to produce an N x M graph, but then not constrain it to that size in the HTML attributes?

@f0o
Member
f0o commented Dec 15, 2015

Sure, scaping the HTML atributes. But that makes the page jump when using lazy load

@Alucardfh

Yeah, you are running in exactly the same mess I was when I worked on this.

As rrdtool is only fed the canva size it gives you a graph that is sometimes quite bigger than what you expect, especially if the legend/axis have lot of text.
If you ask rrdtool to render a graph of a fixed size the canva is usually so small it is unreadable.

So the workaround I used was to resize the width of the graph to be the width requested but keep using lazy_load so the graph can expand in height. Although this does implie a certain loss of quality,

I don't really get why you had to patch it to use lazy_load as I was using it before.

@f0o
Member
f0o commented Dec 20, 2015

What's the status on this now @paulgear ?
Merge or are you keeping your blocker?

@f0o f0o added Blocker and removed Merge in 48 hours labels Dec 20, 2015
@Rosiak
Contributor
Rosiak commented Dec 20, 2015

Tagging #2633

@paulgear
Member

My preference would be: use absolute=1 on all graphs without a legend; this will allow maximum clarity without any cost. For other more general graphs, I think we should prioritise visual quality over smooth operation under lazy load, since having good looking graphs is one of the things that makes people come to LibreNMS. But I don't think this is a blocker; we've had the blurry graphs for quite a few weeks and it has only been a big issue for one user that we know of.

@Alucardfh

Sorry, I hadn't much time to work on this earlier.

There is less blurring now, as I have make the lazy tag expect a graph that is more or less what it will get, hence no resize.

I am not especially convince by the solution, as to implement it everywhere it basically requires guessing what size the axis label will be for each type of graph, With the assumption that the axis label size will not change much with different dataset.

Anyway, I am happy to hear feedback.

For #2633 , does it cause the same blurring or the native bootstrap function is better/smarter ?
I would be happy to hand it over to bootstrap if it give better results.

Louis Bailleul added some commits Dec 2, 2015
Louis Bailleul Use browser width to scale cpu and bandwidth graphs on device overview ee75091
Louis Bailleul Rebase
b40bd2f
@paulgear
Member
paulgear commented Jan 1, 2016

๐Ÿ‘ to merge this alongside PR#2633

@paulgear paulgear added Graphs and removed Blocker labels Jan 2, 2016
@Alucardfh

@paulgear Will you take care of the merging with PR #2633 or do you want to merge it first and I update this PR ?

@paulgear paulgear merged commit 590593e into librenms:master Jan 6, 2016

2 checks passed

Auto-Deploy Build finished. No test results found.
Details
Scrutinizer 2 new issues
Details
@paulgear
Member
paulgear commented Jan 6, 2016

@Alucardfh I've merged - thanks for your efforts on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment