displayModel: second try at recording the actual background color for text when a transparant background color is set.#7440
Conversation
michaelDCurran
left a comment
There was a problem hiding this comment.
I had previously reviewed and approved pr #6844. This last commit looks good.
|
@michaelDCurran It looks like you updated this, can you give a status report on what is in next? |
…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.
|
@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. |
|
it worked!!!!! Thanks for expediting this. |
|
This commit causes a lag when switching windows a document interface such as mIRC. Specifically edcf518. |
|
We need steps to reproduce please. |
|
I can also reproduce this. |
|
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. |
|
@michaelDCurran commented on 27 nov. 2017 08:04 CET:
I'd prefer this to be at least configurable with a hidden config parameter or something similar. |
|
No, this was not in 2017.3 and there is, therefore, no lagging. |
|
Ah, you're right. It was Merged just after.
|
Or an attribute on the NVDA objject? That way it only turns on in certain areas. |
|
Sadly, we currently have no mechanism to convey configuration to
nvdaHelperRemote right now. This would have to be invented.
Some options are:
* Add another shared memory variable to store flags for
nvdaHelperRemote. Injection_initialize would take an extra argument and
set this variable on initialization. DisplayModel could read this
variable in-process.
* Add a new RPC function to specifically enable displayModel transparent
color detection. Note though that this would only work for future text
writes, anything written up to when the function was could would not
have its transparent color detected. thus the user would have to some
how refresh the screen either by alt+tabbing or, code could call
redrawWindow (though that is some what expensive).
I think I like the first option the best.
So, moving forward, either we:
* Back this out completely for now. Ship an rc3, wait a few days, and
then release.
* Don't back this out and ship the release, dealing with the consequence
of NVDA being slower for some people (again).
* Create a new pr to make this configurable, review it, merge it to both
rc and master. Release rc3. Wait a good while (over a week preferably),
and then release.
|
|
@michaelDCurran commented on 27 nov. 2017 22:58 CET:
I think I prefer this. |
|
I have to say I'm very confused what this is about. From comments does it
only come into play in Dictation bridge? If so, then maybe this belongs in
an add on in any case?
But reading a bit further it seems its in need of deep system access, and if
that does mean speed is reduced, then we do not want it to affect stuff
people use a lot for something that they might not.
In the end, I'd agree why chance it when there are no doubt other niggles
going to come up, like the one with Chrome and comboo boxes that might
affect more people given so many switching away from firefox.
Brian
|
|
@Rob-Hudson and @tspivey: |
|
This snapshot seems to have reversed the issue. All is well again, on this end. |
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>
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