Add `showTechnicalFormattingAtReview` global command, with RGB color info (#8674) by JulienCochuyt · Pull Request #12372 · nvaccess/nvda

JulienCochuyt · 2021-05-06T13:50:24Z

Link to issue number:

Summary of the issue:

Developers and content authors often need to know the exact color of the text they review in more precise terms than the human-readable form commonly served to end users.

Description of how this pull request fixes the issue:

Add a showTechnicalFormattingAtReview global command to provide this information along with the rest of the formatting in a browsable message.

Testing strategy:

Ran and manually tested from a source copy.

Known issues with pull request:

Change log entries:

New features
More technical information regarding the formatting of the text at the review position can now be obtained by pressing NVDA+alt+shift+f. For now, it differs from the usual formatting information by providing the exact RGB values for foreground and background colors.

Code Review Checklist:

AppVeyorBot · 2021-05-06T13:58:30Z

Translation comments missing or unexpectedly included.
Build (for testing PR)

See test results for failed build of commit b0f6fb276b

AppVeyorBot · 2021-05-06T14:46:28Z

System test failure
Build (for testing PR)

See test results for failed build of commit 5f2ca71d0d

JulienCochuyt · 2021-05-06T16:05:03Z

If I read correctly, test_boundingRectFromOffsetAtBottom seems to fail in AppVeyor but passes locally.
@feerrenrut, any idea?

LeonarddeR · 2021-05-06T17:13:31Z

We have failures in a bunch of system tests, it seems.

LeonarddeR

Awesome!

AppVeyorBot · 2021-05-06T23:56:32Z

System test failure
Build (for testing PR)

See test results for failed build of commit 5f2ca71d0d

AppVeyorBot · 2021-05-07T11:15:00Z

System test failure
Build (for testing PR)

See test results for failed build of commit 4f76b663ad

…info (nvaccess#8674) Co-authored by: Leonard de Ruijter <leonardder@users.noreply.github.com>

AppVeyorBot · 2021-05-09T07:59:48Z

System test failure
Build (for testing PR)

See test results for failed build of commit 5aa188ba18

michaelDCurran

I would much prefer to see the color info be simply included in the NVDA+f1 dev info. Note that the NVDA+f1 dev info script already acts upon the navigator object, so extending it to alsoinclude some info about the current review position seems logical enough.
This then avoids adding an extra script for just one bit of extra info, and avoids extra logic in speech.
Another alternative could be to have an Advanced setting that allows the user to configure if NvDA exposes raw color info when available where ever it speaks or displays color names. Format changes, and formatting info script as examples.
I think I still prefer my first suggestion though.

feerrenrut · 2021-05-13T10:10:44Z

@michaelDCurran I think the dev info is for a different type of user. I'd differentiate between NVDA developers, for whom reading the log / devInfo output is fine, and general developers who happen to use NVDA. This second group want access to more technical information, but not to have to sift through all of the noise of NVDA specific information.

I think this more appropriate as a command. I struggle to believe anyone would want to always have the color value reported, but there are occasions for developers where you want only the color value reported.

I haven't looked at the implementation yet, and perhaps it is too complicated and just belongs in and addon.

feerrenrut · 2021-05-13T10:26:11Z

+			f"{color.name}"
+			f" (RGB: {color.red}, {color.green}, {color.blue}"
+			f" / #{color.red:02x}{color.green:02x}{color.blue:02x})"


UX feedback: I hadn't thought about cases like _("{color1} to {color2}") which using this as the replacement will be very long. Perhaps it is best to have a single color format reported. Developers should be able to convert themselves. I'd personally choose Hex, since it is shorter.

From a code design, I really don't like the idea of introducing another parameter. This should be two functions sharing a common interface. Then use something like the strategy pattern with those two implementations. Ideally, nothing in getFormatFieldSpeech needs to know, in reality we might need to make a change to getFormatFieldSpeech to enable a mechanism like this. The upside is, if we do it right we can start to split up getFormatFieldSpeech into a series of formatters.

seanbudd · 2021-06-07T02:41:20Z

@feerrenrut, @michaelDCurran thoughts on adding a new settings panel in document formatting for this?

I propose something like Document Formatting > Technical > Colour formatting with a CustomCheckListBox for colours so you can pick which formatting(s) to report, with the default just being the human readable name.

Then, add a __str__ or __repr__ function to colors.RGB which determines the formatting using that setting and similar logic to _getColorName. Then we can just use colors.RGB instances directly in reporting.

We can just use the new function in the custom command to begin with, as the rest of the code is dependent on color.name.

feerrenrut · 2021-07-22T07:53:34Z

@seanbudd I haven't considered the implications of the technical details you suggested, but I think this will have to go down the UI path you recommend, i.e.

I propose something like Document Formatting > Technical > Colour formatting with a CustomCheckListBox for colours so you can pick which formatting(s) to report, with the default just being the human readable name.

Swapping to a preferred technical color description will work for any of the in-line color reporting EG:

I still think there a place for a command (such as script_showTechnicalFormattingAtReview) to present a browsable message containing multiple formats for foreground and background color.

CyrilleB79 · 2021-07-22T09:40:06Z

I am a bit concerned with adding a technical option in the formatting settings panel which has already a lot of options; this is also in contradiction with the reason why #8674 was re-opened and with #8674 (comment).

There are two requirements that are both legitimate and that are not totally compatible here:

Keep the UI free from technical options to avoid to have too complex options for non-developer users. E.g. a simple unassigned technical command is OK
The need for developers to be able to customize the way the information is reported

My initial preference was to have this eature in an add-on but this was already discussed in #8674 and it is not the direction we are taking now.
So if we agree to have this feature in the core, I would prefer the customization option to be in the advanced panel. Or in another new panel that would gather technical options, i.e. the advanced panel would be dedicated to new NVDA features whereas the new technical panel would be more towards advanced needs but not for NVDA debugging / testing.

feerrenrut · 2021-07-22T09:56:09Z

I'm ok with this starting as an advanced option. Users of the advanced settings panel should be more aware of options migrating later on, this makes it easier for us to consolidate the color options later. Thinking about the document formatting panel in settings, these are probably more specific than configuration we are talking about. I think the intention is:

Document formatting: in the context of a document, should color be reported at all.
New Color presentation option: How should the a color (in any context) be reported.

lukaszgo1 · 2021-10-14T09:58:15Z

A few general thoughts:

Having a configuration panel for this is IMO an overkill.
Do we really need to be able to customize how colors are reported when reading - i.e. isn't it sufficient to be able to see RGB (or whatever notation we chose in the end) in the browseable message?
Assuming that we don't need to modify the way in which colors are reported my personal preference would be not to introduce another command for this but just add RGB values to the end of formatting message when pressing NVDA+f twice. Aside from reducing complexity this is the way in which all other AT's for Windows report RGB values for a color - while I am not advocating to blindly copy others since the system is familiar for users and works I see no reason not to implement the same solution.

lukaszgo1 · 2021-11-12T16:16:02Z

It looks like this is stalled due to the fact that we cannot agree on a satisfying implementation. Would it be possible to someone from NV Access to take a look at the discussion including my proposal from the last comment and share their thoughts so that this work can be progressed?

michaelDCurran · 2021-11-15T03:37:57Z

I tend to agree with @lukaszgo1 . We really don't need to over engineer this. We can just include the raw rgb after color names in the Show Formatting browsable message with NvDA+f twice. globalCommand's _reportFormattingHelper should be able to look for colors.RGB objects in the fetched format and change them into strings of "{name}, RGB({red}, {green}, {blue})" itself, if the formatting is being shown as a browseable message.

seanbudd · 2022-02-18T05:42:35Z

This is marked as a draft, this should be followed up with @michaelDCurran design

Adriani90 · 2023-04-12T19:09:39Z

cc: @ABuffEr maybe you are interested in this work as well, not sure if @JulienCochuyt is still very active these days.

seanbudd · 2024-06-17T00:17:17Z

Closing as abandoned

LeonarddeR approved these changes May 6, 2021

View reviewed changes

JulienCochuyt marked this pull request as ready for review May 7, 2021 11:48

JulienCochuyt requested a review from a team as a code owner May 7, 2021 11:48

JulienCochuyt requested a review from seanbudd May 7, 2021 11:48

Add showTechnicalFormattingAtReview global command, with RGB color …

f718b61

…info (nvaccess#8674) Co-authored by: Leonard de Ruijter <leonardder@users.noreply.github.com>

JulienCochuyt force-pushed the i8674-rgbColors branch from 8f53259 to f718b61 Compare May 9, 2021 07:47

michaelDCurran requested changes May 9, 2021

View reviewed changes

feerrenrut reviewed May 13, 2021

View reviewed changes

seanbudd added the blocked/merge-conflicts Merge conflicts exist on this PR label Sep 22, 2021

seanbudd marked this pull request as draft February 18, 2022 05:41

seanbudd closed this Jun 17, 2024

seanbudd added the Abandoned requested reports or updates are missing since more than 1 year, author or users are not available. label Jun 17, 2024

Uh oh!

Conversation

JulienCochuyt commented May 6, 2021 • edited 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!

AppVeyorBot commented May 6, 2021

Uh oh!

AppVeyorBot commented May 6, 2021

Uh oh!

JulienCochuyt commented May 6, 2021

Uh oh!

LeonarddeR commented May 6, 2021

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented May 6, 2021

Uh oh!

AppVeyorBot commented May 7, 2021

Uh oh!

AppVeyorBot commented May 9, 2021

Uh oh!

michaelDCurran left a comment

Choose a reason for hiding this comment

Uh oh!

feerrenrut commented May 13, 2021

Uh oh!

feerrenrut May 13, 2021

Choose a reason for hiding this comment

Uh oh!

seanbudd commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feerrenrut commented Jul 22, 2021

Uh oh!

CyrilleB79 commented Jul 22, 2021

Uh oh!

feerrenrut commented Jul 22, 2021

Uh oh!

lukaszgo1 commented Oct 14, 2021

Uh oh!

lukaszgo1 commented Nov 12, 2021

Uh oh!

michaelDCurran commented Nov 15, 2021

Uh oh!

seanbudd commented Feb 18, 2022

Uh oh!

Adriani90 commented Apr 12, 2023

Uh oh!

seanbudd commented Jun 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

JulienCochuyt commented May 6, 2021 •

edited

Loading

seanbudd commented Jun 7, 2021 •

edited

Loading