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

Add: screenreader-friendly main window #6090

Merged
merged 171 commits into from Jul 23, 2022

Conversation

vadi2
Copy link
Member

@vadi2 vadi2 commented Apr 30, 2022

Brief overview of PR changes/additions

Screenreader-friendly main window.

Motivation for adding to Mudlet

Crucial part of accessibility support in Mudlet

Other info (issues closed, discussion etc)

Addresses #3342 but doesn't close it, as we'd like to migrate to using PTBs for the follow-up improvements.

This focuses on the main window only, and will not tackle other accessibility issues such as miniconsoles or any of the issues in menus.

Builds on the excellent work of @bauermann in #5985. Windows announcements are helped by code from both Osara and Qt.

edit: This comment has links to downloads: #6090 (comment)

vadi2 and others added 30 commits February 21, 2022 22:36
First, rename mLastRenderBottom to mLastRenderedOffset. The old name is
misleading because it actually holds the line that was at the top of the
screen when it was last rendered, not the bottom.

Also add explanation of what mScrollVector means.

These changes help understand the logic at TTextEdit::drawForeground().
TConsole is best viewer as a container of other widgets, rather than a
widget itself. Therefore, it makes more sense to add accessibility support
to its inner widgets.
…ibleTextEdit

Thanks to Stephen Lyons for the suggestion.
Use plain strings rather than QStringLiterals. The QStringLiteral()
versions add annoying double quotes to the output.

Also, use function calls rather than streams to output the messages. The
syntax is more compact and allows for better control of the formatting.
… interface_cast()

QAccessibleWidget's version already does that for us.

Also take the opportunity to remove some unnecessary headers.
Rely on the superclass' implementation.
It's redundant, because QAccessibleWidget's constructor already takes a
role argument.
This communicates that they're internal helpers for the class implementation and
aren't used anywhere else.
…contents

... as oposed to assuming that every (x, y) coordinate contains a character.
The current cursor position isn't relevant in the calculation.
This fixes the following warning:

  'TAccessibleTextEdit::text' hides overloaded virtual function

Adjust lineForOffset() and cursorPosition() to take into account the
newlines added by join().

Also, simplify text(int startOffset, int endOffset) using the new
text(QAccessible::Text t) method.
It's partially based on QAccessibleTextWidget::attributes() from Qt 5.15.
Also, remove ‘DocumentContentChanged’ event from TConsole::print() methods
because both call TTextEdit::showNewLines(), which already sends
‘TextInsertEvent’.
Copy link
Member

@demonnic demonnic left a comment

Choose a reason for hiding this comment

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

ok, I'm not exactly the strongest with c++/Qt but I gave this the once over and it looks ok to me. No glaring issues I noticed, no major typos or anything.

I have not done any functional testing, as I do not have a screenreader setup yet.

@vadi2
Copy link
Member Author

vadi2 commented Jul 23, 2022

Thanks a lot for the review!

@vadi2 vadi2 merged commit 25e6e3c into Mudlet:development Jul 23, 2022
@vadi2 vadi2 deleted the text-window-accessibility branch July 23, 2022 15:13
@vadi2 vadi2 restored the text-window-accessibility branch July 24, 2022 18:14
@vadi2 vadi2 deleted the text-window-accessibility branch July 28, 2022 18:09
SlySven added a commit to SlySven/Mudlet that referenced this pull request Aug 6, 2022
…2/MINGW-W64

The error messages that this was producing were somewhat obscure but only
appeared in my MSYS2/Mingw-w64 based Windows setup (where I have the
environmental variable `WITH_MAIN_BUILD_SYSTEM` defined to `"NO"`)! The
result of it however was that any build based on code since Mudlet#6090 went in
was failing...

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit that referenced this pull request Aug 6, 2022
…2/MINGW-W64 (#6218)

The error messages that this was producing were somewhat obscure but only
appeared in my MSYS2/Mingw-w64 based Windows setup (where I have the
environmental variable `WITH_MAIN_BUILD_SYSTEM` defined to `"NO"`)! The
result of it however was that any build based on code since #6090 went in
was failing...

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit to SlySven/Mudlet that referenced this pull request Aug 7, 2022
Mudlet#6090 added the library to the QMake LIBS variable for the main build
system but it is in fact needed also for the MSYS2/Mingw-w64 setup (that I
use) as well so needs to be put in a different location in the main QMake
project file.

Whilst checking to see if the CMake alternative worked in my setup I
noticed that there is a typo in the adjecent line for the "ws2_32" library.
The documentation at, say:
https://docs.microsoft.com/en-us/windows/win32/winsock/initialization-2
calls it `Ws2_32`, however the files in a Windows filesystem (and the
corresponding line in the CMake build file):
```
if(WIN32)
  target_link_libraries(mudlet ws2_32)
...
endif()
```
specifies an all-lower case (library file) name - this is possibly okay
because of the way that Windows handles file names but it is probably
better to use the same thing all around.

I note that though we list both `libws2_32` and `liboleut32` in the QMake
project files it seems that only the former needs (?) to be mentioned in
the CMake case - limited experimentation I've done on my setup suggests
that the latter is linked in automagically without being explictly stated!

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit that referenced this pull request Aug 7, 2022
…#6220)

#6090 added the library to the QMake LIBS variable for the main build
system but it is in fact needed also for the MSYS2/Mingw-w64 setup (that I
use) as well so needs to be put in a different location in the main QMake
project file.

Whilst checking to see if the CMake alternative worked in my setup I
noticed that there is a typo in the adjacent line for the "ws2_32" library.
The documentation at, say:
https://docs.microsoft.com/en-us/windows/win32/winsock/initialization-2
calls it `Ws2_32`, however the files in a Windows filesystem (and the
corresponding line in the CMake build file):
```
if(WIN32)
  target_link_libraries(mudlet ws2_32)
...
endif()
```
specifies an all-lower case (library file) name - this is possibly okay
because of the way that Windows handles file names but it is probably
better to use the same thing all around.

I note that though we list both `libws2_32` and `liboleut32` in the QMake
project files it seems that only the former needs (?) to be mentioned in
the CMake case - limited experimentation I've done on my setup suggests
that the latter is linked in automagically without being explicitly stated!

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants