8210807: Printing a JTable with a JScrollPane prints table without rows populated #16552

TejeshR13 · 2023-11-08T07:32:52Z

BackingStore uses its own Graphics to Paint the Image. This is fine when we want to paint it on a Window/Frame, but fails to print. Since WPathGraphics is used for Printing, backing store fails to print it and backing store uses newly created SunGraphics2D always. The proposed fix is to ignore backingstore graphics if passed on Graphics is different from SunGraphics2D which ensures the Image is painted on Window and also printed when required.
The fix is tested in CI and doesn't cause any regression. The test is verified manually.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8210807: Printing a JTable with a JScrollPane prints table without rows populated (Bug - P3)

Reviewers

Prasanta Sadhukhan (@prsadhuk - Reviewer) ⚠️ Review applies to 8111eaa8
Abhishek Kumar (@kumarabhi006 - Committer) ⚠️ Review applies to dbcc134b

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16552/head:pull/16552
$ git checkout pull/16552

Update a local copy of the PR:
$ git checkout pull/16552
$ git pull https://git.openjdk.org/jdk.git pull/16552/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16552

View PR using the GUI difftool:
$ git pr show -t 16552

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16552.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-11-08T07:33:39Z

👋 Welcome back tr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-11-08T07:34:57Z

@TejeshR13 The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-11-08T07:38:11Z

Webrevs

src/java.desktop/share/classes/javax/swing/JViewport.java

test/jdk/javax/swing/JTable/JTableScrollPrintTest.java

prsadhuk · 2023-11-08T13:33:26Z

src/java.desktop/share/classes/javax/swing/JViewport.java

-        bsg.setFont(g.getFont());
-        bsg.setClip(g.getClipBounds());
-        return bsg;
+        if (g instanceof SunGraphics2D) {


One thing is the change is not obvious as to why it is being done.
I think we can make the fix more telling by utilizing isPrinting check to make it prominent that the code path is for printing

Suggested change

if (g instanceof SunGraphics2D) {

if (!SwingUtilities2.isPrinting(g)) {

Can you see if it works? And yes, you have to make the method public..

Yes, it works, can't use SwingUtilities2.isPrinting(g) but can check for PrintGraphics instance which is done in SwingUtilities2.isPrinting(g) .. Thank you for the input...

Why cant you use isPrinting?

It's not public..

And yes, you have to make the method public..

Thats what I told already..

Ok, guess I missed it..

test/jdk/javax/swing/JTable/JTableScrollPrintTest.java

src/java.desktop/share/classes/javax/swing/JViewport.java

test/jdk/javax/swing/JTable/JTableScrollPrintTest.java

openjdk · 2023-11-10T09:56:12Z

@TejeshR13 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8210807: Printing a JTable with a JScrollPane prints table without rows populated

Reviewed-by: psadhukhan, abhiscxk

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 572 new commits pushed to the master branch:

fe0ccdf: 8319640: ClassicFormat::parseObject (from DateTimeFormatter) does not conform to the javadoc and may leak DateTimeException
1802cb5: 8319570: Change to GCC 13.2.0 for building on Linux at Oracle
d992033: 8317562: [JFR] Compilation queue statistics
965ae72: 8319753: Duration javadoc has "period" instead of "duration" in several places
115b074: 8319944: Remove DynamicDumpSharedSpaces
c0507af: 8319818: Address GCC 13.2.0 warnings (stringop-overflow and dangling-pointer)
3684b4b: 8306116: Update CLDR to Version 44.0
88ccd64: 8296250: Update ICU4J to Version 74.1
03db828: 8319650: Improve heap dump performance with class metadata caching
b41b00a: 8319820: Use unnamed variables in the FFM implementation
... and 562 more: https://git.openjdk.org/jdk/compare/fc98998627443d6e73ac70661f47f48b30525712...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

kumarabhi006

Minor findings.

test/jdk/javax/swing/JTable/JTableScrollPrintTest.java

TejeshR13 · 2023-11-14T09:51:02Z

/integrate

openjdk · 2023-11-14T09:53:07Z

Going to push as commit 95bd92a.
Since your change was applied there have been 576 commits pushed to the master branch:

21cda19: 8309203: C2: remove copy-by-value of GrowableArray for InterfaceSet
b120a05: 8319406: x86: Shorter movptr(reg, imm) for 32-bit immediates
7df73a2: 8318817: Could not reserve enough space in CodeHeap 'profiled nmethods' (0K)
07eaea8: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test
fe0ccdf: 8319640: ClassicFormat::parseObject (from DateTimeFormatter) does not conform to the javadoc and may leak DateTimeException
1802cb5: 8319570: Change to GCC 13.2.0 for building on Linux at Oracle
d992033: 8317562: [JFR] Compilation queue statistics
965ae72: 8319753: Duration javadoc has "period" instead of "duration" in several places
115b074: 8319944: Remove DynamicDumpSharedSpaces
c0507af: 8319818: Address GCC 13.2.0 warnings (stringop-overflow and dangling-pointer)
... and 566 more: https://git.openjdk.org/jdk/compare/fc98998627443d6e73ac70661f47f48b30525712...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-11-14T09:53:16Z

@TejeshR13 Pushed as commit 95bd92a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Fix + Manual Test

3f795c1

openjdk bot added the rfr Pull request is ready for review label Nov 8, 2023

openjdk bot added the client client-libs-dev@openjdk.org label Nov 8, 2023

prsadhuk reviewed Nov 8, 2023

View reviewed changes

src/java.desktop/share/classes/javax/swing/JViewport.java Outdated Show resolved Hide resolved

Review fix

0bfeee3

prsadhuk reviewed Nov 8, 2023

View reviewed changes

test/jdk/javax/swing/JTable/JTableScrollPrintTest.java Outdated Show resolved Hide resolved

Review fix

9f6afa2

prsadhuk reviewed Nov 8, 2023

View reviewed changes

test/jdk/javax/swing/JTable/JTableScrollPrintTest.java Outdated Show resolved Hide resolved

TejeshR13 added 2 commits November 10, 2023 10:21

Review fix

c7a0988

Review fix

a4389a0

prsadhuk reviewed Nov 10, 2023

View reviewed changes

src/java.desktop/share/classes/javax/swing/JViewport.java Outdated Show resolved Hide resolved

Review fix

89fce5d

prsadhuk reviewed Nov 10, 2023

View reviewed changes

test/jdk/javax/swing/JTable/JTableScrollPrintTest.java Outdated Show resolved Hide resolved

test/jdk/javax/swing/JTable/JTableScrollPrintTest.java Outdated Show resolved Hide resolved

test/jdk/javax/swing/JTable/JTableScrollPrintTest.java Outdated Show resolved Hide resolved

TejeshR13 added 2 commits November 10, 2023 14:38

Review fix

cac0c62

Review fix

8111eaa

prsadhuk approved these changes Nov 10, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 10, 2023

kumarabhi006 reviewed Nov 10, 2023

View reviewed changes

Review fix

7a35907

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 10, 2023

Review fix

dbcc134

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 10, 2023

kumarabhi006 approved these changes Nov 10, 2023

View reviewed changes

turbanoff reviewed Nov 13, 2023

View reviewed changes

test/jdk/javax/swing/JTable/JTableScrollPrintTest.java Outdated Show resolved Hide resolved

turbanoff reviewed Nov 13, 2023

View reviewed changes

test/jdk/javax/swing/JTable/JTableScrollPrintTest.java Outdated Show resolved Hide resolved

turbanoff reviewed Nov 13, 2023

View reviewed changes

test/jdk/javax/swing/JTable/JTableScrollPrintTest.java Outdated Show resolved Hide resolved

Review fix

9e4540e

openjdk bot added the integrated Pull request has been integrated label Nov 14, 2023

openjdk bot closed this Nov 14, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 14, 2023

	if (g instanceof SunGraphics2D) {
	if (!SwingUtilities2.isPrinting(g)) {

8210807: Printing a JTable with a JScrollPane prints table without rows populated #16552

8210807: Printing a JTable with a JScrollPane prints table without rows populated #16552

Uh oh!

Conversation

TejeshR13 commented Nov 8, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Nov 8, 2023

Uh oh!

openjdk bot commented Nov 8, 2023

Uh oh!

mlbridge bot commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

prsadhuk Nov 8, 2023

Choose a reason for hiding this comment

Uh oh!

TejeshR13 Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

prsadhuk Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

TejeshR13 Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

prsadhuk Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

TejeshR13 Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

TejeshR13 Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumarabhi006 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TejeshR13 commented Nov 14, 2023

Uh oh!

openjdk bot commented Nov 14, 2023

Uh oh!

openjdk bot commented Nov 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

TejeshR13 commented Nov 8, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 8, 2023 •

edited

Loading

openjdk bot commented Nov 10, 2023 •

edited

Loading