apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.64k stars 849 forks source link

Donate more SVG icons, and install more duplicates #7463

Closed eirikbakke closed 3 weeks ago

eirikbakke commented 4 months ago

This PR adds scalable SVG versions of a few more NetBeans icons, for Retina/HiDPI screens, originally drawn for the Ultorg project. Some of the existing icons are also copied to new locations that were discovered to be visual duplicates of the same original bitmap images. Part of NETBEANS-2617.

See this HTML page table for a complete summary of icons in the NetBeans codebase as of today and this PR, with identified duplicates and mappings to SVG files.

Details:

The following composite of screenshots shows a few examples of new SVG icons: 240612 New NetBeans Icons

This PR adds or touches SVG files only, and includes no code changes. The Adobe Illustrator file that was used to generate the SVG files will be committed in a separate PR (here), along with the script that was used to find duplicates, copy the files to the right locations, and generate the HTML page summary.

mbien commented 2 months ago

lets move this to NB24

eirikbakke commented 2 months ago

Sure, no hurry!

mbien commented 2 months ago

Sure, no hurry!

would have been cool if this could have made it in, but nobody looked at it so far and it is a large PR. I have also only 2x 1080p screens here so I can't simulate scaling very well.

Maybe split this into two commits, one for modified icons one for added? The added icons are not used at the moment, right?

matthiasblaesing commented 2 months ago

The simplest way to test would most probably to just create a dev build and use it for a few days. And yes, I should have done it sooner.

I don't think it is necessary to check each icon. We have much more invasive changes with less review.

mbien commented 2 months ago

We have much more invasive changes with less review.

sure but those are also the PRs which often get reverted (and I hate doing that!). I think this here is actually reviewable, this looks like half of the changes are new icons. So looking through the modified icons quickly should be possible with the right tool or process. Humans can interpret pictures quite fast. (gh is not the right tool as you can see)

The simplest way to test would most probably to just create a dev build and use it for a few days.

agreed. We could merge this shortly NB 23 is done, this would give enough time to find issues if there are any before NB 24.

matthiasblaesing commented 2 months ago

sure but those are also the PRs which often get reverted

This is off-topic, but our rate of reverted PRs is low. The core problem from my POV is, that people don't use their own changes, else they would see the problems they introduce. In this case these icons were used in a real product and sure, shit happens, but lets assume good faith.

eirikbakke commented 2 months ago

The added icons are not used at the moment, right?

They are actually used: ImageUtilities.loadImage automatically loads any SVG file that exists with the same name (but different extension) as the requested GIF or PNG icon. (So I wouldn't split up into two commits if this was the reason for doing so.)

So looking through the modified icons quickly should be possible with the right tool or process. Humans can interpret pictures quite fast.

This is what the HTML page generated by this script is for: https://people.csail.mit.edu/ebakke/misc/netbeans-icons-240612.html

The simplest way to test would most probably to just create a dev build and use it for a few days.

I've been using a NetBeans IDE build with these changes applied for a month now. Others are welcome to do the same, of course.

I have also only 2x 1080p screens here so I can't simulate scaling very well.

For testing purposes, just seeing that icons are not "out of whack" (obviously missing, errors loading etc.) is probably good enough. The new icons are used at 100% scaling, too, just at a lower resolution.

mbien commented 2 months ago

@eirikbakke thanks for the info. The generated page is super useful.

How does NB render the vector graphics atm, with batik?

I think they look good, but you can definitively see the slight "style" difference if there is a svg/png mix in the same toolbar. But I saw that you already prioritized neighboring icons which is great. somevectoricons (left is NB22, right is this PR)

eirikbakke commented 2 months ago

How does NB render the vector graphics atm, with batik?

Yes, with Batik, originally introduced in this PR. Alternative approaches, such as storing each icon as a bitmap at 2x size, could be considered in the future, if we want to get rid of this rather heavyweight library.

I think they look good, but you can definitively see the slight "style" difference if there is a svg/png mix in the same toolbar.

Yeah, I tried to establish a style that made the mix between old and new icons reasonably tolerable. The main difference is the higher resolution in the newer icons, as well as the removal of bevels and shadows (which are hard to draw in vector graphics and are out of fashion for the same reason).

neilcsmith-net commented 2 months ago

... if we want to get rid of this rather heavyweight library

Or we consider JSVG like FlatLaf and IntelliJ. We need to consider whether Batik is now a platform API, or whether just the ability to load from an SVG is (with potential for minor incompatibilities in changing implementation).

neilcsmith-net commented 2 months ago

I was in favour of merging this for NB23, which was why I added the milestone and dev build. I also didn't have time to review it properly before freeze either! So NB24 is fine IMO.

@eirikbakke please put milestones on PRs and push people for review if necessary - I noticed a bunch of useful platform updates from you, like this, that were getting stale.

mbien commented 2 months ago

I also didn't have time to review it properly before freeze either! So NB24 is fine IMO.

I think we could merge this pretty soon (e.g after NB23rc1 and the version bump), since it is not very likely that icons will cause conflicts between delivery/master. The benefit of pushing this to NB24 is also that maybe we get a few more icons in to reduce the problem of https://github.com/apache/netbeans/pull/7463#issuecomment-2251774844 (followup PRs).

mbien commented 2 months ago

so, should we merge this? Maybe rebase first to latest master to make the merge commit smaller?

cc @ebarboni

eirikbakke commented 2 months ago

Or we consider JSVG like FlatLaf and IntelliJ.

Yes, that should be possible. Some visual comparisons would have to be made to ensure that the rendered SVGs look the same when rendered with a different library. (I assume the lighter libraries implement a smaller subset of the SVG standard than Batik.)

We need to consider whether Batik is now a platform API, or whether just the ability to load from an SVG is (with potential for minor incompatibilities in changing implementation).

The SVG loader was intentionally made into an optional and pluggable ServiceProvider module, implementing org.openide.util.spi.SVGLoader in the openide.util.ui module. The current implementation, based on Batik, is in the optional openide.util.ui.svg module.

please put milestones on PRs and push people for review if necessary - I noticed a bunch of useful platform updates from you, like this, that were getting stale.

OK, will do this more in the future.

The benefit of pushing this to NB24 is also that maybe we get a few more icons in to reduce the problem of https://github.com/apache/netbeans/pull/7463#issuecomment-2251774844 (followup PRs).

I think there will always end up being a mix of old and new icons, due to the sheer number of them (it's not just the Test Results tab, but also Search Results, Notifications, Output + all the debugging ones etc.). But getting work gradually merged in makes it easier to do incremental work later.

eirikbakke commented 2 months ago

Thanks for testing!

I noticed in at least one occasion that an icon got a different visual (the "earth" icon for web) and at first I had a double take on it, but that smoothed and I'm currently happy with this.

In this example it was still an "earth" icon before, right, not a different logo of some kind? (My Bezier curve drawing skills are somewhat limited... so the new earth may have had a somewhat improvised landmass.)

-- Eirik

matthiasblaesing commented 2 months ago

I noticed in at least one occasion that an icon got a different visual (the "earth" icon for web) and at first I had a double take on it, but that smoothed and I'm currently happy with this.

In this example it was still an "earth" icon before, right, not a different logo of some kind? (My Bezier curve drawing skills are somewhat limited... so the new earth may have had a somewhat improvised landmass.)

Actually it was the color that triggered my reaction. Before it was blue-white (left), now it is blue-green (right):

image

Both tell the right story and with blue selection highlight, the earth with green continent has a nicer visual than the white.

TL;DR: This was just a random observation and not intended to spark a discussion :smile:

eirikbakke commented 2 months ago

Great, yes, I just wanted to make sure there were no errors in the filename-to-artboard mapping logic.

Old and new icons may look different in cases where there are many previous variations of the same motif. E.g. there were many different "globe" variations (imageimageimageimage...), and these all got consolidated into a single new icon (image).

weisJ commented 2 months ago

Or we consider JSVG like FlatLaf and IntelliJ.

Yes, that should be possible. Some visual comparisons would have to be made to ensure that the rendered SVGs look the same when rendered with a different library. (I assume the lighter libraries implement a smaller subset of the SVG standard than Batik.)

Creator of JSVG here. If you aren’t using any extremely complex filters or CSS declarations, JSVG should support all features you need. Though if you experience there is something missing feel free to open an issue. If I know what needs to be done I can certainly prioritise things.

eirikbakke commented 2 months ago

@weisJ Oh, thanks for dropping by! These SVGs are generated by Adobe Illustrator, I can test them out with JSVG at some point. There are some SVG export settings that can be adjusted in Illustrator if necessary, too.

matthiasblaesing commented 3 weeks ago

@eirikbakke I think this would the right time to merge this, as we are early in the cycle and at least my runs in the last few weeks at work looked clean. PRs don't get better when the are just lying around.

eirikbakke commented 3 weeks ago

@matthiasblaesing Great, merged! Thanks for the useful discussions.