Add support for SVG images #5275

corny · 2016-12-31T23:27:47Z

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Have you signed the Contributors agreement
Have you followed our code guidelines?

laf · 2016-12-31T23:31:11Z

Out of interest why?

I think a lot of this code will fail php 5.3 support due to the shorthand arrays used. Those will need updating.

Are you converting all os images? How would a new user provide an svg, it's easier to support png so we'd just end up with a mix.

corny · 2016-12-31T23:36:21Z

@laf the current PNG logos are looking aweful on high DPI displays.

I updated my pull request. foreach (array("svg", "png") as $ext) { should work with PHP 5.3? I replaced some logos by SVG versions. More work has to done, but PNG is still supported.

laf · 2016-12-31T23:48:13Z

I think I'd be more comfortable personally if you could add a doc on how to generate svg images to be used otherwise people are going to continue to use png images.

You've got more shorthand array calls which need updating. https://travis-ci.org/librenms/librenms/jobs/187973765

Thanks for contributing btw. You do need to sign the contributors agreement - you removed all of that text above, I've added it back in, please confirm as per the instructions.

corny · 2016-12-31T23:57:37Z

I added myself to the CONTRIBUTING.md, ok thanks for re-adding the text.

There is basically nothing to be aware of when creating SVG images. They are automatically scaled by stylesheets. I grabbed some images from wikipedia and I also cut some. Logos should just not be larger than ~ 20 kb. If the svg is too large, then paths should be simplified (e.g. with Inkscape). Where can I document it?

The SVG version of some logos is even smaller than the PNG.

corny · 2017-01-01T00:18:50Z

I also changed the second shorthand array and set a height for logo inside of the <span class="device_icon">.

laf · 2017-01-01T15:03:10Z

The contributors agreement needs to be signed as per the doc which means commiting a specific message.

As for docs, I'd say add something about the icon here:

http://docs.librenms.org/Developing/Support-New-OS/

and here:

http://docs.librenms.org/Support/FAQ/#how-do-i-add-support-for-a-new-os

corny · 2017-01-01T15:15:02Z

Thanks, I added a dedicated commit for the contributors agreement.

laf · 2017-01-02T00:10:55Z

Once you do some docs explaining to users creating new os' that svg is accepted and where to place the files then this will be good to merge imho.

corny · 2017-01-02T06:48:53Z

I added the docs.

corny · 2017-01-03T10:43:26Z

I used the SVG optimizer to shrink the images by more than 50 % in average.

LibreNMS-CI · 2017-01-03T10:45:15Z

Auto-Deploy finished, Test PR at http://5275.ci.librenms.org or https://5275.ci.librenms.org

murrant · 2017-01-03T15:25:06Z

I would like to have SVGs for use in larger than 32x32 locations for v2, so +1.

The php code could use some refinement. I think the findImage() function needs to be removed and folded in. See code notes for more.

murrant · 2017-01-03T15:26:00Z

includes/functions.php

-    if (file_exists($config['html_dir'] . '/images/os/' . $device['os'] . '.png')) {
-        return $device['os'];
+    foreach ($possibilities as $icon) {
+        if (!empty($icon)) {


You don't need this check if you use is_file() instead of file_exists() (which returns true for directories)

murrant · 2017-01-03T15:26:40Z

includes/functions.php

+    foreach ($possibilities as $icon) {
+        if (!empty($icon)) {
+            $name = findImage($icon);
+            if (!empty($name)) {


If you bring the findImage code into this function, you won't need the !empty() check here...

corny · 2017-01-03T16:27:29Z

@murrant thanks for the review. I pushed another commit.

LibreNMS-CI · 2017-01-03T16:29:45Z

Auto-Deploy finished, Test PR at http://5275.ci.librenms.org or https://5275.ci.librenms.org

murrant · 2017-01-03T16:37:51Z

Looking good now.

I would prefer the red and blue for the Cisco logo however :)

corny · 2017-01-03T16:48:01Z

@murrant The red/blue Cisco logo looks much better. I replaced it.

LibreNMS-CI · 2017-01-03T16:50:23Z

Auto-Deploy finished, Test PR at http://5275.ci.librenms.org or https://5275.ci.librenms.org

murrant · 2017-01-03T20:09:33Z

@corny This needs a rebase (docs got moved around in another commit)

@librenms/reviewers Are we concerned about supporting IE8 and older? SVG img tags is IE9+ (and any other browser)

laf · 2017-01-03T20:13:41Z

IE 8 is completely end of life.

I'd be happy to drop support.

corny · 2017-01-03T20:25:56Z

I rebased my branch on top of master.

LibreNMS-CI · 2017-01-03T20:28:13Z

Auto-Deploy finished, Test PR at http://5275.ci.librenms.org or https://5275.ci.librenms.org

murrant · 2017-01-03T20:46:17Z

Ok, I also reviewed the icons. They all look great, a few deviate in a way that might be concerning.

apple - some might not like the change to the rainbow logo, I'm fine with it.
aruba and mimosa: these could be changed to just the first letter of the logo as they were before.

A good indicator to me is the favicon of a companies website. Perhaps it would be a good idea to have both icon and logo, but it would be pointless if they are not used. That would probably be out of scope for this PR, so let's not worry about it ;)

corny · 2017-01-03T21:07:39Z

Yeah, I could not find a nice looking apple logo. Do you prefer another one?
https://commons.wikimedia.org/wiki/File:Apple_logo_black.svg (look at "Other versions")

I can cut the Aruba and Mimosa logos to the first letter. As a small square icon they would be definitely easier to be recognized. Other opinions?

murrant · 2017-01-03T21:36:25Z

I prefer the rainbow logo over the plain black.

corny · 2017-01-04T20:21:14Z

I rebased my branch on top of master.

LibreNMS-CI · 2017-01-04T20:22:18Z

Auto-Deploy finished, Test PR at http://5275.ci.librenms.org or https://5275.ci.librenms.org

laf · 2017-01-06T23:40:22Z

trigger ci

ghost · 2017-01-07T09:13:45Z

IMO, I don't think that you can use the logo that you prefer. You need to use the correct corporate logo that match the actual design. The Apple rainbow logo isn't used since 1998...

laf · 2017-01-08T16:31:59Z

@librenms/reviewers I'm ok with this. Just rebasing it now so it's ready for a merge.

LibreNMS-CI · 2017-01-08T16:36:02Z

Auto-Deploy finished, Test PR at http://5275.ci.librenms.org or https://5275.ci.librenms.org

murrant · 2017-01-08T18:45:14Z

Did we want to change the Apple icon as FTBZ mentioned? It would probably be to the monochrome grey one.

laf · 2017-01-08T18:57:16Z

I'm not overly fussed but the black one is more modern imho.

…oc/General/Contributing.md.

LibreNMS-CI · 2017-01-08T19:17:30Z

Auto-Deploy finished, Test PR at http://5275.ci.librenms.org or https://5275.ci.librenms.org

corny · 2017-01-08T19:17:52Z

I replaced by Apple logo by the grey one and rebased again.

LibreNMS-CI · 2017-01-08T19:20:07Z

Auto-Deploy finished, Test PR at http://5275.ci.librenms.org or https://5275.ci.librenms.org

scrutinizer-notifier · 2017-01-08T19:22:49Z

The inspection completed: No new issues

murrant reviewed Jan 3, 2017

View reviewed changes

laf approved these changes Jan 8, 2017

View reviewed changes

f0o approved these changes Jan 8, 2017

View reviewed changes

corny added 4 commits January 8, 2017 20:13

I agree to the conditions of the Contributor Agreement contained in d…

4e510a5

…oc/General/Contributing.md.

feature: Add support for SVG images

63830f2

docs: Add instructions for adding OS images

5a9c39f

Replace PNG os images by SVG

011a6f6

murrant approved these changes Jan 8, 2017

View reviewed changes

murrant merged commit 5ad8fd3 into librenms:master Jan 8, 2017

corny deleted the svg-images branch January 8, 2017 20:29

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

Add support for SVG images #5275

Add support for SVG images #5275

Conversation

corny commented Dec 31, 2016 • edited Loading

Please note

laf commented Dec 31, 2016

corny commented Dec 31, 2016

laf commented Dec 31, 2016

corny commented Dec 31, 2016

corny commented Jan 1, 2017

laf commented Jan 1, 2017

corny commented Jan 1, 2017

laf commented Jan 2, 2017

corny commented Jan 2, 2017

corny commented Jan 3, 2017 • edited Loading

LibreNMS-CI commented Jan 3, 2017

murrant commented Jan 3, 2017 • edited Loading

murrant Jan 3, 2017

Choose a reason for hiding this comment

murrant Jan 3, 2017

Choose a reason for hiding this comment

corny commented Jan 3, 2017

LibreNMS-CI commented Jan 3, 2017

murrant commented Jan 3, 2017

corny commented Jan 3, 2017

LibreNMS-CI commented Jan 3, 2017

murrant commented Jan 3, 2017

laf commented Jan 3, 2017

corny commented Jan 3, 2017

LibreNMS-CI commented Jan 3, 2017

murrant commented Jan 3, 2017 • edited Loading

corny commented Jan 3, 2017 • edited Loading

murrant commented Jan 3, 2017

corny commented Jan 4, 2017

LibreNMS-CI commented Jan 4, 2017

laf commented Jan 6, 2017

ghost commented Jan 7, 2017

laf commented Jan 8, 2017

LibreNMS-CI commented Jan 8, 2017

murrant commented Jan 8, 2017

laf commented Jan 8, 2017

LibreNMS-CI commented Jan 8, 2017

corny commented Jan 8, 2017

LibreNMS-CI commented Jan 8, 2017

scrutinizer-notifier commented Jan 8, 2017

corny commented Dec 31, 2016 •

edited

Loading

corny commented Jan 3, 2017 •

edited

Loading

murrant commented Jan 3, 2017 •

edited

Loading

murrant commented Jan 3, 2017 •

edited

Loading

corny commented Jan 3, 2017 •

edited

Loading