Skip to content
This repository has been archived by the owner on Apr 24, 2023. It is now read-only.

8301989: new javax.swing.text.DefaultCaret().setBlinkRate(N) results in NPE #122

Closed
wants to merge 2 commits into from

Conversation

azuev-java
Copy link
Member

@azuev-java azuev-java commented Feb 7, 2023

Check if the component is associated with the caret before calling methods from it. Added test case that will make sure that will not happen again.


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-8301989: new javax.swing.text.DefaultCaret().setBlinkRate(N) results in NPE

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 122

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk20/pull/122.diff

…in NPE

Check if the component is associated with the caret before calling methods from it.
Added test case that will make sure that will not happen again.
@bridgekeeper
Copy link

bridgekeeper bot commented Feb 7, 2023

👋 Welcome back kizune! 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 openjdk bot added the rfr Pull request is ready for review label Feb 7, 2023
@openjdk
Copy link

openjdk bot commented Feb 7, 2023

@azuev-java 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.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Feb 7, 2023
@mlbridge
Copy link

mlbridge bot commented Feb 7, 2023

Webrevs

@@ -1050,7 +1050,7 @@ public void setBlinkRate(int rate) {
throw new IllegalArgumentException("Invalid blink rate: " + rate);
}
if (rate != 0) {
if (component.isEditable()) {
if (component != null && component.isEditable()) {
if (flasher == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So later if there is a non-editable component, what happens ?
Some existing logic for that will kick in ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when the the caret is to be assigned to a component it will reset the blink rate and the logic will kick in. Manually tested with the possible scenarios like adding custom set blink rate cursor to a non-editable component and then turn it into editable later, add to editable and switch to non-editable later and so on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I go back and look at the code before your previous fix, there was no check,
so the timer is created and then I'd expect applies when the caret is installed on a component.
Here that behaviour is changed, so that if we create a caret, then call setBlinkRate(), and then install()
then the blink rate appears to be ignored whereas before 4512626 it was applied.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we create a caret, then call setBlinkRate(), and then install()
then the blink rate appears to be ignored whereas before 4512626 it was applied.

Ok, if the text component is already visible and editable and focused and we install the new caret then the blink rate will not have an effect until the focus is lost and regained or until it became non-editable - and i think that was close to what we had before and there is a bug to investigate and optimize such corner case. In all other cases the code works fine. The value set is being preserved and will be reactivated when any of the methods such as focusGained or when the component installUI is called - if the cursor is set before component becomes visible - will cause to re-evaluate the blink rate and the stored value set by user will take place.

Copy link
Contributor

@prrace prrace Feb 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppose I have this code

Caret c = new DefaultCaret()
c.setBlinkRate(50);
c.install(editableJComponent);

BEFORE 4512626 the JDK code looked like this

 if (rate != 0) {
        if (flasher == null) {
            flasher = new Timer(rate, handler);
        }
        flasher.setDelay(rate);
    }

With your fix it looks like this

 if (rate != 0) {
        if (component != null && component.isEditable()) {
           if (flasher == null) {
               flasher = new Timer(rate, handler);
           }
         }
    }

So with my sample application code there's a change that my blink rate is completely lost.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it seems that in such a case, "savedBlinkRate" is still set and that is used to both report the value of blink rate and create & start a Timer when it is needed.
It is a little hard to follow through all those "when it is needed" cases but if we are attached to an enabled & editable component when focusGained() is called it should then start the flasher timer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the simple code like this one:

        caret.setBlinkRate(10);
        JFrame frame = new JFrame("test");
        JTextArea area = new JTextArea(null, "Some text to make it non-empty", 5, 40);
        area.setEditable(false);
        area.setCaret(caret);
        frame.setLayout(new BorderLayout());
        frame.add(area, BorderLayout.CENTER);
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.pack();
        frame.setVisible(true);
        SwingUtilities.invokeAndWait(() -> area.requestFocus(false));
        SwingUtilities.invokeAndWait(() -> area.setEditable(true));
        SwingUtilities.invokeAndWait(() -> {
            System.out.println(area.getCaret().getBlinkRate());
        });

i tested many possible combinations to see that the blink rate specified in setBlinkRate is preserved and not being overwritten by some code. In some corner cases the cursor does not blink despite reporting the correct blink rate until the focus is lost and regained - but that is still better than the version before JDK-4512626 where in the same corner case the cursor is not visible until the focus is lost and gained again. There is a bug JDK-8299048 that tracks the corner case i am talking about.

Copy link

@DamonGuy DamonGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes seem to fix the NPE as expected. When testing, the provided test fails before the patch and passes after the patch is applied.

I was curious if the 2nd conditional was necessary to check for "component == null", but it definitely is. Also, your test checks for this, so that helped clear it up.

Copy link

@honkar-jdk honkar-jdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch works as expected. NPE problem is resolved. Additionally checked the fix by adding DefaultCaret to component (JTextField) in enabled and disabled states, it works as expected - Blinking caret when enabled and no caret when disabled.

@prrace
Copy link
Contributor

prrace commented Feb 8, 2023

Another question, in the branch where the app calls
setBlinkRate(0)
we have
if ((component == null || component.isEditable()) && isBlinkRateSaved) {
savedBlinkRate = 0;
isBlinkRateSaved = false;
}

Why do we not clear these unconditionally ?
What is the point of saving it if there's a non-editable component ?

@azuev-java
Copy link
Member Author

Why do we not clear these unconditionally ?
What is the point of saving it if there's a non-editable component ?

Because then when we set component non-editable the stored blink rate will be lost and we will not be able to restore it when component will be switched to editable again. Same with the component not assigned - if we assign it to the non-editable component and then flip the editable property then we would not have the previous blink rate saved and will not know what value to set.

@prrace
Copy link
Contributor

prrace commented Feb 8, 2023

Why do we not clear these unconditionally ?
What is the point of saving it if there's a non-editable component ?

Because then when we set component non-editable the stored blink rate will be lost and we will not be able to restore it when component will be switched to editable again. Same with the component not assigned - if we assign it to the non-editable component and then flip the editable property then we would not have the previous blink rate saved and will not know what value to set.

I am not following .. this is about the branch where the app is setting blink rate to zero.
We surely don't want to keep the old blink rate just because a component is non-editable ?

@azuev-java
Copy link
Member Author

azuev-java commented Feb 8, 2023

We surely don't want to keep the old blink rate just because a component is non-editable ?

When we receive focus on non-editable component we set blink rate to 0 to stop caret blinking. If we do not add this condition then we immediately clear off the saved blink rate.

@prrace
Copy link
Contributor

prrace commented Feb 8, 2023

We surely don't want to keep the old blink rate just because a component is non-editable ?

When we receive focus on non-editable component we set blink rate to 0 to stop caret blinking. If we do not add this condition then we immediately clear off the saved blink rate.

How do you tell that apart from the app directly clearing it ? ie the APP calling setBlinkRate(0) ?

@azuev-java
Copy link
Member Author

How do you tell that apart from the app directly clearing it ? ie the APP calling setBlinkRate(0) ?

I do not - while component is non-editable you can not change blink rate to 0, once the component will become editable the last non-zero value will be restored.

@prrace
Copy link
Contributor

prrace commented Feb 8, 2023

How do you tell that apart from the app directly clearing it ? ie the APP calling setBlinkRate(0) ?

I do not - while component is non-editable you can not change blink rate to 0, once the component will become editable the last non-zero value will be restored.

But that seems wrong. Why is it not wrong ?

@azuev-java
Copy link
Member Author

But that seems wrong. Why is it not wrong ?

It might be an oversight of the initial fix, if you think it is wrong we can submit a bug and i can fix it. I definitely do not think it is critical and i do not think it is related to the nature of this change.

@prrace
Copy link
Contributor

prrace commented Feb 8, 2023

But that seems wrong. Why is it not wrong ?

It might be an oversight of the initial fix, if you think it is wrong we can submit a bug and i can fix it. I definitely do not think it is critical and i do not think it is related to the nature of this change.

Well .. it does seem wrong to me .. I was hoping you'd be able to explain why its OK.
That makes two "corner cases" to clean up in a later release.
Dilemma .. way too risky to try to clean that up now .. not sure if we want to back out the ENTIRE fix ... not sure about OKing a fix with known issues.
The only way I can look at it and convince myself is to consider that that corner case itself does not look like a P2 release stopper ..

Copy link
Contributor

@prrace prrace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as critical to fix the NPE.
There seems possible there may be followup work in a later release.

@openjdk
Copy link

openjdk bot commented Feb 8, 2023

@azuev-java 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:

8301989: new javax.swing.text.DefaultCaret().setBlinkRate(N) results in NPE

Reviewed-by: dnguyen, honkar, prr

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 1 new commit pushed to the master branch:

  • 6f460e4: 8301863: ObjectInputFilter example incorrectly calls rejectUndecidedClass

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Feb 8, 2023
@azuev-java
Copy link
Member Author

Approving as critical to fix the NPE. There seems possible there may be followup work in a later release.

There will be.

@azuev-java
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Feb 8, 2023

Going to push as commit e81f20b.
Since your change was applied there has been 1 commit pushed to the master branch:

  • 6f460e4: 8301863: ObjectInputFilter example incorrectly calls rejectUndecidedClass

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Feb 8, 2023
@openjdk openjdk bot closed this Feb 8, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 8, 2023
@openjdk
Copy link

openjdk bot commented Feb 8, 2023

@azuev-java Pushed as commit e81f20b.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants