RichTextPainter fails to render valid XHTML with namespace URI #500

[lcaron]

Hi Jan

I planned to release Nebula 3 tonight. If you can provide à patch, I can wait until tomorrow

[jmauersberger]

Hi, here is my patch, it fixes #500 and adds to be handled as , sorry that this sneaks in. RichTextPainter.patch RichTextPainter.patch

[fipro78]

Are you saying it is a valid xhtml to add a namespace to an end tag? Never seen this and can't find anything about that in the specs.

[fipro78]

@lcaron Don't do anything with that patch file. It is incorrect in many ways! I have created a PR that contains the fixes: https://github.com/eclipse/nebula/pull/507

@jmauersberger Even though this is a project on GitHub, it is still an Eclipse project that has to follow processes. Let me give you some hints on your mistakes, so they don't happen the next time.

Technical:

1. I am not sure if you have a signed ECA. IIRC this was the problem in the past. Without it we are not allowed to take any code from you.
2. This is a GitHub project. As such we only accept pull requests that we can review, comment and simply merge. Patch files are much more work and avoid traceability.
3. The patch file is not based on the current state in the repository. The fix for the start tag was already applied and merged. I suppose if we would try to apply the patch, it would fail because of merge conflicts. And even if not, it is not a good practice.
4. You combine fixes for two different things in one patch. That is a very bad practice and again avoids traceability.

Communication:

1. You opened the issue more than two months ago, without ever getting back to us. I understand that companies, OSS and legal stuff takes time. But you could have at least update us on the progress.
2. After I fixed the issue more than two weeks ago, you replied that it is ok. And two weeks later you actually stop a release by simply saying "This wont work". No further detail on what is not working. I find this very offensive, as I tested it and it worked, and you simply say it isn't. So what is not working? Namespace attributes in end tags? I am actually not aware that this is valid. Or what do I miss here?
3. You are trying to bring in a fix for something you haven't even reported at any time before. And you comment this with a simple "sorry that this sneaks in". That is actually not an excuse for not following the processes of a managed and mature open source project. If I wouldn't have looked into the patch file in more detail (which is no work I actually like to do, verifying patch files in a text editor), this change would have been in the code base without anybody knowing about it.

I hope you take my comments as a constructive critic so the next time your contributions follow the process and we don't need these short time actions and comments that might have broken the legal aspects of our release.

[jmauersberger]

Sure Dirk, no offense taken, I felt a bit under pressure by Icaron's request, should have considered your points before hastily posting a patch, all of them are valid and obvious. I have signed an ECA some years back and the status was nicely shown on the old eclipse pages, I guess I need to get a refresher.

Anyway, on the technical side I have seen such XHTML in the wild so fixing the end tag was necessary too.

[jmauersberger]

Hi @fipro78, I double checked my statement that we "saw end tags with namespaces in the wild". You are right that the end tag never has a namespace in the XHTML. However, the XML parser used by the RichTextPainter returns all XMLEvents with a namespace if there is one at the root begin tag, that includes also EndElement. That's why we needed also to change the XMLStreamConstants.END_ELEMENT case.