Skip to content

Do not sort dictionaries in characterProcessing.listAvailableSymbolDictionaryDefinitions #18178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: beta
Choose a base branch
from

Conversation

CyrilleB79
Copy link
Collaborator

PR against beta since it impacts the API.

Link to issue number:

Discussed in #16823 (comment) and following comments.

Summary of the issue:

characterProcessing.listAvailableSymbolDictionaryDefinitions indicates that it returns the list of available symbol dictionary definitions as initialized in core or in add-ons.

Though, it actually returns a sorted version of the internal list used by NVDA. This sorting order was only useful for the dictionaries displayed in the GUI but have no sense for the whole list of symbol dictionaries.

Description of user facing changes

Return a list in the same order as the order in which the dictionaries were added, i.e. order corresponding to the internal variable.

If the dictionaries need a specific sorting order in the GUI, it can be done outside of this function, and after API breaking change if needed.

Description of development approach

Removed sorting, but still returns a new list to avoid modifying the original list.

Testing strategy:

Launched NVDA with the test add-on used in #16823.

  • Checked that the order of the returned list matches the order in which dictionaries have been added in NVDA's internal symbol dictionaries list.

Also noted that, for this specific add-on, by chance, the order in the GUI has not changed, because the dictionaries had been added in alphabetical order in the add-on's manifest.

Known issues with pull request:

This makes a late API-breaking change. Though, it is very unlikely that an add-on use this function (introduced in 2024.4) and be impacted by this order change.

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@coderabbitai summary

@CyrilleB79 CyrilleB79 marked this pull request as ready for review May 28, 2025 23:21
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner May 28, 2025 23:21
@CyrilleB79 CyrilleB79 requested review from seanbudd and LeonarddeR May 28, 2025 23:21
@seanbudd
Copy link
Member

@CyrilleB79 is it possible for you to create a separate function for your needs? I'm not sure I understand why this needs to go into the beta or break the existing API

@seanbudd
Copy link
Member

I'm also unsure if we would consider this an API breakage. We don't clearly state that the list is ordered, and I don't think ordering like this is generally guaranteed as part of our API

@LeonarddeR
Copy link
Collaborator

@CyrilleB79 I assume your use case is to get the dictionaries in the order they're defined?
You could also consider adding a keyword arg sorted: bool = True. If False, the function doesn't sort. This keeps current behavior but allows you to retrieve the unsorted list as well.

@CyrilleB79
Copy link
Collaborator Author

OK. Thanks for the feedback of you both.
So, I can actually use the private variable for now and ask a suitable public access function, or modification of the existing one, in NVDA 2025.2.

characterProcessing.listAvailableSymbolDictionaryDefinitions's implementation is not very useful for its usage in NVDA, i.e. it returns all dictionaries and sort them for GUI, but the GUI actually filters the list with only the visible ones.
but, since we can add another parameter or define a new function, we can think to it for 2025.2 without worrying of the API.

Converting to draft until I provide the correct request.

@CyrilleB79 CyrilleB79 marked this pull request as draft May 29, 2025 21:42
@LeonarddeR
Copy link
Collaborator

LeonarddeR commented May 30, 2025

I would modify the function for NVDA 2025.2. Just add a sortForPresentation = True param and do not sort when False.

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.

3 participants