Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update poedit appmodule for version 3.4+ #15313

Merged
merged 18 commits into from Oct 31, 2023
Merged

Update poedit appmodule for version 3.4+ #15313

merged 18 commits into from Oct 31, 2023

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Aug 19, 2023 •
edited

Link to issue number:

Fixes #7303
Fixes #4371

Summary of the issue:

NVDA has specific functionality for Poedit, but this is only supported in version 1.

Description of user facing changes

Add support for Poedit 3.4 while dropping support for Poedit 1. Compared to the previous Poedit 1 support, support has been added to report the old source text and any translation warning. There is also an audio indication when the needs work checkbox is enabled (fuzzy messages).

Description of development approach

Created a revamped module for Poedit 3.

Testing strategy:

Tested reporting of:

  • Translation notes
  • Comments
  • Warnings
  • Old source text (for fuzzy messages)
  • Needs work check box (Poedit 3.4+)

I tested with Poedit 3.0, 3.3.2 and 3.4 to ensure the logic to calculate window control ids would be stable across releases.

Known issues with pull request:

This drops support for Poedit 1, but as Poedit 3 is perfectly accessible, I think we shouldn't bother. It might be nice to encourage translators to update their Poedit

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
  • Security precautions taken.

@LeonarddeR LeonarddeR requested review from a team as code owners August 19, 2023 13:44
@AppVeyorBot
Copy link

See test results for failed build of commit f5c2cba967

@AppVeyorBot
Copy link

See test results for failed build of commit 9b26ce53f7

@gregjozk
Copy link
Contributor

Hello,

thanks for much needed upgrade.
just a question, will upgraded modul incorporated features from Poedit addons (one from portugal and one from Nepal) - excuse me I don't know names of all contributers for mensioned addons... See: https://groups.io/g/nvda-translations/topic/new_poedit_add_on/90806613?p=,,,100,0,0,0::recentpostdate/sticky,,,100,2,0,90806613,previd%3D1692558197340101298,nextid%3D1659276346104250870&previd=1692558197340101298&nextid=1659276346104250870

Thanks again.

regards
Jožef

@LeonarddeR
Copy link
Collaborator Author

It's a pity this add-on was never contributed to core. I'm open for suggestions to enhance this, though. I probably need to look into the singular and plural translations first.

@LeonarddeR LeonarddeR marked this pull request as draft August 21, 2023 06:17
@LeonarddeR
Copy link
Collaborator Author

cc @ruifontes

@DraganRatkovich
Copy link

Hello,
@LeonarddeR Many thanks for the much needed update. Now that I have checked the build with the latest version of Poedit, I can easily read any translator note, however, without the Poedit add-on, I cannot read any of the errors presented in the translation.
When I enable the Poedit add-on and press ctrl+shift+e it reads all the errors if there are any, however in this case I can't read the notes for the translators.
Is it possible to fix these conflicts because it would be a huge step in the usability of Poedit as translator notes are a really important feature in translatable strings and because of this feature I think going back to the old version is really useless.

@LeonarddeR Maybe you can contribute to the development of the add-on and make it possible to properly obtain IDs, because as far as I know, the free version has different IDs (or whatever they are called) compared to the Pro version.

@LeonarddeR
Copy link
Collaborator Author

@DraganRatkovich Are you using the fre or the pro version? I have no access to the pro version so I can't test with that.

@DraganRatkovich
Copy link

@LeonarddeR I am using the free version.

@zstanecic
Copy link
Contributor

zstanecic commented Aug 22, 2023 via email

@LeonarddeR
Copy link
Collaborator Author

@zstanecic How does it behave for you?

@Adriani90
Copy link
Collaborator

Will the new updated app module handle #4371 as well?

@LeonarddeR
Copy link
Collaborator Author

Good catch, thanks @Adriani90

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Aug 24, 2023 •
edited

@gregjozk and @DraganRatkovich
I had a brief look into the add-on. I'm inclined not to change anything in the current pull request for now.

  1. Errors (or translation warnings) can be accessed by pressing ctrl+shift+w
  2. There is some logic in the add-on to report additional errors (e.g. missmatch in string replacement parameters), but IMO that's a responsibility of poedit to report as a translation warning, if it not already does so.
  3. There is also logic to clear translations or copy source/translated text to the clipboard. This is also out of scope for a rewrite of the add-on.
  4. Some things in the add-on seem to be buggy, such as reporting comments and fuzzy status. This module also reports fuzzy status and ctrl+shift+o allows you to report the previous text.

I'd suggest @ruifontes to update the add-on after this pr is merged into an official release, basing the appModule with the additions on the module in NVDA core.

@LeonarddeR LeonarddeR marked this pull request as ready for review August 24, 2023 16:28
@gregjozk
Copy link
Contributor

gregjozk commented Aug 24, 2023 via email

seanbudd
seanbudd reviewed Aug 25, 2023
View reviewed changes
source/appModules/poedit.py Outdated Show resolved Hide resolved
source/appModules/poedit.py Show resolved Hide resolved
source/appModules/poedit.py Outdated Show resolved Hide resolved
source/appModules/poedit.py Outdated
obj = None
elif obj:
obj = obj.firstChild
def _getNVDAObjectForWindowControlIdOffset(self, windowControlIdOffset: WindowControlIdOffset):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this get a return type added please

user_docs/en/userGuide.t2t Outdated Show resolved Hide resolved
source/appModules/poedit.py Show resolved Hide resolved
source/appModules/poedit.py Show resolved Hide resolved
LeonarddeR and others added 3 commits August 25, 2023 08:43
@LeonarddeR
Copy link
Collaborator Author

I've had another round of testing with the app module and it seems that fuzzy messages aren't yet covered correctly. I have no idea how to fix this though. It seems like the only thing happening is that the icon displayed on the needs work button changes, as well as the text colors change to orange, but these are colors we couldn't detect.

@LeonarddeR LeonarddeR mentioned this pull request Aug 25, 2023
Open
@ruifontes
Copy link
Contributor

@LeonarddeR you can check if an entry is classified as "Fuzzy" by checking the 3 new fields that are shown.
They have an ID with a value of more 59, 60 and 61 in relation to the entries list ID...

@CyrilleB79
Copy link
Collaborator

In response to @DraganRatkovich's concern:

Perhaps in this case @ruifontes can do something with the add-on so they can both work without interfering with each other, not sure though.

Yes, it's possible:
@ruifontes, just import the built-in appModule on top of your add-on's appModule:
from nvdaBuiltin.appModules.poedit import * # noqa: F401, F403
And then write your module extending the capabilities of the original appModule. See Outlook Extended for an example.
But probably you already know this pattern.

@ruifontes
Copy link
Contributor

@LeonarddeR , I found one thing that can change the game...
The controlID difference between the paid and free version is 1, meaning by instance, that controlID for translated text in free version is -31901 then in paid version will be -31902.
So, checking the difference between the first sub window and translation list IDs, we can determine if it is a paid or free version.
Regarding your comment:

  • how was it working before this PR?
  • The only controlID equals in free and paid version is the first sub window...
    All controls we need to check the ID are afected by free or paid version.

@ruifontes
Copy link
Contributor

@cyrille, thanks!
I did knew about that, but is always good to refresh the memory, and I an not even close so knowledgeable as you and others about NVDA and Python...
Waiting this PR is closed to work on that...

@LeonarddeR
Copy link
Collaborator Author 

* The only controlID equals in free and paid version is the first sub window...
  All controls we need to check the ID are afected by free or paid version.

So are you saying that if we take the translations list control id as a reference, that will work across the paid and free versions?

@ruifontes
Copy link
Contributor

Yes, all controls, except the one you have choosen, change across free and paid version...

@LeonarddeR
Copy link
Collaborator Author

Let's hope the new build that is currently underway also works for the pro version.

@AppVeyorBot
Copy link

  • FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
  • PASS: Unit tests.
  • PASS: Lint check.
  • FAIL: System tests (tags: installer NVDA). See test results for more information.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/mkvcelxqtfp4o8r8/artifacts/output/nvda_snapshot_pr15313-29543,566b9817.exe
  • CI timing (mins):
    INIT 0.0,
    INSTALL_START 0.9,
    INSTALL_END 1.0,
    BUILD_START 0.0,
    BUILD_END 24.6,
    TESTSETUP_START 0.0,
    TESTSETUP_END 0.3,
    TEST_START 0.0,
    TEST_END 5.1,
    FINISH_END 0.1

See test results for failed build of commit 566b981725

@AppVeyorBot
Copy link

See test results for failed build of commit fc55bd7847

@LeonarddeR LeonarddeR marked this pull request as ready for review October 26, 2023 11:49
@AppVeyorBot
Copy link

See test results for failed build of commit db86bf0a3a

seanbudd
seanbudd reviewed Oct 26, 2023
View reviewed changes
source/appModules/poedit.py Show resolved Hide resolved
source/appModules/poedit.py Outdated Show resolved Hide resolved
source/appModules/poedit.py Outdated Show resolved Hide resolved
source/appModules/poedit.py Outdated Show resolved Hide resolved
source/appModules/poedit.py Outdated Show resolved Hide resolved
source/appModules/poedit.py Outdated Show resolved Hide resolved
source/appModules/poedit.py Outdated Show resolved Hide resolved
@seanbudd
Copy link
Member

Could this get a changelog entry please

LeonarddeR and others added 2 commits October 27, 2023 07:55
@LeonarddeR
Update changelog
a6ef021
@LeonarddeR @seanbudd
Apply suggestions from code review
ae44f4b 
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
seanbudd
seanbudd approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @LeonarddeR

@AppVeyorBot
Copy link

See test results for failed build of commit cb098a6673

Qchristensen
Qchristensen approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Userguide looks good.

@seanbudd
Copy link
Member

Just needs a lint fix and then this can be merged

@seanbudd seanbudd merged commit 8b90ece into nvaccess:master Oct 31, 2023
1 check passed
@LeonarddeR LeonarddeR mentioned this pull request Apr 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukaszgo1 lukaszgo1 lukaszgo1 left review comments

@CyrilleB79 CyrilleB79 CyrilleB79 left review comments

@seanbudd seanbudd seanbudd approved these changes

@Qchristensen Qchristensen Qchristensen approved these changes

Assignees
No one assigned
Labels
api-breaking-change conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Milestone
2024.1