Skip to content

Force DictionaryDialog to be subclassed to use it#13268

Merged
seanbudd merged 4 commits into
nvaccess:masterfrom
CyrilleB79:forceDicSubclassing
Jan 25, 2022
Merged

Force DictionaryDialog to be subclassed to use it#13268
seanbudd merged 4 commits into
nvaccess:masterfrom
CyrilleB79:forceDicSubclassing

Conversation

@CyrilleB79

@CyrilleB79 CyrilleB79 commented Jan 21, 2022 •
edited by seanbudd
Loading

Copy link
Copy Markdown
Contributor

Link to issue number:

Follow-up of #12967

Summary of the issue:

Since the merge of #12800, trying to re-open an already opened dialog allows to focus it rather than issuing an error message box. The dialog is refocused according to its class, i.e. trying to open a dialog of the same class rather refocus the first already opened dialog of the same class.

But with #12800, it was not possible to open 2 dictionaries of different type (e.g. speech and default). This was fixed with #12967 by subclassing DictionaryDialog.
In #12967, however, the following known issue was mentionned:

Maybe it would be interesting to forbid to instance an object of class DictionaryDialog directly, i.e. force subclassing.

Indeed, if DictionaryDialog's have been subclassed in NVDA's core, add-on may still instance directly a DictionaryDialog. That should be forbidden as explained in #12967.

However, it could not be done before 2022.1 due to add-ons that may break if they instance directly DictionaryDialog. That's the case of applicationDictionary.

Note: emoticons and EnhancedDictionaries add-ons use DictionaryDialog's but they already subclass it.

Description of how this pull request fixes the issue:

Option 1 (withdrawn after review):
Raise an error in the __init__ method of DictionaryDialog if it is instancied from the same class (not a subclass).

Option 2 (implemented after review):
I have abstracted DictionaryDialog as advised in #12967 (comment) and thanks to @lukaszgo1's review.

Testing strategy:

Manual testing:

  1. Created aglobal plugin (dictTest.py.txt) allowing to open two dictionaries:

    • One directly created from DictionaryDialog
    • One created from a subclass of DictionaryDialog
      And tested that:
    • both dictionaries open with NVDA 2021.3.1
    • Only the subclassed dictionary opens with this PR's code (from source)

  2. Tested that NVDA's default, voice and temp dictionaries dialogs still open (from menu).

Known issues with pull request:

Should be merged in 2022.1 or any API-breaking release.

Change log entries:

For Developers

It is now mandatory to subclass DictionaryDialog to use it. (#13268)

Code Review Checklist:

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

lukaszgo1
lukaszgo1 reviewed Jan 22, 2022
View reviewed changes
Comment thread source/gui/settingsDialogs.py
@AppVeyorBot

AppVeyorBot commented Jan 22, 2022

Copy link
Copy Markdown

See test results for failed build of commit 8fdce76274

@CyrilleB79 CyrilleB79 marked this pull request as ready for review January 22, 2022 23:49
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner January 22, 2022 23:49
@CyrilleB79

CyrilleB79 commented Jan 22, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@seanbudd, as the reviewer of #12967 and the author of #12800, you may be interested in reviewing this one.

@seanbudd

seanbudd commented Jan 24, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Note: I've unchecked "API is compatible with existing add-ons", please leave this unmarked as this is a documented breaking change

@seanbudd seanbudd added this to the 2022.1 milestone Jan 24, 2022
@CyrilleB79

CyrilleB79 commented Jan 24, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for review @seanbudd .

Discussing on process details, you wrote:

Note: I've unchecked "API is compatible with existing add-ons", please leave this unmarked as this is a documented breaking change

Sorry, that's not the way I was told to use these checkboxes (by Reef in another PR).

Moreover if I read githubPullRequestTemplateExplanationAndExamples.md:

The aim of this checklist is to ensure each item has been considered by both the author and the
reviewer.
and
Not all items will be applicable for all situations, in this case checking the item lets reviewers
know it's been considered.

Since I have considered the API-breaking change topic, I think I should check the checkbox to indicate that it has been considered.

And in the section API is compatible with existing add-ons:

Ensure proposed API changes are included in the change log (Changes for Developers).
This let me think I should check the checkbox API is compatible with existing add-ons, since I have actually provided the required change log item.

Just to clarify how I have used these checkbox and to highlight that there may be a misunderstanding about their use.

@seanbudd

seanbudd commented Jan 24, 2022

Copy link
Copy Markdown
Member

@CyrilleB79 - apologies, my mistake. Perhaps we should improve the wording of the checklist item, it is somewhat misleading for it to be checked in this instance.

@seanbudd seanbudd merged commit 7735d96 into nvaccess:master Jan 25, 2022
@CyrilleB79

CyrilleB79 commented Jan 25, 2022

Copy link
Copy Markdown
Contributor Author

In the PULL_REQUEST_TEMPLATE.md which is the content of the edit field when you open a pull-request, for the Code Review Checklist paragraph, you have the following comment:

This seem quite clear to me, as a PR author/submitter. But of course, this paragraph is not visible for the reviewer who reads the PR.

Do you have an idea on how this can be made clearer for reviewer? What would you expect as a reviewer? Maybe a sentence out of the comment telling "Author should check each checkbox to indicate that the relevance of the item has been considered; reviewer should ensure that all checkbox are checked." I fear however that this wording makes the template to heavy.
What do you think?

@seanbudd

seanbudd commented Jan 26, 2022

Copy link
Copy Markdown
Member

@CyrilleB79

I was misled after coming back from break and forgetting the context we added to the template. What I find misleading is "API is compatible with existing add-ons." is not a true statement if there is API breaking changes.

Perhaps it could be "API is compatible with existing add-ons or breaking changes have been documented" or the shorter "API compatibility has been considered"

@CyrilleB79 CyrilleB79 deleted the forceDicSubclassing branch February 15, 2022 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

@michaelDCurran michaelDCurran Awaiting requested review from michaelDCurran michaelDCurran is a code owner automatically assigned from nvaccess/developers

+1 more reviewer

@lukaszgo1 lukaszgo1 lukaszgo1 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2022.1

Development

Successfully merging this pull request may close these issues.

4 participants

@CyrilleB79 @AppVeyorBot @seanbudd @lukaszgo1