Summary: | HTMLPrinter should not use deprecated HTML attributes | ||
---|---|---|---|
Product: | [Eclipse Project] Platform | Reporter: | Matthias Becker <ma.becker> |
Component: | Text | Assignee: | Matthias Becker <ma.becker> |
Status: | NEW --- | QA Contact: | |
Severity: | normal | ||
Priority: | P3 | CC: | daniel_megert, mistria |
Version: | 4.8 | ||
Target Milestone: | --- | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://git.eclipse.org/r/123323 https://bugs.eclipse.org/bugs/show_bug.cgi?id=535049 https://git.eclipse.org/r/128023 https://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=90473a1dc49a94911dd66496af0e8e21cf224d55 |
||
Whiteboard: |
Description
Matthias Becker
2018-05-25 06:33:17 EDT
New Gerrit change created: https://git.eclipse.org/r/123323 +1 A manual test of my fix should be performed. The JDT code element information hovers e.g. use HTML printer. The should look the same way as before. (In reply to Matthias Becker from comment #3) > A manual test of my fix should be performed. The JDT code element > information hovers e.g. use HTML printer. The should look the same way as > before. That class (or better, the package) is marked as friends to many. There is a big chance that you break things here. Can someone pls. test my patch on windows and linux. I am a Mac guy. (In reply to Dani Megert from comment #4) > That class (or better, the package) is marked as friends to many. There is a > big chance that you break things here. I added a second patchset that changes the implementation in a way that the old and the new behavior *should* be the same. In the old implementation the colors added to the body tag could be overrules by color specifications of the body element in the provided sytlesheet. The new implementation added the colors also as CSS rules but at the first position. Due to the "cascading" in CSS rules that come later in the sytlesheet overrule rules that appeared before. Shouldn't we give it a try? +1 I existing code does assume that HTMLPrinter adds the "text" and the "bgcolor" attributes on the "body" element, this change would break that existing code. See: https://git.eclipse.org/r/#/c/124564/ (In reply to Matthias Becker from comment #8) > I existing code does assume that HTMLPrinter adds the "text" and the > "bgcolor" attributes on the "body" element, this change would break that > existing code. > See: https://git.eclipse.org/r/#/c/124564/ As I said. Touching HTMLPrinter will most likely cause havoc. (In reply to Dani Megert from comment #9) > As I said. Touching HTMLPrinter will most likely cause havoc. I would say that relying on the HTML code that is generated by HTMLPrinter (by replacing parts of it) is a miss-use of HTMLPrinter. I just browsed a bit through JDT, PDE and Platform and all the usages of HTMLPrinter look good for me Matthias, would be nice to close this for RC1. (In reply to Matthias Becker from comment #10) > (In reply to Dani Megert from comment #9) > > As I said. Touching HTMLPrinter will most likely cause havoc. > I would say that relying on the HTML code that is generated by HTMLPrinter > (by replacing parts of it) is a miss-use of HTMLPrinter. > I just browsed a bit through JDT, PDE and Platform and all the usages of > HTMLPrinter look good for me So it was only patchset 1 and 2 that did string-manipulate the resulting HTML it optained from HTMLPrinter. In the final version of that change everything is fine. As stated above the behaviour of the generated HTML is the same. But the generated HTML code is different. So everbody that makes assumptions about the generated HTML code (and tries to manipulate it) might break. But as stated above I would say this a miss-use. My change brings JUnit test with it that document the behaviour and does these all combinations - we didn't have JUnits for HTMLPrinter before. So I still would propose to merge this change - but maybe not in the RC-phase as time is short once we find places that break. So should we merge it beginning for 4.10? (In reply to Matthias Becker from comment #12) > I still would propose to merge this change - but maybe not in the > RC-phase as time is short once we find places that break. So should we merge > it beginning for 4.10? +1. New Gerrit change created: https://git.eclipse.org/r/128023 (In reply to Eclipse Genie from comment #14) > New Gerrit change created: https://git.eclipse.org/r/128023 Some of the friends of HTMLPrinter's package (org.eclipse.ui.workbench.texteditor and org.eclipse.jdt.debug.ui) did no longer use that package. So I removed these friends. I did not get any new warnings / errors about using not released stuff - or did I miss something? (In reply to Matthias Becker from comment #15) > (In reply to Eclipse Genie from comment #14) > > New Gerrit change created: https://git.eclipse.org/r/128023 > > Some of the friends of HTMLPrinter's package > (org.eclipse.ui.workbench.texteditor and > org.eclipse.jdt.debug.ui) did no longer use that package. > So I removed these friends. I did not get any new warnings / errors about > using not released stuff - or did I miss something? Sounds reasonable. Gerrit change https://git.eclipse.org/r/128023 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=90473a1dc49a94911dd66496af0e8e21cf224d55 What's missing here? Can it be marked as resolved? (In reply to Mickael Istria from comment #18) > What's missing here? https://git.eclipse.org/r/#/c/123323/ Moving t. 4.11 (see latest comment in the Gerrit change). What's the status here? Matthias, can you finish this one in 4.14? |