Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8262981: Create implementation for NSAccessibilitySlider protocol #1811

Closed

Conversation

RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Mar 17, 2023

I propose to backport JDK-8262981 in a way that does not require a CSR. Just integrate the native code but don't touch the Java interface of javax.swing.JSlider.


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

Issues

  • JDK-8262981: Create implementation for NSAccessibilitySlider protocol
  • JDK-8300791: Create implementation for NSAccessibilitySlider protocol (CSR) (Withdrawn)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/1811/head:pull/1811
$ git checkout pull/1811

Update a local copy of the PR:
$ git checkout pull/1811
$ git pull https://git.openjdk.org/jdk11u-dev.git pull/1811/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1811

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/1811.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 17, 2023

👋 Welcome back clanger! 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 changed the title Backport 9d669c912d3977027f321a5e6b7f045ca2ce9482 8262981: Create implementation for NSAccessibilitySlider protocol Mar 17, 2023
@openjdk
Copy link

openjdk bot commented Mar 17, 2023

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Mar 17, 2023
@mlbridge
Copy link

mlbridge bot commented Mar 17, 2023

Webrevs

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Mar 17, 2023
@RealCLanger
Copy link
Contributor Author

/csr unneeded

@openjdk
Copy link

openjdk bot commented Mar 17, 2023

@RealCLanger The CSR requirement cannot be removed as there is already a CSR associated with the issue JDK-8262981. Please withdraw the CSR JDK-8300791 and then use the command /csr unneeded again.

@RealCLanger
Copy link
Contributor Author

/csr unneeded

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Mar 17, 2023
@openjdk
Copy link

openjdk bot commented Mar 17, 2023

@RealCLanger determined that a CSR request is not needed for this pull request.

@RealCLanger
Copy link
Contributor Author

@Autumn808 @phohensee Would you mind reviewing this one (and integrating into your patch bulk)? Thx

Copy link
Contributor

@Autumn808 Autumn808 left a comment

Choose a reason for hiding this comment

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

I think this is a great fix to avoid CSR and unblock the series of backports.

@RealCLanger RealCLanger force-pushed the RealCLanger-backport-9d669c91 branch from 5f5342b to f72d269 Compare March 17, 2023 20:35
Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

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

Works for me.

@openjdk
Copy link

openjdk bot commented Mar 17, 2023

@RealCLanger Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@openjdk
Copy link

openjdk bot commented Mar 17, 2023

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

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 17, 2023

- (BOOL)accessibilityPerformDecrement
{
return [self performAccessibleAction:DECREMENT];
Copy link
Member

Choose a reason for hiding this comment

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

This method should upcall the java part of the patch, and may throw an exception if we will skip it.

I think we should include the java part but make it private, then re-implement this next code path to call it:
accessibilityPerformDecrement->performAccessibleAction:->CAccessibility#doAccessibleAction()->JSlider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, that's a good catch. We'll need to rework that. performAccessibleAction, however, will expect an AccessibleAction, so we need to do this differently, if we can't implement this interface in JSlider.

Copy link
Member

Choose a reason for hiding this comment

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

CAccessibility#doAccessibleAction() can be reworked to accept Object, and then we can switch on type of the parameter, the private methods can be accessed via SwingAccessor

@RealCLanger RealCLanger marked this pull request as draft March 19, 2023 16:43
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 19, 2023
@RealCLanger RealCLanger deleted the RealCLanger-backport-9d669c91 branch April 13, 2023 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 participants