Skip to content

displayModel: second try at recording the actual background color for text when a transparant background color is set.#7440

Merged
michaelDCurran merged 4 commits into
masterfrom
i6467
Aug 29, 2017
Merged

displayModel: second try at recording the actual background color for text when a transparant background color is set.#7440
michaelDCurran merged 4 commits into
masterfrom
i6467

Conversation

@michaelDCurran

@michaelDCurran michaelDCurran commented Jul 25, 2017

Copy link
Copy Markdown
Member

This PR is based on PR #6844 which was reverted due to performance reasons.

It improves upon #6844 by only calling GetPixel if the text being written is using a bk mode of transparent, greatly reducing the amount of calls.

This extra commit from @mwcampbell was provided as a patch attached to #6467.
Fixes #6467

@michaelDCurran michaelDCurran left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had previously reviewed and approved pr #6844. This last commit looks good.

@derekriemer

Copy link
Copy Markdown
Collaborator

@michaelDCurran It looks like you updated this, can you give a status report on what is in next?

mwcampbell and others added 4 commits August 1, 2017 16:29
…color at the provided X, Y coordinates before calling the original function, so we at least have a chance of knowing the correct background color if the text is being drawn in transparent mode. This is necessary to detect highlighted text in some parts of Dragon NaturallySpeaking.
…t is transparent. GetPixel can be slow so we want to limit its use.
@michaelDCurran

Copy link
Copy Markdown
Member Author

@derekriemer: This pr was a little bit stuffed up. I think because git doesn't handle adding back duplicate code after a revert. I have now re-done the branch by cherry picking all mat's commits, and force pushed to this pr. I have also triggered a try build... Please test the try build and let me know if this works again. If so, I will re-incubate to next.
https://ci.appveyor.com/project/NVAccess/nvda/build/try-i6467-14259%2c69dba6ce

@derekriemer

Copy link
Copy Markdown
Collaborator

it worked!!!!! Thanks for expediting this.

michaelDCurran added a commit that referenced this pull request Aug 3, 2017
@michaelDCurran michaelDCurran merged commit edcf518 into master Aug 29, 2017
@nvaccessAuto nvaccessAuto added this to the 2017.4 milestone Aug 29, 2017
@Rob-Hudson

Rob-Hudson commented Nov 19, 2017

Copy link
Copy Markdown

This commit causes a lag when switching windows a document interface such as mIRC. Specifically edcf518.

@derekriemer

Copy link
Copy Markdown
Collaborator

We need steps to reproduce please.

@tspivey

tspivey commented Nov 22, 2017

Copy link
Copy Markdown
Collaborator

I can also reproduce this.
Run mIRC, connect to a server and repeatedly press control tab to switch windows. After this commit, NVDA is much slower at reporting the window switch. Roughly 100 MS vs. 400-500, using gestureTiming.
That was with the window full of channel text.

@derekriemer

Copy link
Copy Markdown
Collaborator

@michaelDCurran

@michaelDCurran

Copy link
Copy Markdown
Member Author

I'm not going to pull this out at this late stage as it was already in 2017.3. However, as this is now the second time this has caused a problem, it is very likely this will be removed again. Perhaps in future we can come up with a way to configure this, so that it is only used for Dictation Bridge.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

@michaelDCurran commented on 27 nov. 2017 08:04 CET:

it is very likely this will be removed again. Perhaps in future we can come up with a way to configure this, so that it is only used for Dictation Bridge.

I'd prefer this to be at least configurable with a hidden config parameter or something similar.

@Rob-Hudson

Copy link
Copy Markdown

No, this was not in 2017.3 and there is, therefore, no lagging.

@michaelDCurran

michaelDCurran commented Nov 27, 2017 via email

Copy link
Copy Markdown
Member Author

@derekriemer

Copy link
Copy Markdown
Collaborator

I'd prefer this to be at least configurable with a hidden config parameter or something similar.

Or an attribute on the NVDA objject? That way it only turns on in certain areas.

@michaelDCurran

michaelDCurran commented Nov 27, 2017 via email

Copy link
Copy Markdown
Member Author

@LeonarddeR

Copy link
Copy Markdown
Collaborator

@michaelDCurran commented on 27 nov. 2017 22:58 CET:

  • Back this out completely for now. Ship an rc3, wait a few days, and
    then release.

I think I prefer this.

@Brian1Gaff

Brian1Gaff commented Nov 28, 2017 via email

Copy link
Copy Markdown

michaelDCurran added a commit that referenced this pull request Nov 28, 2017
…olor for text when a transparant background color is set. (#7440)"

This reverts commit edcf518.

As reported in #7440, this again caused a significant performance hit.  We must avoid using GetPixel. It is just too slow.
@michaelDCurran

michaelDCurran commented Nov 28, 2017

Copy link
Copy Markdown
Member Author

@Rob-Hudson and @tspivey:
A try build with this pr reverted: https://ci.appveyor.com/api/buildjobs/vumey63f8kg079cv/artifacts/output%2Fnvda_snapshot_try-pr7440_revert-14666%2C89ad5a5d.exe
Please test and let me know as soon as possible if the performance issue has been fixed. This is a build of pr #7803 which simply reverts this pr, removing the calls to GetPixel.
If all is okay, #7803 will be merged into rc, and rc3 will be made, and then a final release next week.

@Rob-Hudson

Copy link
Copy Markdown

This snapshot seems to have reversed the issue. All is well again, on this end.

michaelDCurran added a commit that referenced this pull request Nov 29, 2017
…olor for text when a transparant background color is set. (#7440)" (#7803)

This reverts commit edcf518.

As reported in #7440, this again caused a significant performance hit.  We must avoid using GetPixel. It is just too slow.
feerrenrut added a commit that referenced this pull request Jul 27, 2021
Resolves concerns about contributed PR: #9197
Workaround for #6467

# Summary of the issue:
From #6467 (comment)

> When text is written with the GDI background mode set to transparent, the display model doesn't necessary report the
> correct background color for that text. In this case, the GDI hook should look at the actual background color before the
> text is drawn, rather than using the result of GetBkColor.

# Description of fix:
There have been two prior attempts to fix this, both causing severe performance regressions.
- #6844: "Display model: Try to provide the correct background color for transparent text drawing (issue #6467)"
- #7440: "displayModel: second try at recording the actual background color for text when a transparant background color is set."

This implementation merely exposes information about the potential transparency of the text background.
Using the samples in https://github.com/nvaccess/testDisplayModel
to test with various GDI dependent GUI APIs shows that the GetBkColor result can be:
- Opaque and matches visual presentation(rawGDI)
- Transparent and matches visual presentation: (mfcTest, GDItest)
- Transparent and does not match visual presentation: (anecdotal)
- The exposure of the transparency (via the alpha value on the colors.RGB class) will allow appModules
or add-ons to customize NVDA's presentation as necessary.

For users, there is no change in behavior by default. An option is introduced to the advanced settings panel to allow reporting of transparency in colors. The intention here is that developers will use this to identify where
color reporting could be improved.

The prior approach used the colorref high-level byte of a colorref struct. However, MSDN requires this byte be zero.
This prior approach did not pass the colorref into any Windows API's, so it was safe. But to prevent this from being accidentally introduced in the future, a compatible alternative type is introduced and used instead.

In order to stay compatible with python code interpreting this color value, a single bit is set if the color is considered transparent.

# Testing:
- Built applications from https://github.com/nvaccess/testDisplayModel
- Tested with/without transparency reporting enabled (advanced panel)
- Ensure color reporting is enabled in document formatting panel of NVDA settings
- Use NVDA+Numpad 7 to switch to screen review
- Use Numpad 7, Numpad 8, Numpad 9 to read the dialog

Co-authored-by: Leonard de Ruijter <leonard@babbage.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.

8 participants