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

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 10189
Build 10207: 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.Fri, Mar 22, 6:20 AM

Move build files to CMake build directory

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

Add color blind mode

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

Add search function

pino added a subscriber: pino.Fri, Mar 22, 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.Fri, Mar 22, 10:55 AM

Improve bash script

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

exists -> exist

14

ditto

pino added a comment.Sun, Mar 24, 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
8

this should go to stderr, as it is an error

14

ditto

index.html
94

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.Wed, Mar 27, 6:25 AM

Don't use remote JavaScript library

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

Output to stderr

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

Hi all,

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