On Wed, 20 Nov 2024 13:15:29 GMT, Daniel Gredler <d...@openjdk.org> wrote:
>> @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 @gredler > Will we need a separate bug in the issue tracker, since this PR will not fix > the macOS parts of JDK-8148334? That's correct. I think it is better to create a new JBS bug and use the new JBS ID for this PR than using [JDK-8148334](https://bugs.openjdk.org/browse/JDK-8148334). Let me know if you need any help with creating a new JBS issue. Since this test span across multiple pages testing various glyph transformations it is better to keep [JDK-8148334](https://bugs.openjdk.org/browse/JDK-8148334) in open state and reference it while problemlisting the test. > 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. A PR can have fix that focuses on specific issue and separate PR can be created to address other issues. So in this case it seems reasonable to have a PR for Windows and Linux fix and create a separate one for macOS. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21980#issuecomment-2489212478
