Add a web page to view and compare icons of different sizes
AbandonedPublic

Authored by guoyunhe on Mar 16 2019, 10:30 PM.

Details

Reviewers
ngraham
ndavis
Group Reviewers
VDG
Breeze
Summary

This simple web page provides a table of all icons. So developers can easily know which icon size is
missing, or not in dark theme.

Diff Detail

Repository
R266 Breeze Icons
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10055
Build 10073: arc lint + arc unit
guoyunhe created this revision.Mar 16 2019, 10:30 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 16 2019, 10:30 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
guoyunhe requested review of this revision.Mar 16 2019, 10:30 PM
ngraham requested changes to this revision.Mar 16 2019, 10:40 PM
ngraham added a subscriber: ngraham.

Neato! Looks pretty handy. Two issues I found:

  • generate_web_data.sh isn't executable
  • I'd like to see this run as part of the build process, so that index.html gets updated automatically, and so running it doesn't pollute your source folder since you're of course doing out-of-source builds :) Then you don't have to add the generated text files to .gitignore.
This revision now requires changes to proceed.Mar 16 2019, 10:40 PM
guoyunhe updated this revision to Diff 54048.Mar 16 2019, 10:44 PM

Fix data updating issues

guoyunhe updated this revision to Diff 54049.Mar 16 2019, 10:54 PM

Add CMake build steps for web page

guoyunhe edited the summary of this revision. (Show Details)Mar 16 2019, 10:55 PM
guoyunhe added a reviewer: Breeze.

Neato! Looks pretty handy. Two issues I found:

  • generate_web_data.sh isn't executable
  • I'd like to see this run as part of the build process, so that index.html gets updated automatically, and so running it doesn't pollute your source folder since you're of course doing out-of-source builds :) Then you don't have to add the generated text files to .gitignore.

Hi @ngraham , I have added execution bit to the script. I have too little knowledge of CMake. Not sure if this will do what you said. Can you give some hints?

guoyunhe updated this revision to Diff 54050.Mar 16 2019, 10:59 PM
This comment was removed by guoyunhe.

I assume this fetches the icons from your local system?

mmm, I'm afraid that didn't work. running cmake on it reveals the following:

CMake Error at CMakeLists.txt:71 (add_custom_target):
  add_custom_target cannot create target "generate-web" because another
  target with the same name already exists.  The existing target is a custom
  target created in source directory "/home/dev/kde/src/breeze-icons/icons".
  See documentation for policy CMP0002 for more details.

You have it inside the generate_binary_resource function, which means that it can be called twice. You'll need to move this target outside of that function.

I assume this fetches the icons from your local system?

yes. it depends on text files generated by scripts, which contains list of all icons

guoyunhe updated this revision to Diff 54051.Mar 17 2019, 1:02 AM

Move outside of generate_binary_resource

Getting closer:

Scanning dependencies of target generate-web
make[2]: generate_web_data.sh: Command not found
CMakeFiles/generate-web.dir/build.make:57: recipe for target 'CMakeFiles/generate-web' failed

Are you able to test this yourself?

guoyunhe updated this revision to Diff 54252.Mar 18 2019, 7:03 PM

Add ${CMAKE_SOURCE_DIR} prefix to script

Getting closer:

Scanning dependencies of target generate-web
make[2]: generate_web_data.sh: Command not found
CMakeFiles/generate-web.dir/build.make:57: recipe for target 'CMakeFiles/generate-web' failed

Are you able to test this yourself?

Now it should work. I forgot to add ${CMAKE_SOURCE_DIR} before the script name.

Could you use #eff0f1 and #31363b for the light and dark backgrounds since those are the colors we normally use for window backgrounds?

Could you use #eff0f1 and #31363b for the light and dark backgrounds since those are the colors we normally use for window backgrounds?

Like this?

guoyunhe updated this revision to Diff 54257.Mar 18 2019, 7:45 PM

Use typical Breeze background colors

Could you use #eff0f1 and #31363b for the light and dark backgrounds since those are the colors we normally use for window backgrounds?

Like this?

Yes

Thanks, it compiles with CMake now! \o/

I'm not a fan of how it dumps all this stuff in the source directory. Ideally it would put everything to the build directory when doing an out-of-source build so the source dir isn't cluttered up with transient and rapidly-changing information. Then you also wouldn't need to add these files to gitignore.

Thanks, it compiles with CMake now! \o/

I'm not a fan of how it dumps all this stuff in the source directory. Ideally it would put everything to the build directory when doing an out-of-source build so the source dir isn't cluttered up with transient and rapidly-changing information. Then you also wouldn't need to add these files to gitignore.

The challenge is that in HTML file, it uses certain relative paths to icons and data files. CMake build directory can be anywhere. I have to make sure icons, index.html, .breeze.txt always in same relative paths.

They should all go into the build dir IMO.

guoyunhe updated this revision to Diff 54532.Mar 22 2019, 6:20 AM

Move build files to CMake build directory

guoyunhe updated this revision to Diff 54533.Mar 22 2019, 6:32 AM

Add color blind mode

guoyunhe updated this revision to Diff 54535.Mar 22 2019, 7:47 AM

Add search function

pino added a subscriber: pino.Mar 22 2019, 8:46 AM

Few notes on the new generate_web_data.sh script:

  • please quote the paths properly, otherwise it will break when either the source directory or the build directory contain e.g. spaces
  • it does not seem using any bash-specific features, so make it using /bin/sh, to help non-Linux Unices (where usually bash is not available by default, and/or not in /bin)
  • please harden the script using at least -e and -u flags for set: this way, it will not keep executing when a command fails, and undeclared variables are not silently expanded to empty string (to prevent typos)
In D19812#436154, @pino wrote:
  • please harden the script using at least -e and -u flags for set: this way, it will not keep executing when a command fails, and undeclared variables are not silently expanded to empty string (to prevent typos)

Sorry, I am not very familiar with shell script. What does the -e or -u flags mean and how to use them? Can you give an example? Thanks.

guoyunhe updated this revision to Diff 54546.Mar 22 2019, 10:55 AM

Improve bash script

ngraham added inline comments.Mar 24 2019, 4:36 PM
generate_web_data.sh
8

exists -> exist

14

ditto

pino added a comment.Mar 24 2019, 4:54 PM
In D19812#436154, @pino wrote:
  • please harden the script using at least -e and -u flags for set: this way, it will not keep executing when a command fails, and undeclared variables are not silently expanded to empty string (to prevent typos)

Sorry, I am not very familiar with shell script. What does the -e or -u flags mean and how to use them? Can you give an example? Thanks.

  • -e: exits whenever any of the programs return a non-zero (i.e. failure) return code; this is useful to not silently ignore failures, and makes the behaviour similar to each line in a target of a Makefile
  • -u: immediately fails when trying to expand a variable that was not previously set; this way, things like mkdir "$DIR/foo" will immediately fail if $FOO was not set previously (so prevent misbehaviours due to typos, or code path not taken into account)
generate_web_data.sh
7

this should go to stderr, as it is an error

13

ditto

index.html
93

Always using the network is not exactly a good idea:

  • the page is unusable if there is no Internet connection
  • this (private!) website will be phoned home every time an user loads this page locally, without even notifying the user

At least in Debian I see a libjs-vue package, so please make sure to work with local copies only. Otherwise this is a big privacy concern.

guoyunhe updated this revision to Diff 54918.Mar 27 2019, 6:25 AM

Don't use remote JavaScript library

guoyunhe updated this revision to Diff 54919.Mar 27 2019, 6:35 AM

Output to stderr

guoyunhe marked 5 inline comments as done.Apr 10 2019, 2:35 PM

Hi all,

Can you check if here is anything else that needs to be changed? Thanks.

Hi @ngraham @pino @bcooksley , I have updated the script/configuration. Can you give some further opinion?

TBH I wonder how useful this actually is. Hopefully some of the other folks involved in making icons (@ndavis, @GB_2, @trickyricky26) can comment on whether or not this would be useful for them.

GB_2 added a comment.Apr 25 2019, 7:04 PM

I personally think this could be quite useful.

ndavis added a comment.EditedApr 26 2019, 12:13 AM

TBH I wonder how useful this actually is. Hopefully some of the other folks involved in making icons (@ndavis, @GB_2, @trickyricky26) can comment on whether or not this would be useful for them.

If I can link directly to a specific icon on that page, it can be useful for showing icons to others rather than only calling the icon by its name (e.g., list-add). Even if I can't link to an icon, Ctrl+f works.

What is this giant javascript file with no whitespace that you're adding? I really don't like how it's formatted in such a way that makes the code practically impossible to read. This would be the perfect place to sneak in malware (not saying you or the author have done this, but that this kind of obfuscated code is a perfect vector for it).

What is this giant javascript file with no whitespace that you're adding? I really don't like how it's formatted in such a way that makes the code practically impossible to read. This would be the perfect place to sneak in malware (not saying you or the author have done this, but that this kind of obfuscated code is a perfect vector for it).

It is Vue, a MVC library to make interactive single page application. I can provide some examples:

  1. WordPress https://github.com/WordPress/WordPress/tree/master/wp-includes/js
  2. Bootstrap https://github.com/twbs/bootstrap/tree/master/dist/js
  3. Phabricator (this website) https://github.com/phacility/phabricator/blob/master/webroot/rsrc/externals/d3/d3.min.js

They all have some minified JavaScript in Git repository.

filipf added a subscriber: filipf.Apr 26 2019, 5:56 PM

What is this giant javascript file with no whitespace that you're adding? I really don't like how it's formatted in such a way that makes the code practically impossible to read. This would be the perfect place to sneak in malware (not saying you or the author have done this, but that this kind of obfuscated code is a perfect vector for it).

I have to strongly second this sentiment. What's the source of the vue.min.js file? At the very least code shouldn't be formatted this way, and if this was the base file I'm seeing it's not in its original form: https://github.com/vuejs/vue/blob/dev/dist/vue.js

The code is stable version Vue.js. Source is here https://github.com/vuejs/vue/blob/v2.6.10/dist/vue.min.js
Uncompiled version is here https://github.com/vuejs/vue/blob/v2.6.10/dist/vue.js

I used to include it from remote URL. But someone think it is not trusted. So I changed it to local file.

Here are some KDE projects also including compiled JavaScript files https://cgit.kde.org/websites/docs-krita-org.git/tree/theme/static/js/modernizr.min.js

The documentation website of krita is a different type of repository. This project is going to be distributed and compiled by the distributions, and many of them will have to patch out the minified javascript and replace it with a dependency on a proper packaged version. The idea is that the minified (obscured) version is not the preferred form of distribution (not the "source code"), more or less. So special care should be taken here.

The documentation website of krita is a different type of repository. This project is going to be distributed and compiled by the distributions, and many of them will have to patch out the minified javascript and replace it with a dependency on a proper packaged version. The idea is that the minified (obscured) version is not the preferred form of distribution (not the "source code"), more or less. So special care should be taken here.

The web page is only used by developer/designer. End users don't need it. It won't be executed during packaging or installation. It won't be installed, either. Here isn't cmake rule to execute or install it.

At the minimum, I'd like to see the original/un-minified version rather than the illegible version here that's vulnerable to attack. But why do we even need it in the first place? The original is 12,000 lines of Javascript. Is it actually necessary to have such a thing just for building a simple webpage of icons? Seems like overkill to me.

guoyunhe abandoned this revision.Apr 26 2019, 7:35 PM

Using a library can save a lot of time. Just like many simple web pages use jQuery, D3, etc.

I don't have the ability to write it all by hands from scratch...

If you want to use a library, then it would be better to mark it as a dependency in CMake rather than bundling it here.