Reporting table headers can now be configured separately for rows and columns#14132
Conversation
See test results for failed build of commit d58cc36794 |
See test results for failed build of commit 0c7d4eefb9 |
See test results for failed build of commit 733f082224 |
See test results for failed build of commit 03525bd0cc |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
seanbudd
left a comment
There was a problem hiding this comment.
Thanks @CyrilleB79, generally looks good.
Qchristensen
left a comment
There was a problem hiding this comment.
User Guide change looks fine, good work.
|
Please don't merge yet as I have some suggestions I will post later today. |
| def script_setColumnHeader(self,gesture): | ||
| scriptCount=scriptHandler.getLastScriptRepeatCount() | ||
| if not config.conf['documentFormatting']['reportTableHeaders']: | ||
| if config.conf['documentFormatting']['reportTableHeaders'] == ReportTableHeaders.OFF: |
There was a problem hiding this comment.
Shouldn't this only work when column header reporting is enabled? So, only when columns or both are reported?
There was a problem hiding this comment.
Actually, setting or removing column headers while only row headers are reported is working. If you remove this "if" statement, i.e. allow this script to work regardless of this reportTableHeaders configuration, it works too.
For simplicity (regarding the error message), I am thinking to remove this if statement, i.e. setting column headers even when reporting table headers is disabled. @LeonarddeR, @seanbudd, Can you see any drawback to this solution?
There was a problem hiding this comment.
I have finally allowed setting/removing row/column headers even if reporting table headers is disabled. I have updated the initial description accordingly.
@seanbudd, @LeonarddeR, let me know if this is OK for you.
| def script_setRowHeader(self,gesture): | ||
| scriptCount=scriptHandler.getLastScriptRepeatCount() | ||
| if not config.conf['documentFormatting']['reportTableHeaders']: | ||
| if config.conf['documentFormatting']['reportTableHeaders'] == ReportTableHeaders.OFF: |
There was a problem hiding this comment.
Shouldn't this only work when row header reporting is enabled? So, only when row headers or both are reported?
See test results for failed build of commit b57fc75cd1 |
…ew schema. (#14183) fix-up of #14132 Summary of the issue: When upgrading configurations to the new versions of the schema we often need to check what value was set previously as not to change user preferences. In our profile upgrade steps we relied on the fact that false values in the config are falthy in the boolean context, but at the state where profile is upgraded these are not yet transformed into right types, so a value set to False is set to literal string "False" which is always truthy in a boolean context. I've discovered this when, after #14132 got merged, in the profile where I had reporting of row and column headers disabled it got re-enabled after the schema update. Description of user facing changes People who have reporting of row and column header disabled would still have them disabled after updating to NVDA 2022.4. Description of development approach In places where we check if the option is set to True in our profile upgrade steps I used the default configobj converter between string and booleans to convert options into the right values. In addition I've removed some unnecessary pass statements and some variables capturing exceptions which weren't used. While fixing previous upgrade steps would not help people who are upgrading NVDA regularly it still made sense to do so, to support people who skipped several upgrades and to have consistent style for the upgrade steps.
Closes #14170 Summary: For some GUI settings, a difference between the default (base) profile and another profile, could have unexpected results. An example: - A profile named 'Notepad' with setting (GUI option) 'line indentation reporting' to 'tones' - The default profile with the same GUI option set to 'speech' - Result: indentation reported with speech and tones instead of tones only when returning to notepad - Issue #14170 contains detailed steps. Only one combo-box was used to set line indentation reporting in the UI, but the value of this combo-box was saved in two items in the configuration. These two items were then managed separately when dealing with configuration profile switching. The following combo-boxes in the GUI were impacted by this issue: * "Line indentation reporting" in doc formatting settings - See initial description of #14170 * "Cell borders" in doc formatting settings - See #14170 (comment) * "Show messages" combined with "Message timeout" in braille settings - See #14170 (comment) * "Tether Braille" in braille settings - See #14170 (comment) Change for users: The values of the following settings should not change unexpectedly anymore when switching profile: * Line indentation in Document formatting settings. * Cell borders in doc formatting settings * Show messages in braille settings * Tether Braille in braille settings Approach: * One control in the GUI now matches one and only one item in the config * Upgrade the config adequately * Used the same approach as #14132 (and the fix in #14183) A note on profile upgrade: Because it is possible for arbitrary stacking order for config profiles it is not possible to determine the setting which would be expected by the user. Config stacking can be caused by mixes of the following, default/base, manually enabled profile, app triggered profile, say-all triggered profile
|
@seanbudd, I just realize that this PR was an API breaking change, due to the configuration modification and upgrade, but it has been merged in 2022.4 which was not intended to be an API breaking release. I don't know of an add-on that uses this config keys. This has no sense to revert it now. However, should this API breaking change be advertised somewhere? I would say at least on the API mailing list. And maybe in the change log, even if usually the log of older releases is not amended. |
|
Hi, StationPlaylist add-on uses this config key, but as of now the add-on code will work on both 2022.3 and 2022.4 as integer values above 0 are seen as “true” in Python. I do plan to edit the add-on code son to use 2022.4 changes anyway. Thanks.
|
|
Hi,
I would like to know how an API break is defined.
It seems to me that any modification of NVDA's code is an API break for
add-ons that rely on it.
For example, documentBase.py has undergone a major overhaul in version
2022.2 compared to 2022.1 to add in particular the commands for moving
tables in tables.
And that involved a serious upgrade in one of my add-ons.
So it seems to me that for developers of add-ons embedded in NVDA, any
new version of NVDA is an API break and it is necessary to
systematically check that the NVDA code on which it was based did not
not undergone any modification.
That's why I don't quite understand the principle of API breaking only
when releasing the first version of the year.
Best regards.
Le 19/01/2023 17:25, Joseph Lee a écrit :
… Hi, StationPlaylist add-on uses this config key, but as of now the
add-on code will work on both 2022.3 and 2022.4 as integer values
above 0 are seen as “true” in Python. I do plan to edit the add-on
code son to use 2022.4 changes anyway. Thanks.
—
Reply to this email directly, view it on GitHub
<#14132 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZLFFEOQF2SB6CNUSGNMQLWTFTI3ANCNFSM6AAAAAAQK3CK6A>.
You are receiving this because you are subscribed to this
thread.Message ID: ***@***.***>
|
|
Replying to all in the same message: @josephsl wrote:
Sorry for the unforeseen API change; by chance this did not break your add-on.
For packages, modules, classes, functions, variables, they are private if their name begin with a
|
|
Hi, it didn’t affect StationPlaylist ad-on, so all is good with me. Thanks.
|
This comment was marked as resolved.
This comment was marked as resolved.
|
Hi Cyrille, Is this an accurate summary of the API breakage? |
Link to issue number:
Closes #14075
Summary of the issue:
It was asked in #14075 to be able to have table row/column headers reporting configured separately.
Description of user facing changes
Description of development approach
noqaexception for code complexity ofgetObjectSpeechhas been removed because it was not relevant anymore. Excepted the removal of this #noqadirective,getObjectSpeechis not modified in this PR.Testing strategy
Manual testing of table navigation commands combining all the following possibilities:
ui.browseableMessage)It has also been checked in
nvda.inithat saving the config works as expected.Known issues with pull request:
None
Change log entries:
New features (or Changes ?)
Reporting table headers can now be configured separately for rows and columns. (#14075)Code Review Checklist: