On Tue, 19 Nov 2024 01:49:50 GMT, Harshitha Onkar <hon...@openjdk.org> wrote:
>> @DamonGuy Brilliant, thanks for the check. This feels like it might be >> drawing offscreen because of incorrectly-combined transforms, like what was >> fixed in PR #20993 (but this would be somewhere in the Mac-specific code). >> >> @honkar-jdk I was able to dig up an old Windows laptop, and have confirmed >> that the page 8 issue on Windows is caused by two regressions introduced >> over the past decade but not caught because this is a manual test, not an >> automated test... I'm going to see if I can't get this test passing on both >> Linux and Windows. Keep an eye out for additional commits to this PR. > > @gredler Tested on windows with the latest fix. The portrait looks good but > looks like the fix causes more issues in landscape mode. I'll require some > time to go through the source code changes when I get a chance. Looks like > you might be in the process of adding few more changes for Windows issue. Let > me know once the PR is ready. > > **Portrait** > > <img width="1393" alt="Win_PortraitPg#8" > src="https://github.com/user-attachments/assets/a6b48dfe-19f0-4739-825f-e50b0ae50128";> > > **Landscape** > > <img width="1532" alt="Win_LandscapePg#8" > src="https://github.com/user-attachments/assets/eeeb309f-fd36-4b8f-a77b-d53acc8a178b";> @honkar-jdk OK, I think this PR is ready for another review on Windows. The issue you highlighted in your last comment was caused by a partial fix to the JDK-8132890 regression (which had obviously removed the font rotation component, but had less-obviously also removed the device transform rotation component). The goal is to completely fix this test on Windows and Linux, so I've updated the problem list to flag only macOS as problematic. Will we need a separate bug in the issue tracker, since this PR will not fix the macOS parts of JDK-8148334? I'm not sure whether PRs are allowed to partially fix issues (when they represent more than one bug), or whether they are expected to fully fix whatever issue they reference. @prrace This is related to two bugs that you worked on in the past: JDK-8132890 and JDK-8029204. It has been a few years, but it would be good to get your review on this as well, if you have the time. In particular, it feels like we stopped using most of the device transform in some parts of the JDK-8132890 changes (e.g. glyph position calculation), and now we're re-adding the rotation component, and I wonder if there are other parts of the device transform that we are missing, and/or whether it would be easier to go back to using the device transform in the glyph position calculation. (The code seems to work as-is, I'm just wondering if it's more complex than is necessary.) Summary of changes: * PathGraphics: Do not apply font transform translation component twice (results in incorrect text start point) * WPathGraphics: Do not apply font transform translation component twice (results in incorrect text start point) * WPathGraphics: Include both font and device rotation when calculating glyph positions * WPathGraphics: Fix direction of y scale ------------- PR Comment: https://git.openjdk.org/jdk/pull/21980#issuecomment-2488555719
