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

Preview SVG files #7938

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

Chris2011
Copy link
Contributor

The PR will preview SVG files inside the IDE with MultiViewSupport

image

I already add actions that make sense to the SVGDataObject as for XML files like:

  • View (Opens in browser)
  • Check XML and Validate XML
  • Reload Document (Disabled atm unfortunately)

I have 2 problems faced here:

  1. I can't find an implementation for Reload Document. Adding the action to mf-layers.xml file isn't enough without having the cookie set in the class as you can see for View, CheckXML and ValidateXML.
  2. I have problems showing embedded images with relative paths:
<svg xmlns="http://www.w3.org/2000/svg" width="300" height="200">
    <image href="icons/vue.png" x="10" y="10" height="180px" width="280px"/>
</svg>

For problem 1. reload document for XML files were implemented, when there was an assumption that some references can change and the IDE can't recognize them. I really don't know whether I need this action or still this action is relevant for SVG. If you have a hint please lemme know.

For problem 2. I didn't want to add custom logic but I need to think about to handle this. Also I don't know whether batik can handle external resources as relativ paths. I have another image with an base64 encoded image and this is working just fine, but not to load from the path. VS Code and the browser can handle it quite well.

I got the image from https://commons.wikimedia.org/wiki/File:SVG_Logo.svg it is CC-BY-SA-4.0 I used it, converted it to PNG and will now use Apache, is this correct?

Otherwise CC-BY-SA-4.0 is missing in nbbuild/licenses folder. Does that means, that Apache 2.0 is not compatible with CC-BA-SA-4.0 or just not up to date?

@Chris2011 Chris2011 added the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Nov 5, 2024
Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing to watch out for is that we might switch from batik to JSVG #7463 (comment) at some point

not sure if anyone has this on the radar atm but it would be good to keep batik specific usages low.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious if the paperwork job catches this since you committed two jars by accident

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, I see. I dunno why this happened. I thought this was just due to make them public to use in another ant module but it seems not. I will change this and remove this.

@Chris2011
Copy link
Contributor Author

one thing to watch out for is that we might switch from batik to JSVG #7463 (comment) at some point

not sure if anyone has this on the radar atm but it would be good to keep batik specific usages low.

Thx for the hint, I didn't noticed that. When this happens, of course I'm willing to switch to this. For now I would stick with batik if this is ok.

@mbien mbien added Upgrade Library Library (Dependency) Upgrade Editor Platform [ci] enable platform tests (platform/*) labels Nov 5, 2024
@neilcsmith-net
Copy link
Member

one thing to watch out for is that we might switch from batik to JSVG #7463 (comment) at some point
not sure if anyone has this on the radar atm but it would be good to keep batik specific usages low.

Thx for the hint, I didn't noticed that. When this happens, of course I'm willing to switch to this. For now I would stick with batik if this is ok.

IMO, the two things don't need to be related. Possibly switching to JSVG (or build time generation) for icons in the core platform doesn't necessarily mean we don't keep Batik around as an autoload library elsewhere in the IDE for other purposes. In line with that, I would look at adding the additional Batik libraries required for this separately in the ide cluster rather than platform cluster. The size of Batik already caused enough of a concern in #1278 - let's not add to that.

As and when anyone does make the icon loading use something else, all of the Batik libraries (along with a couple of dependencies that were previously moved), could move to the ide cluster?

I like the general idea of providing this support in the IDE incidentally.

cc/ @eirikbakke

@Chris2011
Copy link
Contributor Author

@neilcsmith-net thx for the info. So as I understand it correctly, I should add those required libs to the XML module where I added the SVGDataObject, instead of the batik ant module, right?

@neilcsmith-net
Copy link
Member

I'd consider adding a libs.batik module in the ide cluster that relies on libs.batik.read in the platform cluster. If we ever switch to something else for icons, all the external libraries could end up in the former. But let's see what @mbien or @eirikbakke or anyone else think on that approach before you put time into it!

@eirikbakke
Copy link
Contributor

eirikbakke commented Nov 6, 2024

Thanks for this PR!

Possibly switching to JSVG (or build time generation) for icons in the core platform doesn't necessarily mean we don't keep Batik around as an autoload library elsewhere in the IDE for other purposes.

I agree with this. I think it's OK to rely on Batik for IDE features that actually relate to SVG files, like this one. Batik probably supports a larger part of the SVG spec than the lighter-weight alternatives.

Whereas for NetBeans icons, it could make sense to switch to a lighter library, which just needs to work for the particular SVG files that are used for NetBeans icons.

@mbien
Copy link
Member

mbien commented Nov 7, 2024

IMO: if batik is supposed to stay we shouldn't switch to JSVG. The main motivation there would be to reduce dependencies (and potentially also become more resource efficient due to the changed impl). If batik is already there anyway, adding another lib just for icons is making it worse not better.

In other words: replacing batik with jsvg would be something worth exploring IMO, maintaining a lib wrapper for jsvg additionally to batik should only be done if it is also demonstrated that there is a noteworthy memory and/or performance benefit in practice.

In reality you probably want to see how svgs look in a browser not in swing, having preview for it is more of a convenience thing. If it wouldn't support some SVG features it might be still ok? NB will never be inkscape ;)

@eirikbakke
Copy link
Contributor

@mbien Good point, two libraries is kind of awkward...

In reality you probably want to see how svgs look in a browser not in swing, having preview for it is more of a convenience thing. If it wouldn't support some SVG features it might be still ok? NB will never be inkscape ;)

Probably true. (I don't know exactly what the limitations with jSVG are compared to Batik.)

@troizet
Copy link
Collaborator

troizet commented Nov 7, 2024

In reality you probably want to see how svgs look in a browser not in swing, having preview for it is more of a convenience thing. If it wouldn't support some SVG features it might be still ok? NB will never be inkscape ;)

It would be really nice if there was an option to preview svg in the navigator, similar to other images:
image_navigator_preview

@Chris2011
Copy link
Contributor Author

@troizet good catch, will check this too.

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Nov 7, 2024

If batik is already there anyway, adding another lib just for icons is making it worse not better.

I think these are mostly unrelated concerns, and probably don't need to be discussed on this PR ...

maintaining a lib wrapper for jsvg additionally to batik should only be done if it is also demonstrated that there is a noteworthy memory and/or performance benefit in practice.

This is the only reason for switching for icons at all IMO. JSVG claims a ~98% less memory usage than Batik. We're not sure if this is true in practice or whether we have an issue as yet. If as more SVG icons are added we start to see startup time or memory concerns, then JSVG might be a way forward. As might be compile-time generation of bitmaps and/or Java classes. FlatLaf has icons that are Java coded mirrors of SVG, and use of Radiance's converter could also be a consideration.

Libraries to enable SVG previewing only need to be loaded if the user edits such a file. Therefore there are different considerations. Accuracy might trump overheads. If we did switch to JSVG for icons, and if we felt JSVG was good enough for previewing, that would be the time to consider whether we need one library or two. The key thing here, for me, is whether we want the feature?

At the moment, if we need to add additional Batik libraries purely for previewing SVG files, then I'd like to see them added separately (ie. in the IDE cluster) to the base library support for icons so we at least don't increase the weight of things in the base platform cluster.

@mbien
Copy link
Member

mbien commented Nov 7, 2024

@neilcsmith-net

I think these are mostly unrelated concerns, and probably don't need to be discussed on this PR ...

the reason why I brought this up at all is because thinking a bit ahead can sometimes save some work.

This is the only reason for switching for icons at all IMO. (...)

Having the option to trade ~17 jars against one sounds like a good trade all by itself (esp in context of how dependencies are managed in the NB build) - even if jSVG would make no measurable difference.

@eirikbakke

@mbien Good point, two libraries is kind of awkward...

yep. Adding a dependency with overlapping functionality of an existing dependency should have a higher barrier.

Probably true. (I don't know exactly what the limitations with jSVG are compared to Batik.)

same, I didn't take a look how JSVG compares to batik since I don't know batik very well, the readme of JSVG does list its supported and not supported features though. The failure mode would be also interesting since we could render a open-in-system button or something like that if it sees animation etc.

Overall I don't really mind adding more batik jars if it is well motivated. But if we are going to remove them two weeks from now again it would be kinda awkward.

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Nov 7, 2024

Having the option to trade ~17 jars against one sounds like a good trade all by itself (esp in context of how dependencies are managed in the NB build)

Sure, I made that point when it was introduced in the first place (and pretty much the same as above) - #1278 (comment) and #1278 (comment)

If number of jars is the primary concern here, let's just ship batik-all.jar! Anyway, a better development for icons might be batik at compile time, not run time.

EDIT: actually, the size differential of shipping batik-all over the subset is a lot lower than I thought (~400kb) - this might actually be a good idea. And I wonder what the overhead of using 17 files vs 1 is?

@eirikbakke
Copy link
Contributor

I implemented the changes needed to switch ImageUtilities from Batik to JSVG in #7941 . I'll need to test it for a while to see that all icons appear correctly etc., including in my own NetBeans Platform app. (Out of time for today...)

@matthiasblaesing
Copy link
Contributor

matthiasblaesing commented Nov 7, 2024

I think having SVG preview is great. It is stupid to carry an SVG rendering library (be it Batik or JSVG) and not integrate it to render SVG. Having rendering identical to browser will be hard, as it seems, SVG goes the same way as HTML (i.e. chaotic support and development), so a library not aiming for perfection, but pragmatic rendering might even be the best option.

The rendering issue of images can be explained as this

<svg xmlns="http://www.w3.org/2000/svg" width="300" height="200">
    <image href="icons/vue.png" x="10" y="10" height="180px" width="280px"/>
</svg>

is neither SVG 1.1 nor 1.2. Both require the reference to be xlink:href and that works beautifully. In fact inkscape writes that by default.

For the preview I think being able to scale would be good, as implemented in the ide/image module (Screenshot shows example image with toolbar):

grafik

In that module there is also an ImageNavigatorPanel and ImagePreviewPanel that could be an inspiration for SVG support.

I'm not sure, that I see the SVG support inside the XML module, it could also be in image or independent.

@Chris2011
Copy link
Contributor Author

Yes, I thought about adding scaling stuff and some other stuff into it and I also want to implement this, but not for this MVP. But at the end, we can leave this open until I:

  • Implemented preview for the navigator panel
  • Implementing the other stuff for scaling
  • Move this stuff to SVG or an own package

Thx for the hints all.

@matthiasblaesing
Copy link
Contributor

This https://www.apache.org/legal/resolved.html#cc-sa indicates to me, that we should keep away from "CC-SA" licensed media. This might be an alternative (CC-0 licensed): https://www.svgrepo.com/svg/255830/svg. Alternatively, maybe @eirikbakke could be persuaded to draw an icon?

@eirikbakke
Copy link
Contributor

@matthiasblaesing Sure, here's an icon you're free to use:

fileSVG

It's in the style of other NetBeans file type icons, which are generally of the form "tiny art inside a piece of paper with a folded corner". The "art" was drawn freehand as a hint to the SVG logo.

image

@matthiasblaesing
Copy link
Contributor

@eirikbakke thank you!
@Chris2011 thats one problem solved ;-)

@Chris2011
Copy link
Contributor Author

Chris2011 commented Nov 9, 2024

I dunno whether everyone knows that little indication so isn't it possible to put the whole SVG icon inside this "file" but that's not that important, I'm fine with everything which doesn't look like the XML icon :). @eirikbakke thx for the suggestion.

Also and out of curiosity, For me it looks like a bit of a mens genital. But maybe it is just my mind.

@eirikbakke
Copy link
Contributor

eirikbakke commented Nov 9, 2024

Here's an alternative that alludes to a a line or bezier curve being edited.

fileSVG

@eirikbakke
Copy link
Contributor

I have problems showing embedded images with relative paths:

I would probably prefer just disallowing loading of external resources, and require that any graphics be self-contained within the SVG file. An SVG file that tries to load external resources is going to break in a lot of contexts anyway, and it's useful for the preview to reflect this fact by not drawing that part of the image.

For example, when ImageUtilities loads SVG files, it explicitly disallows loading of external resources (both in the old Batik version and the new JSVG version at #7941 ).

@Chris2011
Copy link
Contributor Author

So as suggested moving SVG to an own module or to image or wherever it fits best. I can think about image.svg ant module also with this name or directly into the ide/image module. Honestly, I don't care much for me it should be simple. Do you have any preferences? Also with the pros and cons for your suggestions. That would be nice.

@matthiasblaesing
Copy link
Contributor

Integration into ide/image:

  • Pro: GUI with scaling exists and Navigator with preview is implemented
  • Contra: Requires additional dependency on JSVG and xml module

Integration into ide/xml:

  • Pro: xml modules is not required
  • Contra: Requires additional dependency on JSVG
  • Contra: Image viewer and navigator need to be copied from ide/image or to be reimplemented
  • Contra: What makes SVG special compared to other XML based formats?

New Module:

  • Pro: Clean slate and enough space to implement new functions
  • Contra: Image viewer and navigator need to be copied from ide/image or to be reimplemented

Stuffing the new functions into existing modules seems like the easier road, but a new module seems to be correcter road.

@eirikbakke
Copy link
Contributor

I have merged #7941 now, which switches ImageUtilities from Batik to the lighter JSVG library. Eventually we will remove Batik, per #7969, but for now the libs.batik.read library remains, which means this PR should rebase cleanly on top of the latest master.

With respect to the SVG library switch, I'd suggest now (1) rebasing, (2) adding a dependency on the "libs.jsvg" dependency, (3) switching the Batik-related code to use the JSVG APIs instead, (4) verifying that it works, and then (5) removing the dependency on "libs.batik.read" and the changes relating to updating the Batik libraries. (Sorry for the extra work of switching SVG libraries!)

@Chris2011
Copy link
Contributor Author

Integration into ide/image:

  • Pro: GUI with scaling exists and Navigator with preview is implemented
  • Contra: Requires additional dependency on JSVG and xml module
    ...

Thx @matthiasblaesing I already created a new module called image.svg. I also can rename it just to SVG.

Just curious, I had a look into the ImageViewer that you already mentioned, of course, when I reuse them I need to change some stuff which is not required for SVG, but it seems that it is not recommended to make a dependency to the image module to, if possible, to reuse the required panels. So is this correct and why?

@eirikbakke thx and no problem, I will change it as soon as possible :).

@Chris2011
Copy link
Contributor Author

So I changed from XML first to image, then I moved from image to a new module called image.svg, now I called it just svg and the name is SVG Support as we have it for XML Support.

Next steps are:

  • Add preview to navigator
  • Add toolbar to the preview for scaling options.

FYI

@Chris2011
Copy link
Contributor Author

Chris2011 commented Nov 20, 2024

@eirikbakke if I understand it correctly, there is no such a JSVGCanvas as we have it in batik so I need to create a BufferedImage as seen in this example from the readme: https://github.com/weisJ/jsvg and the quality is not that nice, because it will be converted to a pixel graphic.

I already added the settings:

g.setRenderingHint(RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON);
g.setRenderingHint(RenderingHints.KEY_STROKE_CONTROL, RenderingHints.VALUE_STROKE_PURE);

It is okish, but not perfect. Let's see how it fits now. Watching SVG graphics they look like pixel graphics is kind a confusing. IMHO

@Chris2011
Copy link
Contributor Author

Ok i see it myself, forget the last comment. Svg needs to be rendered as an image anyway so I can have a look what is the JSVGCanvas is doing and try to adapt it somehow.

@neilcsmith-net
Copy link
Member

@Chris2011 there appear to be at least two examples of SVG Swing components in the readme. You almost certainly don't want to use BufferedImage.

@eirikbakke
Copy link
Contributor

@Chris2011 Yeah, I agree with @neilcsmith-net, normally you just paint the SVG directly in the component's paint() method; no need to go through a BufferedImage. That way you automatically get the right behavior on HiDPI screens, and don't have to allocate a buffer. You'd parse the SVG once to a com.github.weisj.jsvg.SVGDocument and then use that object to paint (rather than re-parsing on every paint).

@Chris2011
Copy link
Contributor Author

Yeah I'm on it :). Thx

@Chris2011 Chris2011 marked this pull request as draft November 22, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Editor Platform [ci] enable platform tests (platform/*) Upgrade Library Library (Dependency) Upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants