Fix regression causing older versions of NotePad++ to crash by LeonarddeR · Pull Request #13382 · nvaccess/nvda

LeonarddeR · 2022-02-23T07:33:18Z

Link to issue number:

Fixes regression caused by #13364
Closes #13397

Summary of the issue:

The version matching to apply the new NP++ 8.3+ behavior was incorrect, as it doesn't work for versions like 8.2.1 and 8.1.9.2.

Description of how this pull request fixes the issue:

Only match against the first character of the minor version.

Testing strategy:

Tested on version 8.1.9.2, 8.2.1, 8.3 and 8.3.1 of NP++

Known issues with pull request:

None known

Change log entries:

None needed

Code Review Checklist:

Pull Request description:
- description is up to date
- change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English

LeonarddeR · 2022-02-23T07:34:09Z

@lukaszgo1 requesting your review as well.

lukaszgo1

Please clarify the comment with the (8.1.9.2 version. The rest looks okay.

lukaszgo1 · 2022-02-23T10:25:26Z

 		if self.appModule.is64BitProcess:
 			appVerMajor, appVerMinor, *__ = self.appModule.productVersion.split(".")
-			if int(appVerMajor) >= 8 and int(appVerMinor) >= 3:
+			# appVerMinor could be either one digit (e.g. in 8.3), two digits (e.g. in 8.21), or even three (8.1.9.2)


I'm not sure this comment is accurate. In the example in which the version is (8.1.9.2 appVerMinor would be one digit - 1.

This is incorrect. appVerMinor would be 192.

In any case, your suggestion is way nicer.

This is incorrect. appVerMinor would be 192.

Sorry for insisting, but I've started doubting that I understand how split works ;) so had to check and it turns out I'm right.
From the Python repl:

C:\Users\Lukasz>py Python 3.8.10 (tags/v3.8.10:3d8993a, May 3 2021, 11:48:03) [MSC v.1928 64 bit (AMD64)] on win32 Type "help", "copyright", "credits" or "license" for more information. >>> appVer = "8.1.9.2" >>> appVerMajor, appVerMinor, *__ = appVer.split(".") >>> appVerMajor '8' >>> appVerMinor '1' >>> __ ['9', '2']

Note that everything after the second dot is unpacked into a temporary __ variable and is not used in the comparisons.

Co-authored-by: Łukasz Golonka <lukasz.golonka@mailbox.org>

lukaszgo1 · 2022-02-23T11:05:06Z

 			appVerMajor, appVerMinor, *__ = self.appModule.productVersion.split(".")
-			if int(appVerMajor) >= 8 and int(appVerMinor) >= 3:
+			# appVerMinor could be either one digit (e.g. in 8.3), two digits (e.g. in 8.21), or even three (8.1.9.2)
+			appVerMinor = appVerMinor.ljust(3, '0')


This is not needed anymore.

AppVeyorBot · 2022-02-23T11:08:25Z

See test results for failed build of commit 7db6398658

lukaszgo1

Please update the PR description to describe the new approach. Thanks for fixing up my mess! LGTM.

seanbudd · 2022-02-23T22:54:44Z

The version matching to apply the new NP++ 8.3+ behavior was incorrect, as it doesn't work for versions like 8.2.1 and 8.1.9.2.

I am still confused about the issue here.
Should 8.2.1 and 8.1.9.2 apply the same fix as applied for 8.3 - i.e. should 8.2.1 have the same behaviour as 8.3?
Why are we only comparing the leading digit of an integer?

lukaszgo1 · 2022-02-24T09:50:51Z

I am still confused about the issue here. Should 8.2.1 and 8.1.9.2 apply the same fix as applied for 8.3 - i.e. should 8.2.1 have the same behaviour as 8.3?

No - the new behavior should apply to versions 8.3 and above only.

Why are we only comparing the leading digit of an integer?

Versions of Notepad++ are exposed differently on their web page and in in its UI, and differently when querying NPP binary for the version info programmatically. For example when @LeonarddeR said version 8.1.9.2:

the version number in the UI was indeed 8.1.9.2
The version as exposed by appModule.productVersion is "8.192"

Since the new behavior should apply only to the versions 8.3 and above we're interested in the first digit of the minor version only, as that would cover all versions regardless of how long the minor part is.

seanbudd · 2022-02-24T22:49:10Z

I just inspected my notepad.exe properties.

It appears that the "File version" is set correctly to "8.1.4.0" whereas the "Product version" is "8.14".

Is it simple to get and use the file version instead here?

Otherwise, can we add a comment outlining this difference and why we are comparing the leading digit of the integer?

rdipardo · 2022-02-26T14:29:19Z

@seanbudd,

It appears that the "File version" is set correctly to "8.1.4.0" whereas the "Product version" is "8.14".

Those are not the version values that @lukaszgo1 is referring to.
The Notepad++ plugin API exposes version information as explained here:

The value is made up of 2 parts: the major version (the high word) and minor version (the low word).
For example, 4.7.5 is encoded like:
HIWORD(version) == 4
LOWORD(version) == 75

An idiosyncratic system, to be sure, but hopefully this clarifies why the unusual comparison logic is necessary.

seanbudd · 2022-02-28T00:17:41Z

Thanks @rdipardo

seanbudd

Thanks @LeonarddeR

lukaszgo1 · 2022-03-01T15:04:07Z

It appears that the "File version" is set correctly to "8.1.4.0" whereas the "Product version" is "8.14".

Is it simple to get and use the file version instead here?

That is an interesting thought. While this should not be done for 2022.1 it would be pretty nice to use VS_FIXEDFILEINFO when retrieving file version as that gives us the versions in a reliable format and not as a string which we need to split our selves.

Fix regression causing older versions of NotePad++ to crash

db27df4

LeonarddeR requested a review from a team as a code owner February 23, 2022 07:33

LeonarddeR requested a review from seanbudd February 23, 2022 07:33

LeonarddeR added this to the 2022.1 milestone Feb 23, 2022

lukaszgo1 suggested changes Feb 23, 2022

View reviewed changes

Update source/appModules/notepadPlusPlus.py

3086bd5

Co-authored-by: Łukasz Golonka <lukasz.golonka@mailbox.org>

lukaszgo1 reviewed Feb 23, 2022

View reviewed changes

CLeanup

66b8b62

lukaszgo1 approved these changes Feb 23, 2022

View reviewed changes

Add comments

e72db16

josephsl mentioned this pull request Feb 27, 2022

Recent alpha versions crash Notepad++ versions 8.2.x and below #13397

Closed

seanbudd approved these changes Feb 28, 2022

View reviewed changes

Comment thread source/appModules/notepadPlusPlus.py Outdated

Clarify comment further

6109060

seanbudd merged commit 4c3a67b into nvaccess:master Feb 28, 2022

rdipardo mentioned this pull request Mar 23, 2022

Wrong application version in file details notepad-plus-plus/notepad-plus-plus#11431

Closed

rdipardo mentioned this pull request Apr 14, 2022

VERSION_VALUE should always have at least 3 digits notepad-plus-plus/notepad-plus-plus#11535

Closed

LeonarddeR deleted the fixNP++ branch August 23, 2025 06:27

Uh oh!

Conversation

LeonarddeR commented Feb 23, 2022 • edited by seanbudd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

LeonarddeR commented Feb 23, 2022

Uh oh!

lukaszgo1 left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

LeonarddeR Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

lukaszgo1 Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukaszgo1 Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Feb 23, 2022

Uh oh!

lukaszgo1 left a comment

Choose a reason for hiding this comment

Uh oh!

seanbudd commented Feb 23, 2022

Uh oh!

lukaszgo1 commented Feb 24, 2022

Uh oh!

seanbudd commented Feb 24, 2022

Uh oh!

rdipardo commented Feb 26, 2022

Uh oh!

seanbudd commented Feb 28, 2022

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukaszgo1 commented Mar 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LeonarddeR commented Feb 23, 2022 •

edited by seanbudd

Loading