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

Fix Java Access Bridge compatibility on 32 bit systems #16557

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

FelipeZanabria
Copy link

@FelipeZanabria FelipeZanabria commented May 16, 2024

Link to issue number:

fixes #10842, fixes #16330

Summary of the issue:

When NVDA focuses a Java application on a 32-bit system, it gets stuck on the icon, without the ability to interact with it.

Description of user facing changes

When the user on a 32-bit system focuses on a Java application, the controls can be navigated normally, as was the case in version 2019.2.1 and earlier.

Description of development approach

  • Check the architecture of the running system
  • Based on this define the jobj64 class.
    • If the system is 64 bits use the type c_int64, otherwise use c_int.
    • Finally define which dll should be loaded. If the system is 64-bit, load WindowsAccessBridge-32.dll included in NVDA, otherwise load WindowsAccessBridge.dll, available in c:\Windows\System32.

Testing strategy:

Assuming the user has Java installed on a 32-bit system, simply search for Java in the start menu or control panel. When you find the configure Java element, press enter and move through it

Known issues with pull request:

None.

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@FelipeZanabria FelipeZanabria requested a review from a team as a code owner May 16, 2024 17:55
@AppVeyorBot
Copy link

See test results for failed build of commit 3d1837b138

@AppVeyorBot
Copy link

See test results for failed build of commit 833372aadc

@@ -43,6 +44,9 @@
import config
from utils.security import isRunningOnSecureDesktop

#: Verification of the architecture of the running system
is_64Bit = maxsize > 2**32
Copy link
Member

Choose a reason for hiding this comment

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

given #16330, could you make this a system wide constant in winAPI.constants?

Copy link
Member

Choose a reason for hiding this comment

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

See also #16330 (comment), please use os.environ["PROCESSOR_ARCHITECTURE"].endswith("64")

source/JABHandler.py Show resolved Hide resolved
source/JABHandler.py Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
@seanbudd seanbudd marked this pull request as draft May 17, 2024 02:28
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@mwhapples
Copy link
Collaborator

What environment precisely is this other DLL to be used in? To clarify specifically what I am thinking, is it for a 32-bit OS environment where both NVDA and the Java runtime will be 32-bit, or is it for a 32-bit (java runtime regardless of the OS environment (IE. it is possible to run a 32-bit JRE on a 64-bit OS). Does this correctly detect the actual thing of concern or does it miss a case IE. if the DLL is to be used whenever a 32-bit JRE application is running, then detecting the OS architecture misses the case of 32-bit JRE on 64-bit windows). Can NVDA detect whether the application is a 32-bit or 64-bit process?

…ystems in an NVDA lib folder. The way of checking the system's architecture has also been modified for better readability.
@AppVeyorBot
Copy link

See test results for failed build of commit 95b6ced447

@FelipeZanabria
Copy link
Author

FelipeZanabria commented May 17, 2024

What environment precisely is this other DLL to be used in? To clarify specifically what I am thinking, is it for a 32-bit OS environment where both NVDA and the Java runtime will be 32-bit?

Yes.

@FelipeZanabria
Copy link
Author

@FelipeZanabria FelipeZanabria marked this pull request as ready for review May 17, 2024 21:32
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label May 20, 2024
@@ -43,6 +43,9 @@
import config
from utils.security import isRunningOnSecureDesktop

#: Verification of the architecture of the running system
is_64Bit = os.environ["PROCESSOR_ARCHITECTURE"].endswith("64")
Copy link
Member

Choose a reason for hiding this comment

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

As requested earlier, given #16330, could you make this a system wide constant in winAPI.constants?

source/JABHandler.py Outdated Show resolved Hide resolved
@seanbudd seanbudd marked this pull request as draft May 21, 2024 05:46
@josephsl
Copy link
Collaborator

josephsl commented May 21, 2024 via email

@seanbudd
Copy link
Member

Thanks @FelipeZanabria, can you please pull in the latest changes from nvaccess/javaAccessBridge32-bin#2 and also update

* [Java Access Bridge 32 bit, from Zulu Community OpenJDK build 17.0.9+8Zulu (17.46.19)](https://github.com/nvaccess/javaAccessBridge32-bin)

@seanbudd
Copy link
Member

Please also add a message to changes.md like this one

* Updated Java Access Bridge to 17.0.9+8Zulu (17.46.19). (#15744)

@josephsl
Copy link
Collaborator

josephsl commented May 24, 2024

Hi,

I see. While I'm not from NV Access, I would like to strongly recommend doing the following before contributing future pull requests:

  1. UPGRADE TO (or BUY) Windows 10, preferably Version 22H2 (2022 Update/build 19045); or even better, if you are able, get a system with Windows 11 Version 23H2 (2023 Update) installed so you can be on the same page as most developers (NV Access folks included).
  2. INSTALL Git LOCALLY! As you have learned, there are tasks requiring local Git on your computer (one important task is updating submodules).
  3. If able, try setting up the local development environment as close to what NV Access folks and contributors are using (Python 3.11 32-bit, Visual Studio (or VS build tools) 2022, to name a few). You can use Git for Windows, or if you would like to do so, try using Windows Subsystem for Linux (WSL) with a distro such as Ubuntu with Git and other tools installed. You can also try using Visual studio Code for writing NVDA contributions.

The reason for recommending these (not merely suggesting them) is because you are using a setup that is not ideal (or even unusable/unacceptable) for software development - Windows 7 is no longer supported by Microsoft in any shape or form, and a failing (dying) hard disk means you need to be careful of what you are doing before the data (including contributions you are making) is gone; I would go further and get a new system as soon as possible or back up your data to somewhere safe if you intend to work on future NVDA contributions. Trust me when I advise you: the new system doesn't have to be a shiny computer with topmost specs - the system should meet your needs when doing various tasks, including coding, hopefully lasting several years. I thank you for taking our pull request review seriously and incorporating our feedback; at the same time, take our advice to heart when we recommend getting a system that can help you do many things for years to come.

Thanks.

@FelipeZanabria FelipeZanabria marked this pull request as ready for review May 24, 2024 05:27
@FelipeZanabria FelipeZanabria marked this pull request as draft May 24, 2024 05:41
@FelipeZanabria FelipeZanabria marked this pull request as ready for review May 24, 2024 07:41
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks, I'm going to apply some minor changes. When the final build completes, please do a final round of testing and then we can merge

user_docs/en/changes.md Outdated Show resolved Hide resolved
source/winAPI/constants.py Outdated Show resolved Hide resolved
source/JABHandler.py Outdated Show resolved Hide resolved
source/JABHandler.py Outdated Show resolved Hide resolved
source/JABHandler.py Outdated Show resolved Hide resolved
source/JABHandler.py Outdated Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
sconstruct Outdated Show resolved Hide resolved
@seanbudd
Copy link
Member

@seanbudd
Copy link
Member

Some wider community testing would be also be welcome, for using Java Access Bridge Applications on various combinations of:

  • 32/64bit system architecture
  • 32/64bit software
  • Windows 8.1, 10, 11

@FelipeZanabria
Copy link
Author

@FelipeZanabria - can you test this build out: https://ci.appveyor.com/api/buildjobs/lxn6iv0md5en4cd8/artifacts/output%2Fnvda_snapshot_pr16557-32144%2C803cdcf3.exe

With my W7 it is impossible to test this snapshot, but 3 days ago when you updated the submodule, I checked if the two Java dlls were in the installer. They weren't there, so I looked in sconscript, and after seeing that the first dll was there, I added the last one, waited for the compilation and it was included there.
Also before publishing each commit I tested the changes in my setup.

@josephsl
Copy link
Collaborator

Hello Felipe,

Do you have access to a computer running Windows 8.1 or later, preferably Windows 10 22H2 (build 19045) or later? That way you can do a live test (running the modified NVDA executable) of the modifications from this pull request instead of doing compilation and library inclusion tests.

As I stated earlier, testing NVDA pull requests on Windows 7 is really questionable because NVDA 2024.1 and later (including the master branch code) does NOT support Windows 7 in any form or shape. This is an important thing to remember because the best way to see if your PR is working is doing live tests - running the modified NVDA with Java apps (with JAB applied) yourself. I think testing the inclusion of the library is a useful strategy on Windows 7, but by not considering live tests, the effectiveness and impact of the pull request is diminished (I'm sorry to say). So as I also stated, before writing another pull request, PLEASE get a system running updated Windows releases such as 10 and 11 so you can do live tests, improving the effectiveness and impact of pull requests.

Thanks.

@FelipeZanabria
Copy link
Author

@josephsl I don't have it, but I'm going to write to the mailing list in Spanish inviting them to try the changes.

@josephsl
Copy link
Collaborator

Hi Felipe,

I see. Thanks for the pull request, and please do use Windows 10 or 11 yourself while writing the next pull request.

Thakns.

@britechguy

This comment has been minimized.

@Adriani90
Copy link
Collaborator

@mwhapples have you any chance to test this PR on some 32bit java applications on 32 / 64 bit systems?

At least on my Windows 11 on a 64 bit system it works well with the 64bit java configuration window which indicates that the java access bridge for 64 bit works aas expected, so this PR doesn't seem to have any effect on 64 bit.
So if we can test that for 32bit works as well, then I guess it should be fine.

@Adriani90
Copy link
Collaborator

I also tested with JRE 8.411 32bit, it still works fine on my 64 bit system.

So I think we need here testing on pure 32 bit systems I guess.

@FelipeZanabria do you have a test application we can test with? Or can you give some recommendations?

@FelipeZanabria
Copy link
Author

I also tested with JRE 8.411 32bit, it still works fine on my 64 bit system.

So I think we need here testing on pure 32 bit systems I guess.

@FelipeZanabria do you have a test application we can test with? Or can you give some recommendations?

@Adriani90 Thanks for trying and commenting.
The Java control panel itself is enough, although I also tried the Linux sampler for Windows 32-bit.
https://download.linuxsampler.org/packages/win32/snapshots/linuxsampler_20240326_setup.exe
The installer covers both architectures, so you must have a 32-bit system for x32 to be installed. In the components to be installed, you must check Jsampler, which is the Java frontend application.

@seanbudd

This comment was marked as off-topic.

@XLTechie

This comment was marked as off-topic.

@seanbudd
Copy link
Member

I don't have it, but I'm going to write to the mailing list in Spanish inviting them to try the changes.

@FelipeZanabria - can you link to this thread?
When we get testing confirmed with a 32bit system I think we can merge

@FelipeZanabria
Copy link
Author

FelipeZanabria commented May 29, 2024

@FelipeZanabria
Copy link
Author

@seanbudd it seems that no one on the Spanish mailing list uses 32-bit Windows, except for one who also uses w7, which doesn't help moving forward.
Perhaps you could ask on the international mailing list.
What do you think? Could someone do it? Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked/needs-testing conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
9 participants