• PASS: Translation comments check.
• PASS: Unit tests.
• PASS: Lint check.
• FAIL: System tests (tags: installer NVDA). See test results for more information.
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/839w6dq6cccd7qrx/artifacts/output/nvda_snapshot_pr14671-27750,a60df41c.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.9,
  INSTALL_END 0.9,
  BUILD_START 0.0,
  BUILD_END 24.5,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.3,
  TEST_START 0.0,
  TEST_END 20.5,
  FINISH_END 0.2

See test results for failed build of commit a60df41c12

Hi @FalkoBabbage, I don't think that change log description is appropriate for bug fixes.
If there are user facing changes, list them there.
Otherwise can we put in "changes for developers"?

I'm also not sure if this description is specific enough:

-OffsetsTextInfo.boundingRects sometimes returned rectLTRB instances in some JAB textInfo objects, this is no longer the case.

What does it return instead now?

Can you add typing information to _get_boundingRects?

Considering that rectLTRB was returned previously, will changing this return type be an API breakage?
What do other parts of NVDA expect as a type here?

Hi @seanbudd.
I agree that this should be in "changes for developers", since the bugs that it fixes occur later down the line.

I'm also not sure if this description is specific enough:

The origin of the error that I occurred is as follows:

1. I try to retrieve the boundingRects property of a JAB text info. The _get_boundingRects method that is used is from textInfos.offsets.OffsetsTextInfo
2. the _get_boundingRects method uses _getBoundingRectFromOffset for with the startoffset to get the startLocation. If the endoffset is bigger than startOffset + 1, all rectangles that are obtained from _getBoundingRectFromOffset for each offset are put through the function locationHelper.RectLTWH.fromCollection, making it automatically a rectLTWH.
3. If the endoffset is not bigger than startOffset + 1, the startLocation is only checked if this is a point. If it is not a point, the start location is put into the rects list as is.
4. _getBoundingRectFromOffset does not have any type hinting. In JABTextInfo in NVDAObjects/JAB/init.py this function explicitly returns a rectLTRB.

Maybe the following is better to describe the changes:
"_get_boundingRects" is assumed to return locationHelper.rectLTWH types, but for textInfos.offsets.OffsetsTextInfo there there can be situations where a instance of locationHelper.rectLTRB is returned. This is solved by adding an explicit conversion to rectLTWH on the location where a rectLTRB can occur.

Can you add typing information to _get_boundingRects?

In textInfos/init.py the _get_boundingRects method of the class TextInfo already specifies the return type locationHelper.RectLTWH in its comment. This is not enforced by the typing module and is just a comment.

Considering that rectLTRB was returned previously, will changing this return type be an API breakage?
What do other parts of NVDA expect as a type here?

Since _get_boundingRects does not have the return type enforced there indeed might be pieces of code that expect a rectLTRB, But considering the indicated return type of _get_boundingRects they shouldn't and therefore I wouldn't consider it a breaking API change.

Thanks for explaining the bug in more detail.

In textInfos/init.py the _get_boundingRects method of the class TextInfo already specifies the return type locationHelper.RectLTWH in its comment. This is not enforced by the typing module and is just a comment.

Can you please add typing information to the function header here. For example def func(self) -> List[locationHelper.RectLTWH]:

Since _get_boundingRects does not have the return type enforced there indeed might be pieces of code that expect a rectLTRB, But considering the indicated return type of _get_boundingRects they shouldn't and therefore I wouldn't consider it a breaking API change.

Ah I see, makes sense. Thanks for explaining

Can you please add typing information to the function header here. For example def func(self) -> List[locationHelper.RectLTWH]:

_get_boundingRects is defined in textInfos.TextInfo. I suggest to add the correct typing there if that is desired since I assume that any override would also be "forced" to use this type. Should
I also add typing for all other overrides for _get_boundingRects?

I think it would be sufficient to just add it for the function you updated.

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/crm3ig429r9s2rqs/artifacts/output/nvda_snapshot_pr14671-27811,9cac227d.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.8,
  INSTALL_END 0.8,
  BUILD_START 0.0,
  BUILD_END 24.2,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.3,
  TEST_START 0.0,
  TEST_END 23.2,
  FINISH_END 0.2

See test results for failed build of commit 9cac227d01

• PASS: Translation comments check.
• PASS: Unit tests.
• FAIL: Lint check. See test results for more information.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ojf6m0130rchn24h/artifacts/output/nvda_snapshot_pr14671-27836,bd715df7.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 1.1,
  INSTALL_END 1.0,
  BUILD_START 0.0,
  BUILD_END 25.6,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.3,
  TEST_START 0.0,
  TEST_END 22.8,
  FINISH_END 0.1

See test results for failed build of commit bd715df7a1

I do not understand the failed link check from appveyor:

source/textInfos/offsets.py:183:2: C901 'OffsetsTextInfo._get_boundingRects' is too complex (19)
def _get_boundingRects(self) -> List[locationHelper.RectLTWH]:
^
1 C901 'OffsetsTextInfo._get_boundingRects' is too complex (19)

Should I also reduce complexity of the function in order to do finish this pr?

I have looked into reducing the complexity but as far as I know that would mean that I have to split up the function into different subfunctions. In that case we would end up with more methods in the OffsetsTextInfo class which should also make sense for its child-classes downstream.
Besides, I am not sure why appveyor flags my code as to complex. I have already reduced the existing complexity of this function by removing an if/else statement.

Besides, I am not sure why appveyor flags my code as to complex. I have already reduced the existing complexity of this function by removing an if/else statement.

The linter only checks the code that you have modified. And among others, it checks the complexity of the functions that you have modified. It does not care if you reduced the complexity, it just check if it is too high or not. The original function was very too complex. But it has probably been written before the linter check was introduced.

You have two options:

• Either you split the function to reduce its complexity
• Or (if NVAccess agrees with it), given you have modified only few lines of this function, you may add a #noqa directive to ignore the complexity.

If it's not too much work and depending on the impact of the modifications, the first solution is preferred. This allows the code to converge towards a linted code.

Either option @CyrilleB79 suggested here is appropriate. Search for C901 for functions handled in a similar way.

I have decided to add a #noqa to the function. Splitting this function into multiple functions would mean that these new functions should also make sense for all classes that inherent this class, which would take quite some time to figure out on my part.