Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ikonli FontAwesome pack as a source for IMAGE in NAVIGATOR #339

Closed
wants to merge 3 commits into from
Closed

ikonli FontAwesome pack as a source for IMAGE in NAVIGATOR #339

wants to merge 3 commits into from

Conversation

ValentinChirikov
Copy link

@ValentinChirikov ValentinChirikov commented Mar 5, 2020

Hello !
I propose to use font's pack as IMAGE sources.

Font packs originally created by kordamp (see https://github.com/kordamp/ikonli). I switched it to maven and modified a little bit to produce Image (see FontIcon getImage() method https://github.com/ValentinChirikov/ikonli-maven-java8) that ImageIcon constructor consumes (see NavigatorElement setImage method).

To produce a build ikonli-maven-java8 must be cloned and artifacts pushed to repo.

Then it could be called from LSF :
NAVIGATOR {
NEW MainForm FIRST;
NEW Teams IMAGE 'fa-users' AFTER MainForm;
}

Аннотация 2020-03-05 115120

The cheat-sheet for icons http://kordamp.org/ikonli/cheat-sheet-fontawesome.html

If You like idea please give some feedback. Thanks.

@AlexKirkouski
Copy link
Contributor

Maybe I'm doing something wrong, but there is no 11.3.5-8 in central repository only 11.3.5. And even in this version there is no getImage method for the FontIcon class.

However the idea is pretty interesting, but the question what is the size of that library to embed it in
the app-server. I mean if there are very many icons there can be significant overhead, if not it may be not that useful.

@ValentinChirikov
Copy link
Author

ValentinChirikov commented Mar 5, 2020

Maybe I'm doing something wrong, but there is no 11.3.5-8 in central repository only 11.3.5. And even in this version there is no getImage method for the FontIcon class.

Please clone https://github.com/ValentinChirikov/ikonli-maven-java8.git branch maven-java8, then issue mvn install

but the question what is the size of that library to embed it in

for FontAwesome now its is around 600k, but i think that size could be reduced (there are font resources that not used, as i see). I removed unused resources {svg, eot, woff, woff2} and now it is 150k total.

@AlexKirkouski
Copy link
Contributor

AlexKirkouski commented Mar 6, 2020

Please clone https://github.com/ValentinChirikov/ikonli-maven-java8.git branch maven-java8, then issue mvn install

In that case we'll need to keep that fork in our local repository (with auto-build and so on). It's not that huge problem, but in general it's not very desirable (actually we wanna get rid of all dependencies not from central repository to put lsfusion there). The question what have you patched there? There are a lot of methods movements there, so it's pretty hard to understand what exactly you've changed.

If it's only FontIcon.getImage (because buffer is private) it can be workarounded with Java Reflection (pretty common trick). Or is there something else?

@ValentinChirikov
Copy link
Author

ValentinChirikov commented Mar 6, 2020

If it's only FontIcon.getImage (because buffer is private) it can be workarounded with Java Reflection (pretty common trick). Or is there something else?

One thing is that i changed required platform to Java 8 (it is 11 in original repo), the other one is FontIcon.getImage. If Java 11 is acceptable for lsfusion platform - i will create a pull request to original ikonli repo to include getImage method for FontIcon class, imho it could be better than use reflection. What do you think ?

@AlexKirkouski
Copy link
Contributor

Well I don't think that using ikonli would be enough reason to remove Java 8 support for the whole project.

Of course it is possible to rebuild ikonli with Java 8, just like you did (as far as i get, since we are not using Java FX (kordamp/ikonli#44) it should be ok). However, as I've mentioned, in that case we have to maintain that dependency ourself in our local repository. We have some that "patched" dependencies, but it's not the reason to have one more. Also we use 2-colored icons by default and ikonli supports only one color.

But still I like the idea of having some predefined "iconpack" embedded into the platform. As far as I understand ikonli is the only that kind of library in central maven repository (at least I haven't found any other library). So we'll need to think a little bit about all this.

And can you provide more "clean" ikonli fork without this method movements? By the way, I believe that FontIcongetImage is not synchronized right (return buffer should be inside synchronized block). No big deal (because I'm not sure that multithread usage is important for FontIcon), but still.

@ValentinChirikov
Copy link
Author

ValentinChirikov commented Mar 8, 2020

And can you provide more "clean" ikonli fork without this method movements? By the way, I believe that FontIcongetImage is not synchronized right (return buffer should be inside synchronized block). No big deal (because I'm not sure that multithread usage is important for FontIcon), but still.

Sure, i'll clean-up on my spare time. I do agree with you about "patched" dependencies, i don't like it either, but sometimes it is the only way (or we could include sources within project, but i think that it won't look any better).

Valentin added 2 commits March 11, 2020 09:25
…ditional BufferImage object and one additional call of image drawing method
@ValentinChirikov
Copy link
Author

ValentinChirikov commented Mar 11, 2020

Hi. I cleaned ikonli fork, so now it is as close to source as possible, without any additional methods & patches. But there is a cost of "un-patched" fork. It is one additional BufferedImage instantiation and one additional drawing method call for one icon. But i think, it is reasonable cost - it is not that hard to update ikonli fork if needed (with something like "git diff --name-only master - icon-packs / ** / kordamp / ** / *. Java | xargs git checkout master "), and it is acceptable until there will be millions of icons creation, which obviously won't happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants