On Wed, 20 Nov 2024 17:48:26 GMT, Harshitha Onkar <hon...@openjdk.org> wrote:
>> @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). I can create a > new JBS issue on your behalf. > Since this test spans 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 PRs 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. @honkar-jdk I just got access to JBS today, so that's not a problem. I've created JDK-8344637 to track just the Linux and Windows fixes in this PR (scoping out macOS for now). Have you had a chance to review on Windows? ------------- PR Comment: https://git.openjdk.org/jdk/pull/21980#issuecomment-2489330915
