6955128: Spec for javax.swing.plaf.basic.BasicTextUI.getVisibleEditorRect contains inappropriate wording

[prsadhuk]

BasicTextUI.getVisibleEditorRect wording is rephrased to remove the wording "Due to an unfortunate set of historical events this method is inappropriately named".

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

• JDK-6955128: Spec for javax.swing.plaf.basic.BasicTextUI.getVisibleEditorRect contains inappropriate wording (Enhancement - P4)

Reviewers

• Damon Nguyen (@DamonGuy - Committer)
• Abhishek Kumar (@kumarabhi006 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25850

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back psadhukhan! 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]

@prsadhuk This change is no longer ready for integration - check the PR body for details.

[openjdk]

@prsadhuk 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]

Webrevs

• 00: Full (aeed7361)

[kumarabhi006]

CSR?

[TejeshR13]

Suggested change

	* the method is thus named to preserve backward compatibility.
	* the method name is preserved for backward compatibility.

If method name is retained from past then this sentence would be right ?

[honkar-jdk]

I agree. The above rephrasing sounds clearer.

[honkar-jdk]

@kumarabhi006

CSR?

javadoc change is for a protected method so I think CSR is not required for this change

[aivanov-jdk]

Suggested change

	* Gets the allocation to give the root View.
	* Gets the allocation for the root view.

[aivanov-jdk]

Suggested change

	* The Rectangle returned has nothing to do with visibility but
	* The rectangle returned has nothing to do with visibility but

I suggest going lower case unless you specifically refer to the rectangle as the type in which case, it should be {@code Rectangle}.

[aivanov-jdk]

@kumarabhi006

CSR?

javadoc change is for a protected method so I think CSR is not required for this change

A protected method is part of the public API contract, see BasicTextUI.getVisibleEditorRect in Java 24.

However, I say a CSR is not needed because the meaning hasn't changed.

[prrace]

"nothing to do with" -> "unrelated to"

But what I'd really like to see is an explanation of what the return from this method ACTUALLY means / is used for.
What does "the allocation" mean ? Too woolly for me.

[prsadhuk]

getVisibleEditorRect() return the bounds of the text editor and the return value is used to to set-size/paint the
javax.swing.text.View of this text editor component.
"the allocation" refers to what allocated area the view needs to paint, it seems to be a common phrase used in context of View class which says

"The view gets its allocation from the parent at paint time, so it must be prepared to redo layout if the allocated area is different from what it is prepared to deal with. "

In that regard, probably getAllocatedViewRect() would have been better choice of the method name

[aivanov-jdk]

The term allocation is commonly used in javax.swing.text.View and its subclasses, for example, The view gets its allocation from the parent at paint time, and While a view may render into its entire allocation, typically a view does not. (Emphasis mine.)

The View class has a method getChildAllocation.

Essentially, the allocation for text view means the size and position of the view. In this case, the returned rectangle is used an allocation for the root view to perform painting.

jdk/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTextUI.java

Lines 767 to 771 in 0dce98b

	// paint the view hierarchy
	Rectangle alloc = getVisibleEditorRect();
	if (alloc != null) {
	rootView.paint(g, alloc);
	}

I do not think this method should explain what the allocation means; it may point to the View class.

[aivanov-jdk]

@prsadhuk @prrace Does this description work?

    /**
     * Gets the allocation for the root view.
     * The returned rectangle is unrelated to visibility,
     * it is passed as the {@code allocation} parameter to
     * the {@link View#paint(Graphics, Shape) View.paint} method.
     * The component must have a non-zero positive size for
     * this translation to be computed.
     *
     * @return the bounding box for the root view
     */
    protected Rectangle getVisibleEditorRect() {

This renders like this:

Gets the allocation for the root view. The returned rectangle is unrelated to visibility, it is passed as the allocation parameter to the View.paint method. The component must have a non-zero positive size for this translation to be computed.

We can also add @TejeshR13's suggestion: “The method name is preserved for backward compatibility.”

[prrace]

/reviewers 2 reviewer

[prrace]

This may yet need a CSR depending on where the wording ends up.

[openjdk]

@prrace
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).