OmegaT - multiplatform CAT tool Code

Looks like sourceforge is linking to an unrelated feature request; the bug in question here is https://sourceforge.net/p/omegat/bugs/636/.

Thanks! Shall I recreate the merge request with the corrected ticket mention in the commit message?

Sure. If SourceForge is anything like GitHub, you can simply reword your commit message and force-push over the existing remote branch (no need to make a new merge request).

Done; let me know if you'd like more details about the included changes!

Thanks. I'll hopefully be able to take a look this weekend.

Unfortunately it seems I can't comment on lines of your code on SourceForge :(

It's a very minor point, but appends like res.append("%" + name + ";"); would be nicer as res.append('%').append(name).append(';'); so you don't build an unneeded intermediary string. (I know the original code was like the former, so I don't blame you for copying its style.)

In Handler.localizeSystemId, please don't swallow the IllegalArgumentException. At the very least it should be logged.

Is there a reason you do new URL(systemId).toURI() instead of simply new URI(systemId)?

We might as well fix the typo in the local variable parameterEntiry in the doEndEntity method.

I'm a little concerned about the if (extWriter != null) condition at the end of doEndEntity. Previously it was assumed that extWriter was not null, so it would be an error for it to be null at that point. Now such an error would not be revealed. However this class is extremely stateful and to be honest I don't understand it fully, so maybe it's not a problem. But I would prefer to remove the if if possible (maybe it should be only in one branch of the if (dtd != null) conditional immediately above).

There are several places where you have an open bracket attached to a parenthesis: ){; please put a space between.

Thanks very much for the unit test. I confirmed that it passes with your changes, and fails when they are reverted.

Please take a look at my feedback above and let me know when you're done with any additional changes.

I haven't touched Java code in decades, so I'm grateful for the thorough code review.

About Handler.localizeSystemId: swallowing IllegalArgumentException is in line with the previous behavior: if the path can't be relativized, the absolute path is preserved without any fuss. It is not an actual error case.

As for the extra condition in doEndEntity, this is due to my decoupling of extEntity and extWriter in doResolve; this corrects the behavior when referring to an extEntity that is in a parent directory of the current output directory (in which case we don't want to be writing in the output path's parent).

All the other items you mentioned, I will correct and resubmit.

This merge request is now up-to-date with the corrections.

Thanks for the update.

Is there a reason you do new URL(systemId).toURI() instead of simply new URI(systemId)?

I just realized this was a dumb question; the smart question would have been "why not do Paths.get(systemId)?" Unfortunately it seems like I can't push to your branch (unlike GitHub) so I will cherry pick your commit and push it to master manually.

Actually Paths.get(systemId) is wrong because first you need to parse it as a URI, otherwise you get file: in your path. Duh.