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

A11y: Combine client info into a group #2638

Merged
merged 1 commit into from
May 29, 2022

Conversation

chigkim
Copy link
Contributor

@chigkim chigkim commented May 22, 2022

Short description of changes
Combine all the client info, so screen reader users don't have to track down multiple places.

CHANGELOG: Accessibility: Combine all the client info, so screen reader users don't have to track down multiple places.

Context: Fixes an issue?

Does this change need documentation? What needs to be documented and how?

Status of this Pull Request

What is missing until this pull request can be merged?

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@ann0see
Copy link
Member

ann0see commented May 22, 2022

Thanks! Is it that easy?

@ann0see ann0see added this to the Release 3.9.0 milestone May 22, 2022
@ann0see ann0see requested a review from pljones May 22, 2022 17:58
@chigkim
Copy link
Contributor Author

chigkim commented May 22, 2022 via email

Copy link
Member

@hoffie hoffie left a comment

Choose a reason for hiding this comment

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

Thanks -- that sounds great! One inline question (see above).

@@ -862,6 +862,8 @@ void CChannelFader::SetChannelInfos ( const CChannelInfo& cChanInfo )
plblLabel->setToolTip ( strToolTip );
plblLabel->setAccessibleName ( strAliasAccessible );
plblLabel->setAccessibleDescription ( tr ( "Alias" ) );
dynamic_cast<QWidget*> ( plblLabel->parent() )
Copy link
Member

Choose a reason for hiding this comment

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

Just trying to understand why the cast is needed:

  • This is in CChannelFader which is a QObject. Therefore, parent() returns a QObject as well.
  • Doing the dynamic_cast is safe and right to do because we know that CChannelFader is only used as a child in CAudioMixerBoard, which is a QWidget.

Is that correct? If so, would it make sense to add a short code comment about that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Most UI things are widgets. But if parent (i.e. the form it's stuck to) is only declared as a QObject, you'll get told off by the compiler trying to use methods QObject doesn't have -- even though the QObject you're talking about does have them. Hence the cast. So long as it's viable (I've not checked).

@chigkim
Copy link
Contributor Author

chigkim commented May 22, 2022 via email

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Unfortunately, I couldn't verify this PR. How can I see it in action?

@chigkim
Copy link
Contributor Author

chigkim commented May 28, 2022 via email

@ann0see
Copy link
Member

ann0see commented May 29, 2022

Ok. Thanks. Could hear it now. The integrated one of Win 11 didn't find it

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

This PR needs to be rebased to resolve the conflicts

Copy link
Collaborator

@pljones pljones left a comment

Choose a reason for hiding this comment

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

It is a shame about that cast - I'd have though the parent widget might be known somewhere. It probably is....

@chigkim chigkim force-pushed the a11y-client-info branch from 936fc3c to c174496 Compare May 29, 2022 12:04
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Thanks!

@ann0see ann0see merged commit 1056396 into jamulussoftware:master May 29, 2022
@chigkim chigkim deleted the a11y-client-info branch July 31, 2022 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

4 participants