In D19812#432326, @ngraham wrote:

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?

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.

In D19812#432348, @bcooksley wrote:

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

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?

In D19812#432424, @ngraham wrote:

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?

In D19812#433916, @ndavis wrote:

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?

In D19812#433941, @guoyunhe wrote:

In D19812#433916, @ndavis wrote:

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.

In D19812#434089, @ngraham wrote:

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.

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.

In D19812#436174, @guoyunhe wrote:

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)

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.

I personally think this could be quite useful.

In D19812#455478, @ngraham wrote:

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).

In D19812#456848, @ngraham wrote:

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.

In D19812#456848, @ngraham wrote:

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.

In D19812#456869, @ltoscano wrote:

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.

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.