Skip to content

Conversation

@lkishalmi
Copy link
Contributor

Before we get too eager and start to throw 40+ svg-s into 100+ places, I think we shall really consider to have some icon directory.

In this PR I sketched the basics what I'm thinking about. This one is not about completeness. I have no idea what would be the methods of whatevericon, though I think it shall be able to load itself, probably with different sizes (as that's missing now)
Also the LegacyIcons and the GeneralIcon classes are just in the Utilities API module to time for the sketch,

Ideas?

@lkishalmi lkishalmi added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Sep 22, 2020
@neilcsmith-net
Copy link
Member

Thanks for taking a look at this. Although something feels backwards about the API. Adding new APIs (LegacyIconLocator) for old behaviour feels wrong. What I suggested before a number of times, including in discussion around introduction of SVG support, was adding in an API around something like IconLocator for the newer options. eg. we currently have a resource name which is a path. The existing behaviour handled png and gif files, with branding support. You'd have had an optional SVG loader which handled paths looking for related SVG files, and you could have a logical icon locator that translated non-path (no slash) resource names into icons. The icon locators could stack, so you could have first to resolve wins, or transformed locations from one pass down to the next.

The other API which we shouldn't ignore, and is already used in icon loading is ImageIO. I also suggested at the time we look at this for SVG using the TwelveMonkeys plugin. Amongst other things, ImageIO has the ability to specify target resolution, which I believe is something missing in our current support - we might use or at least adapt the API from that. eg. ImageReadParam

@ebarboni
Copy link
Contributor

Hi, just where would wait.png be at the end because you remove all wait.png and I'm not sure to follow.

@eirikbakke
Copy link
Contributor

I don't have strong opinions on exactly how icons are retrieved. I will happily copy SVGs around if necessary, or follow some other scheme. Exact copies of the same icon are not that hard to identify, and people don't update icons the same way they update source code, so there isn't much risk of different copies getting out of sync.

And I know @jtulach has pointed out that icons should not be collected all in one place for reasons of modularity--suddenly there's one location that every module depends on. The existing approach is certainly valid enough that I would not rename it to "LegacyXYZ". Perhaps a build time approach would be better for organizing the icons, keeping one copy of each distinct icon in a single folder and copying it to every relevant location at build time?

The situation is perhaps a bit different for platform apps, where it would be logical to be able to collect icon replacements in the branding module.

@lkishalmi
Copy link
Contributor Author

@ebarboni once the ImageUtilities.getIcon() was able to resolve the old locations to GeneralIcon.WAIT it is up to that object to decide where does it load the wait.png or maybe just a wait.svg . I have not worked out that part. Again this PR is not meant to be integrated. It even barely compiles.

@lkishalmi
Copy link
Contributor Author

@eirikbakke I would not say that collect all icons into a one big module. To load icons from the module resources is a completely legitimate use case. Especially for platform users. Introducing an API like this one would not take that away.
Yes, it would create a few runtime dependencies (depending how we would like the icons organized) for those modules which icon resource has been migrated. My question is. is it that bad? I do not know that answer, I just feel probably not.
If there would be a major hiccup, just present the case, I'm happy to learn.

On the other hand, these are the resources in Gradle Project module:
image

Probably the only module related icon would be the gradle one in different sizes. The others could come form a platform icon catalog or a java icon catalog. Would I have been happy to add a compile dependency and use those instead of copying them over? Yes, I would. Would I have considered add the Gradle icon into an existing icon catalog? Yes, I would.

As of the build time approach. It could definitely work, though that would be just a displacement activity, not a solution.

@lkishalmi
Copy link
Contributor Author

lkishalmi commented Sep 22, 2020

@neilcsmith-net We are not necessarily talking about different things. If I get it right, you propose to have a new API to locate SVG icons. In this proposal, that would be the GeneralIcon class serving as an icon catalog, and the WhateverIcon interface as the actual image retriever/handler and or metadata holder. The reason I have not dug deeper on that, is that I do not know the use cases and when I was working with Images in Java was 19 years ago. So someone else needs to bring his/her knowledge on the table.

The LegacyIconLocator is just the other side of the bridge. It would allow old module code to work with the new icons, at the cost of introducing a runtime dependency.

@ebarboni
Copy link
Contributor

Thanks for clarification,

Reorganization of the icon in catalog(s) make sense to me. I'm not sure it will solve the duplicate. You may have gradle catalog with all the icon inside just to simplify usage for extension.

To me having a "ant bootstrap" copying all the icon from an folder in nbbuild to their final place resolve the duplicate issue.
Having in this "icon folder" some tools and organization to help designer (not sure what they needs :p)
transformer script svg-> 16x16 24x24, album view

@eirikbakke
Copy link
Contributor

Having in this "icon folder" some tools and organization to help designer (not sure what they needs :p)

Designers don't use git in the first place. Easier for them to work with a spreadsheet.

@lkishalmi
Copy link
Contributor Author

Well, some discussion is going on. I'd summarize it here as well. So we are kind of agree that a kind of catalog API could be generated using a custom annotation. Supporting replacing the existing icons modifying the implementation of getIcon() is not a priority, so that is Ok as it is. I'm closing this PR now. Let's continue on the dev list from now.

@lkishalmi lkishalmi closed this Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge Don't merge this PR, it is not ready or just demonstration purposes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants