8332416: Add more font selection options to Font2DTest #19273

prrace · 2024-05-16T19:05:03Z

Enhance Font2DTest as follows

Add main menu Radio Button options so that you select the font to use as either
(1) Font Family + Style (as now)
(2) Font Family + Menu of all members of the Family, replacing the Style
(3) List of all fontnames - which can still be adjusted by Style if you want.
The default is (1) so nothing looks different except that I updated the UI to use Nimbus instead of Metal.

There's new code to gather these ways of referencing the fonts.
Also changes were needed for the "Save/Load" options to include the new UI state and font settings.

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-8332416: Add more font selection options to Font2DTest (Enhancement - P4)

Reviewers

Tejesh R (@TejeshR13 - Committer)
Harshitha Onkar (@honkar-jdk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19273

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-05-16T19:05:43Z

👋 Welcome back prr! 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 · 2024-05-16T19:06:55Z

@prrace 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:

8332416: Add more font selection options to Font2DTest

Reviewed-by: tr, honkar

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 84 new commits pushed to the master branch:

da3001d: 8331975: Enable case-insensitive check in ccache and keytab entry lookup
424eb60: 8331683: Clean up GetCarrierThread
9b1d6d6: 8316328: Test jdk/jfr/event/oldobject/TestSanityDefault.java times out for some heap sizes
f8a3e4e: 8328998: Encoding support for Intel APX extended general-purpose registers
ddd73b4: 8332082: Shenandoah: Use consistent tests to determine when pre-write barrier is active
0a9d1f8: 8332749: Broken link in MemorySegment.Scope.html
c9a7b97: 8332829: [BACKOUT] C2: crash in compiled code because of dependency on removed range check CastIIs
7fd9d6c: 8332340: Add JavacBench as a test case for CDS
417d174: 8331348: Some incremental builds deposit files in the make directory
303ac9f: 8332671: Logging for pretouching thread stacks shows wrong memory range
... and 74 more: https://git.openjdk.org/jdk/compare/da9c23ace9bdf398d811a88ed137217dd3167231...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.

openjdk · 2024-05-16T19:07:18Z

@prrace 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 · 2024-05-16T19:10:08Z

Webrevs

prrace · 2024-05-16T23:39:15Z

BTW, it is probably obvious , but maybe 50% of "reviewing" this will be pulling it down and building it locally and trying it out. Very hard to review by code changes alone.

TejeshR13

I have an observation, not sure if it's supposed to be this way or not. The Style/SubFamily type selected in other mentioned type (Select font using Family and Style/SubFamily/Full Name) has an impact on each other type. For example, if we select bold as Style and then in type 2 we select SubFamily as .plain then it'll be bold+.plain right? Or its supposed to be only .plain in type 2?

turbanoff · 2024-05-21T16:42:15Z

src/demo/share/jfc/Font2DTest/Font2DTest.java

+              else if (familyChars[familyIndex] == ' ' || familyChars[familyIndex] == '-') {
+                  familyIndex++;
+              }
+              else if (fullChars[fullIndex] == ' '  || fullChars[fullIndex] == '-') {


Suggested change

else if (fullChars[fullIndex] == ' ' || fullChars[fullIndex] == '-') {

else if (fullChars[fullIndex] == ' ' || fullChars[fullIndex] == '-') {

prrace · 2024-05-21T16:51:26Z

I have an observation, not sure if it's supposed to be this way or not. The Style/SubFamily type selected in other mentioned type (Select font using Family and Style/SubFamily/Full Name) has an impact on each other type. For example, if we select bold as Style and then in type 2 we select SubFamily as .plain then it'll be bold+.plain right? Or its supposed to be only .plain in type 2?

Probably not.
I was OK with "Style" being applied to "FontName" because we show the "Style" menu alongside font name.
Ordinarily you would use PLAIN but being able to select BOLD then allows you to test what happens in our implementation
when you say "Arial Italic with style Bold please" .. we figure out that you want "Arial Bold Italic".

But if we are using Family + SubFamily you don't see the "Style" menu, but I think you are right that it probably remembers the last selected style and applies it and then the substitution will happen as described above, but the UI doesn't show this.
To avoid this I think I can just reset "Style" to "PLAIN" whenever switching the UI to use SubFamily.

prrace · 2024-05-21T23:05:50Z

I think I can just reset "Style" to "PLAIN" whenever switching the UI to use SubFamily.

It was a bit more than that, but I think it is behaving sensibly now. Please take another look.

TejeshR13 · 2024-05-22T04:58:25Z

I think I can just reset "Style" to "PLAIN" whenever switching the UI to use SubFamily.

It was a bit more than that, but I think it is behaving sensibly now. Please take another look.

Yeah, it's fine now. The reset is happening and the selections are independent of its type.

honkar-jdk · 2024-05-23T18:43:21Z

To avoid this I think I can just reset "Style" to "PLAIN" whenever switching the UI to use SubFamily.

Changes looks good after the following fix. Save and Load option works as expected.

honkar-jdk · 2024-05-23T18:50:01Z

src/demo/share/jfc/Font2DTest/Font2DTest.java

+                f.addWindowListener( new WindowAdapter() {
+                    public void windowOpening( WindowEvent e ) { f2dt.repaint(); }
+                    public void windowClosing( WindowEvent e ) { System.exit(0); }


@prrace Minor Spacing issue. I see this pattern in the original code as well, could be a quick find+replace all fix for opening and closing brace. But then again it would also create a lot of git diff footprint along with the fix.

Suggested change

f.addWindowListener( new WindowAdapter() {

public void windowOpening( WindowEvent e ) { f2dt.repaint(); }

public void windowClosing( WindowEvent e ) { System.exit(0); }

f.addWindowListener( new WindowAdapter() {

public void windowOpening(WindowEvent e) { f2dt.repaint(); }

public void windowClosing(WindowEvent e) { System.exit(0); }

There's a LOT of such things, that my itchy fingers wanted to fix, but I deliberately resisted temptation and don't see any reason to pick on a few random infractions to fix.

Yeah, agreed on this.

prsadhuk · 2024-05-24T05:39:30Z

src/demo/share/jfc/Font2DTest/FontPanel.java

@@ -740,7 +740,8 @@ private void resetScrollbar( int oldValue ) {
                verticalBar.setValues( oldValue, numCharDown, 0, totalNumRows );
            }
            if ( totalNumRows <= numCharDown && drawStart == 0) {


Is this check needed now since both if and else condition do the same thing?

I left it because I might want to re-enable this in the future if / when I figure out why the disabled Nimbus scroll bar just looks like a blank piece of window.

prsadhuk · 2024-05-24T05:42:31Z

src/demo/share/jfc/Font2DTest/Font2DTest.java

+                    public void windowClosing( WindowEvent e ) { System.exit(0); }
+                });
+
+                f.getContentPane().add( f2dt );


Since this is new code, we can remove getContentPane which is not needed for add

It isn't "new code". I just moved the block inside invokeAndWait because starting the UI
on the main thread can be a real source of hangs. So these lines are indented more but not new.

prrace · 2024-05-25T23:17:35Z

/integrate

openjdk · 2024-05-25T23:17:58Z

Going to push as commit 97ee2ff.
Since your change was applied there have been 105 commits pushed to the master branch:

985b9ce: 8330694: Rename 'HeapRegion' to 'G1HeapRegion'
05f13e7: 8329667: [macos] Issue with JTree related fix for JDK-8317771
7bf1989: 8320575: generic type information lost on mandated parameters of record's compact constructors
253508b: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
ebc520e: 8332841: GenShen: Pull shared members from control thread into common base class
236432d: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook
b3b3366: 8332631: Update nsk.share.jpda.BindServer to don't use finalization
f66a586: 8332641: Update nsk.share.jpda.Jdb to don't use finalization
cd3e4c0: 8326734: text-decoration applied to lost when mixed with or

c2cca2a: 8330647: Two CDS tests fail with -UseCompressedOops and UseSerialGC/UseParallelGC

... and 95 more: https://git.openjdk.org/jdk/compare/da9c23ace9bdf398d811a88ed137217dd3167231...master

~~Your commit was automatically rebased without conflicts.~~

openjdk · 2024-05-25T23:18:04Z

@prrace Pushed as commit 97ee2ff.

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

8332416

0fc0e27

openjdk bot changed the title ~~8332416~~ 8332416: Add more font selection options to Font2DTest May 16, 2024

openjdk bot added the rfr Pull request is ready for review label May 16, 2024

openjdk bot added the client client-libs-dev@openjdk.org label May 16, 2024

8332416

9721de2

TejeshR13 reviewed May 21, 2024

View reviewed changes

turbanoff reviewed May 21, 2024

View reviewed changes

8332416

9491ae3

honkar-jdk reviewed May 23, 2024

View reviewed changes

honkar-jdk approved these changes May 23, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 23, 2024

TejeshR13 approved these changes May 24, 2024

View reviewed changes

prsadhuk reviewed May 24, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 25, 2024

openjdk bot closed this May 25, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8332416: Add more font selection options to Font2DTest #19273

8332416: Add more font selection options to Font2DTest #19273

prrace commented May 16, 2024 •

edited by openjdk bot

bridgekeeper bot commented May 16, 2024

openjdk bot commented May 16, 2024 •

edited

openjdk bot commented May 16, 2024

mlbridge bot commented May 16, 2024 •

edited

prrace commented May 16, 2024

TejeshR13 left a comment

turbanoff May 21, 2024

prrace commented May 21, 2024

prrace commented May 21, 2024

TejeshR13 commented May 22, 2024

honkar-jdk commented May 23, 2024

honkar-jdk May 23, 2024

prrace May 23, 2024

honkar-jdk May 23, 2024

TejeshR13 May 24, 2024

prsadhuk May 24, 2024

prrace May 24, 2024

prsadhuk May 24, 2024

prrace May 24, 2024

prrace commented May 25, 2024

openjdk bot commented May 25, 2024

openjdk bot commented May 25, 2024

	else if (fullChars[fullIndex] == ' ' \|\| fullChars[fullIndex] == '-') {
	else if (fullChars[fullIndex] == ' ' \|\| fullChars[fullIndex] == '-') {

8332416: Add more font selection options to Font2DTest #19273

8332416: Add more font selection options to Font2DTest #19273

Conversation

prrace commented May 16, 2024 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 16, 2024

openjdk bot commented May 16, 2024 • edited

openjdk bot commented May 16, 2024

mlbridge bot commented May 16, 2024 • edited

Webrevs

prrace commented May 16, 2024

TejeshR13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prrace commented May 21, 2024

prrace commented May 21, 2024

TejeshR13 commented May 22, 2024

honkar-jdk commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prrace commented May 25, 2024

openjdk bot commented May 25, 2024

openjdk bot commented May 25, 2024

prrace commented May 16, 2024 •

edited by openjdk bot

openjdk bot commented May 16, 2024 •

edited

mlbridge bot commented May 16, 2024 •

edited