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.

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

Enhance WXPython4 compatibility in Default Profile Reversion dialog #52

Merged
merged 3 commits into from
Sep 2, 2018

Conversation

ivnc
Copy link
Collaborator

@ivnc ivnc commented Aug 31, 2018

In order to improve compatibility with WXPython 4, deprecated wx.NewId() has been changed to wx.ID_ANY on lambdaSetupProfile, as IDs are not relevant in this case.
In addition, the version in buildVars was updated from 1.2.1A to 1.2.2-dev.

If any problem please comment.

@ivnc ivnc requested a review from nvdaes August 31, 2018 22:15
Copy link
Collaborator

@nvdaes nvdaes left a comment

Choose a reason for hiding this comment

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

This ensures compatibility with wx Py 4. Anyway, this dialog could be improved by using guiHelper to enhance the visual appearance, according to NVDA 2016.4 and above.
Also, this could be documented in the readme.

@ivnc
Copy link
Collaborator Author

ivnc commented Sep 1, 2018

Now guiHelper is used. Please review it again.
What do you suggest to include in documentation? Changelog usually isn't updated until a stable release preparation.

Regards.

@nvdaes
Copy link
Collaborator

nvdaes commented Sep 2, 2018

I approved changes since comments are related just to style, not functionality:

  • Generally, sHelper instead of helper is used, but this is optional.
  • -1 argument can be removed in the static text control, and wx.ID_ANY is not needed for checkboxes using guiHelper. See the Document formatting dialog in NVDA for an example.
  • About documentation, I suggest something like:
  • Compatible with NVDA 2018.3 and later.

Thanks

@ivnc
Copy link
Collaborator Author

ivnc commented Sep 2, 2018

I implemented your suggestions, so I think we can merge now. Regarding the update of the changelog I think that we can do it when preparing 1.2.2 stable, and anyway directly to master.

Regards.

@ivnc ivnc merged commit 40aee86 into master Sep 2, 2018
@ivnc ivnc deleted the WX4 branch September 2, 2018 19:21
@ivnc
Copy link
Collaborator Author

ivnc commented Sep 2, 2018

By the way, thanks for your comments. I'm new to coding and any help is always appreciated.

Regards.

@nvdaes
Copy link
Collaborator

nvdaes commented Sep 2, 2018

OK, if you need access to addonFiles to update get.php let me know. Joseph and me are admins of addonFiles repo. Or I can modify get.php, but perhaps it's better that people can write to the repo to work faster and we can do other things too.

@ivnc
Copy link
Collaborator Author

ivnc commented Sep 2, 2018 via email

@nvdaes
Copy link
Collaborator

nvdaes commented Sep 2, 2018

Yes, I remember I gave access to you, but in case this was lost or something, since a person removed it and then access for people could suffer changes, and I'm not sure when this happened.

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.

2 participants