SVG icon support

[ajmeese7]

This PR allows themes to include icons of type SVG, PNG, and GIF, the three primary formats used for images with transparency. The code makes a request to check if the default image type .png exists first, and if not it then checks for .svg and .gif images, respectfully. The styles in _search.scss were updated to enforce a standard size on SVG files, which could show up much larger than expected otherwise.

I chose this method because the proposed solutions in #150 were either:

1. Complicated and involved a lot of JSON configuration, which is an unnecessary barrier for users interested in introducing new themes
2. Didn't allow for mix-and-match of icon formats, which is something that is worth considering (I have had to introduce a .png in a theme because there was no equivalent .svg in the theme I was modifying)

My solution is not perfect, but it functions as intended. Reviews and critiques are of course always welcome :)

---

Closes #150 Refs #172

[ajmeese7]

I do not believe the failing unit tests have anything to do with the code that I introduced. However I can break the code out of the nested function into a separate function for CodeClimate if desired, that wouldn't be a problem.

[andersevenrud]

I do not believe the failing unit tests have anything to do with the code that I introduced.

Huh. It fails on tests that don't use any network interactions, so that's weird. Haven't seen this. Tried to re-run, but same result.

Going to try again in a while in case it's just something on Github's end. All of the new commits in this repo went through just fine.

[andersevenrud]

Huh. It fails on tests that don't use any network interactions

Actually, heh, it does. A icon() call is made in there :)

I do not believe the failing unit tests have anything to do with the code that I introduced

So the chances are that's the case after all. Probably because no HTTP server runs in the tests. Which is where you can see how this would break in the cases of standalone wo/server.

[andersevenrud]

Maybe with a simple webpack plugin supplied in dev-meta ? It's possible to get a list of every single file that went into a bundle. If list is not available, fall back to png.

An alternative would be just to provide a simple CLI (in dev-meta ideally since you don't usually have osjs-cli in your packages) utility where you gave a directory and it would update the metadata file with everything it found :man_shrugging:

[andersevenrud]

NB: Updated your PR description and corrected the issue reference syntax so that when this goes through things gets closed as intended.

[ajmeese7]

Maybe with a simple webpack plugin supplied in dev-meta ? It's possible to get a list of every single file that went into a bundle. If list is not available, fall back to png.

Good idea! Any chance you have an example handy of how you would accomplish this / could give some ideas of how to create and implement?

An alternative would be just to provide a simple CLI (in dev-meta ideally since you don't usually have osjs-cli in your packages) utility where you gave a directory and it would update the metadata file with everything it found

Same question for this suggestion :)

[andersevenrud]

Good idea! Any chance you have an example handy of how you would accomplish this / could give some ideas of how to create and implement?

Probably find what you need here (except for things that changed between webpack 4 and 5):

https://webpack.js.org/contribute/writing-a-plugin/

But the gist is, tap into the compilation, get the list of assets, figure out which ones are the icons (usually pretty easy because of path patterns).

Same question for this suggestion :)

Just do a glob (and then same as above), really. Then write it to the metadata file.

[andersevenrud]

As for providing this stuff:

It could just be a import plugin from '@osjs/dev-meta/themePlugin or something like that.

The simpler script method could just be stored in a src file, then add it as bin in package.json . Then user can run npx generateThemeMetadata <path> or something like that.

[ajmeese7]

@andersevenrud the metadata file is not included in the dist, but I just wrote a webpack plugin that generates a JSON file (iconTypes.json unless specified otherwise) mapping the image name to the file type like the following:

{
  "mail-reply-all": "svg",
  "mail-reply-sender": "gif",
  "mail-send": "png",
  "media-eject": "svg",
  ...
}

How to you recommend reading from the JSON file in the client to determine what image type to attempt to load? Do you just want all icon() function calls to have await and the function converted to async with fetch, or do you have a better idea? Not sure if I can import the JSON file or if it has to be requested.

[andersevenrud]

the metadata file is not included in the dist

Correct. Not in dist, but it's read when you do package discovery so that the client gets access to whatever is in the file :)

[andersevenrud]

but it's read when you do package discovery so that the client gets access to whatever is in the file :)

Just to elaborate a little bit here. When you do package discovery, all packages with the osjs stuff in package.json is picked up and stored into an array of paths.

These paths are then used to look up a metadata.json file in the same roots.

All of the metadata files are then merged together and written into the distro, and client reads it when it starts up.

---

Oh, and the resulting manifest can be imported into the client configuration to statically build all of this (for file:// standalone mode etc.).

[ajmeese7]

Correct. Not in dist, but it's read when you do package discovery so that the client gets access to whatever is in the file :)

Like I said, I went with the webpack plugin route that you suggested, not the cli method. Manipulating manifest.json is not an option with that route, since it is not one of the assets that webpack is copying to the dist folder.

That's why I asked the second portion:

How to you recommend reading from the JSON file in the client to determine what image type to attempt to load? Do you just want all icon() function calls to have await and the function converted to async with fetch, or do you have a better idea? Not sure if I can import the JSON file or if it has to be requested.

Sorry for any confusion.

[andersevenrud]

is not an option with that route, since it is not one of the assets that webpack is copying to the dist folder.

I assume that Webpack's compiler asset output API thingy don't support (absolute) paths.

But can't you:

1. Provide a path in the plugin options that points to the metadata file (maybe there's a way to detect the root path even)
2. Tap into the output and place detected stuff into a state in the plugin class
3. When webpack is done, trigger on a "done" event and place the state above into the given metadata file path

[ajmeese7]

I haven't tested with the dev-meta repo yet since I'm currently developing on my fork, but I was able to get that working in a single package via a relative path, so it should work when abstracted out.

That leads to two questions:

1. How would I tap into that metadata in the icon function, given access to the path and the theme as there is currently?
2. Do you still want the default behavior to point to a .png even if it is known to not exist, or do you want null returned as it is in the soundResource function?

[andersevenrud]

How would I tap into that metadata in the icon function, given access to the path and the theme as there is currently?

From the top of my head, something like this:

• Create the method Packages#getMetadataFromName that returns a lookup from this.metadata (return a copy here)
  • In createPackagesContract in the core provider, add this function
  • Also update the d.ts if you find the time
• You have the getThemeName there, which is a packages name
• Use the new function to get the metadata based on this name
• Now you can resolve the icon filename based on the metadata if available
  • If not, fall back to png extension

Do you still want the default behavior to point to a .png even if it is known to not exist

Yes, same fallback. The sound resolver returns null because sound themes can be disabled, whereas here that's not possible :)

[ajmeese7]

Changes implemented as requested, I'll open the PR on dev-meta so you can test that things work on your end.

[ajmeese7]

I know these aren't the most comprehensive tests known to man, but for now I believe they get the job done. I would be happy to add more if there are additional aspects that you would like tested.

Update: My bad, forgot to run the tests to make sure there were no linter copy errors. Fixing now.

[andersevenrud]

forgot to run the tests to make sure there were no linter copy errors

Probably should add Husky or something like that to the core repos to prevent this sort of stuff.

[ajmeese7]

Alright now I'm confused, do you want a mix of tabs and spaces in the file ? In this comment you pointed out the indentation changes made by the linter, so in this commit I standardized the file to the space indentation that you normally use.

What do you want done to the file, do you want it left as spaces, changed to tabs, or reverted back to a random mix?

[andersevenrud]

Totally missed that comment.

Ideally, just add the updates to .d.ts then take care of formatting in a separate PR so that this does not go out of context and is blown up with 2000+ lines of change :sweat_smile:

Edit: If you do an interactive rebase and split out the commits so that we get a precise history then we could do it here because then I don't have to do a squash. It's usually a bit of a chore though, so it's easier just to revert :)

Edit again: Or hey, just do a new PR now with the formatting, then you can rebase onto it and the 2000 lines of change here just disappears by themselves :D

[andersevenrud]

Oh, I see you did a merge, and not a rebase. So this won't really work as expected... but a squash will brush all the stuff under a rug :D

With a rebase you would be able to "erase" the changes (history) from the bottom up, but a merge effectively does nothing to the history as it's on top.

[andersevenrud]

With a rebase you would be able to "erase" the changes

Assuming you're in your branch you could do something like this:

I haven't tested this with your PR or whatever...but this is generally how it's done.

# Update your master branch
git fetch origin master:master

# Start rebasing
git rebase master

# Now you'd hit a collision because there was changes to the type defs on both sides, so to resolve
# reset to the master file
git checkout --ours index.d.ts

# Now you can add your changes to the file, then
git add index.d.ts

# Finish up
git rebase --continue

Voila. The commit that had all of the changes to the file now only has the ones you initially was going to add.

And it can be force-pushed to the repo and the PR will reflect this :raised_hands:

[ajmeese7]

@andersevenrud reminder to check this out locally when you get a chance, no rush :)

[andersevenrud]

Sorry about the delay. Life got in the way :/ I'll get this merged this weekend.

[andersevenrud]

Gave it a go and it seems to be working fine :)

[andersevenrud]

This and all your other merged stuff was released under 3.9.0.