Preview SVG files #7938
Preview SVG files #7938
Conversation
Chris2011
added
the
ci:dev-build
There was a problem hiding this comment.
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.
platform/libs.batik.read/release/modules/ext/batik-anim-1.18.jar
Outdated
Show resolved
Hide resolved
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
added
Upgrade Library
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 |
|
@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? |
|
I'd consider adding a |
|
Thanks for this PR!
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. |
|
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 ;) |
|
@mbien Good point, two libraries is kind of awkward...
Probably true. (I don't know exactly what the limitations with jSVG are compared to Batik.) |
It would be really nice if there was an option to preview svg in the navigator, similar to other images: |
|
@troizet good catch, will check this too. |
I think these are mostly unrelated concerns, and probably don't need to be discussed on this PR ...
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?
|
the reason why I brought this up at all is because thinking a bit ahead can sometimes save some work.
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.
yep. Adding a dependency with overlapping functionality of an existing dependency should have a higher barrier.
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. |
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 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? |
|
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...) |
|
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 For the preview I think being able to scale would be good, as implemented in the
In that module there is also an I'm not sure, that I see the SVG support inside the XML module, it could also be in image or independent. |
|
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:
Thx for the hints all. |
|
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? |
|
@matthiasblaesing Sure, here's an icon you're free to use:
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. 
|
|
@eirikbakke thank you! |
|
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. |
|
Here's an alternative that alludes to a a line or bezier curve being edited.
|
|
@weisJ While fixing the addressed feedback, I addressed an exception of course that this is rubbish because I took a file, renamed it to *.svg which was a *.http file with stuff that is not SVG. Shouldn't this be handled somehow? Yes I know, who the hell would do this. I wanted to test someting and this came up. |
This is why I'm asking about the exceptions. I got a lot of exceptions if parsing is not possible. For example, exceptions will throw for content <>lads to this exception or without closing tag <svg>leads to this exception or with this sample <?xml version="1.0" encoding="UTF-8"?>
<svg version="1.1"
baseProfile="full"
width="300" height="200"
xmlns="http://www.w3.org/2000/svg">
<!-- Ein Rechteck in Rot -->
<rect x="10" y="10" width="100" height="100" fill="red" />
<!-- Hier der absichtlich fehlerhafte Kreis mit falsch geschlossenem Tag -->
<g>
<circle cx="150" cy="60" r="50" fill="blue"></circl>
</g>
</svg>
leads to this exception com.ctc.wstx.exc.WstxParsingException: Unexpected close tag </circl>; expected </circle>.
at [row,col {unknown-source}]: [12,55]
at com.ctc.wstx.sr.StreamScanner.constructWfcException(StreamScanner.java:634)
at com.ctc.wstx.sr.StreamScanner.throwParseError(StreamScanner.java:504)
at com.ctc.wstx.sr.StreamScanner.throwParseError(StreamScanner.java:488)
at com.ctc.wstx.sr.BasicStreamReader.reportWrongEndElem(BasicStreamReader.java:3373)
at com.ctc.wstx.sr.BasicStreamReader.readEndElem(BasicStreamReader.java:3300)
at com.ctc.wstx.sr.BasicStreamReader.nextFromTree(BasicStreamReader.java:2920)
at com.ctc.wstx.sr.BasicStreamReader.next(BasicStreamReader.java:1122)
at com.ctc.wstx.evt.WstxEventReader.nextEvent(WstxEventReader.java:283)
[catch] at com.github.weisj.jsvg.parser.StaxSVGLoader.parse(StaxSVGLoader.java:106)
at com.github.weisj.jsvg.parser.StaxSVGLoader.load(StaxSVGLoader.java:167)
at com.github.weisj.jsvg.parser.SVGLoader.load(SVGLoader.java:111)
at com.github.weisj.jsvg.parser.SVGLoader.load(SVGLoader.java:62)
at com.github.weisj.jsvg.parser.SVGLoader.load(SVGLoader.java:54)
at com.github.weisj.jsvg.parser.SVGLoader.load(SVGLoader.java:49)
at org.netbeans.modules.svg.navigation.SVGNavigatorPanel.lambda$setNewContent$1(SVGNavigatorPanel.java:149)
at org.openide.util.RequestProcessor$Task.run(RequestProcessor.java:1403)
at org.netbeans.modules.openide.util.GlobalLookup.execute(GlobalLookup.java:45)
at org.openide.util.lookup.Lookups.executeWith(Lookups.java:287)
at org.openide.util.RequestProcessor$Processor.run(RequestProcessor.java:2018) |
|
I must have changed this since the last released version. That's my bad. In this case of course I would recommend to catch exceptions. I will try to release the next version soon. |
|
Is this even possible to catch the error if the lib already does this? I already tried try/catch but I still get the exception. Is this done in another thread? |
|
So is the exception thrown or simply logged? I am a bit confused about what the particular problem is here. Parsing is done on the same thread as |
This is what happens, it is shown in the notifications so I guess not just logged and I can't paint my error on that Panel due to the exception. 20241229-2246-51.6934276.mp4 |
since you mentioned "painting the error", you are probably looking at the wrong method. The trace shows that this is a svg loading exception, not an exception which happens during painting. |
|
Yeah maybe it didn't show everysthing but we have more than one exception: This happens in the SVGViewerElement where I also run load in updateView. |
|
right. So usages of it might have been lost between so many messages but the timeline was like this if i remember correctly:
its off topic but IMO: anything what does IO and/or non-trivial parsing should require checked exceptions, this automatically documents how to handle failures etc, but its a minor detail here and we can't influence it anyway |
I already do in my local code and still the error is thrown. This is why I'm wondering. In my code I already have try catch (locally). In the SVGViewerElementm in the SVGNavigatorPanel in the SVGPanel everywhere, I call load or render. But the screencapture is the result still. Maybe I still have a mistake in my code, could be, I can push my changes but a simple try/catch where load is called, does nothing. I need to debug into it, but first I wanted to know whether the exception was handled by the lib somehow. |
|
yes please share your changes somehow, if it doesn't build push into another branch and link it or something similar, otherwise simply add a commit to this PR like you did before. |
It builds so no problem at all. I'm still working on it. I also just add hints for me to fix this code. As you can see, I just wanted to show a JOptionPane for "debugging". This is my personal console.log :D. |
|
I can see how this can look confusing: jsvg itself logs with SEVERE level, this lands in the NB log handler. All severe log lines (maybe also warnings - can't remember) will land in the notifications since they are automatically intercepted, as mentioned in #7938 (comment).
this experiment confirms it: static{
// JSVG loader/parser is logging with SEVERE level which would land in the NB exception reporter
Logger.getLogger("com.github.weisj.jsvg").setLevel(Level.OFF);
}
public SVGDataObject(...mystery solved ;) |
ide/svg/src/org/netbeans/modules/svg/navigation/SVGNavigatorPanel.java
Outdated
Show resolved
Hide resolved
|
After my last changes I forgot to say that I have still one problem. It updates all messages on the opened SVG so filesChanged fired and everything works fine. When I switch to another SVG, resultChanged is fired but filesChanged never fired again so on save, navigator never updated. This is what I saw inside the debugger. So still working on it why this happens. |
|
For your next revision, here is an updated version of the SVG icon, which matches the slightly darker background color I settled on for file icons in #8083.
|
|
So first I just updated the svg, as @eirikbakke mentioned. Thx. Second I debugged a lot to identify the problem. The behavior that I mentioned, happens sometimes and I dunno whether this is just my dev env or real a bug. So pleaes can someone please confirm the problem that I have? Update of navigator and svg preview is working after build and run the app. When the SVG was open before or a new one opens. When I have open multiple SVGs and I switch to another, just the svg preview updates, not the navigator. Fun fact, when I click in the project tab around and go to the SVG which was not working correctly, it is working. So this is why I just saying it is sometimes not working correctly. Please verify, I will go deeper, but for now I couldn't find a solution, just that sometimes the listeners are not triggered anymore. |
| static { | ||
| // JSVG loader/parser is logging with SEVERE level which would land in the NB exception reporter | ||
| Logger.getLogger("com.github.weisj.jsvg").setLevel(Level.OFF); | ||
| } |
There was a problem hiding this comment.
thought about this a bit more: it might be better to set this via the config file. Since turning the logger on again might help @eirikbakke to debug icon loading.
I might be looking at the wrong places but I can't find any logging configs anywhere - a bit surprising that NB didn't have to adjust log levels of third party libs before. cc @neilcsmith-net
There was a problem hiding this comment.
Perhaps set it to level Level.INFO instead? That way the error ends up on the console but does not trigger the exception reporter in the user interface.
There was a problem hiding this comment.
i think INFO is the default. Keep in mind that this setting is the cutoff for the log handler and does not influence the message log levels. The lib logs with SEVERE, unfortunately the next higher level is OFF.
There was a problem hiding this comment.
Keep in mind that this setting is the cutoff for the log handler and does not influence the message log levels.
Oh... you're right. Idea: Turn the logging off only temporarily while the file is being loaded for SVG Preview purposes, but then turn it back to its previous value in a "finally" block right after.
There was a problem hiding this comment.
wouldn't recommend doing that for two reasons: threading and it being brittle since the places where the lib logs with SEVERE could change. Turning logging off for that package is just configuration - thats why I thought to move it to the NB config.
the spec is that it returns null if it fails - we have to live with it i guess. For debugging reasons we can switch it on again.
There was a problem hiding this comment.
OK, I'm fine with turning logging off for JSVG then, by configuration or otherwise.
Perhaps eventually we could request from @weisJ that JSVG reduces the log level from SEVERE to INFO for problems that are caused by invalid SVG inputs, rather than true unexpected conditions?
There was a problem hiding this comment.
Thx, will remove this.
There was a problem hiding this comment.
If we decide to upgrade JSVG in this PR too.
There was a problem hiding this comment.
Given API changes, let's leave JSVG upgrading out of this and after branching for NB27.
Chris2011
force-pushed
the
feature/svg-preview
branch
from
9b6d4ff to
c17efcd
Compare
Chris2011
force-pushed
the
feature/svg-preview
branch
from
a14e389 to
19e104c
Compare
|
@mbien @matthiasblaesing you can now recheck, i fixed all problems. I was kind a lost for the decision whether we would suppress the jsvg errors or not. It is also just one commit. |
|
Thx to all and no problem. I will wait a bit, until end of the week and if there are no other concerns, I will merge this. |
The PR will preview SVG files inside the IDE with MultiViewSupport
I already add actions that make sense to the SVGDataObject as for XML files like:
I have 2 problems faced here:
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?