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

Support qt toolkit name for Qt5 and Qt4 #1436

Merged
merged 2 commits into from
Apr 1, 2021

Conversation

rahulporuri
Copy link
Contributor

@rahulporuri rahulporuri commented Feb 19, 2021

This PR updates ETSConfig to support the use of qt as the toolkit name for Qt4 and Qt5 instead of only supporting qt4 as the toolkit name for Qt4. This is in line with similar changes we have made in other ETS packages e.g. traitsui, enable.

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference)
  • Update User manual (docs/source/traits_user_manual)
  • Update type annotation hints in traits-stubs

instead of only supporting qt4 as the toolkit name for PyQt4

	modified:   traits/etsconfig/etsconfig.py
	modified:   traits/etsconfig/tests/test_etsconfig.py
@@ -314,7 +314,7 @@ def _get_kiva_backend(self):
self._kiva_backend = (
"quartz" if sys.platform == "darwin" else "image"
)
elif self.toolkit == "qt4":
elif self.toolkit.startswith("qt"):
Copy link
Member

Choose a reason for hiding this comment

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

Could we stick to a whitelist of toolkits we allow? (Probably just "qt4" and "qt".) Otherwise we risk having to support everything starting with "qt" for evermore.

Copy link
Member

Choose a reason for hiding this comment

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

(Though I guess it's a moot point, because the fall-through branch will pick "image" anyway.)

@mdickinson
Copy link
Member

mdickinson commented Feb 19, 2021

Just to be clear, this PR doesn't change user-visible behaviour at all, right?

@rahulporuri rahulporuri added this to In progress in Enthought OSS Q1 2021 Feb 22, 2021
@mdickinson
Copy link
Member

@rahulporuri Ping! Could you rebut or confirm my assertion that this doesn't change user-visible behaviour in any way? If it doesn't, then it would be good to explain what the benefit of the PR is.

@rahulporuri
Copy link
Contributor Author

... explain what the benefit of the PR is.

The main intention here was to explicitly set the backend to image if the toolkit string starts with qt i.e. qt4 and qt.

Could you rebut or confirm my assertion that this doesn't change user-visible behaviour in any way?

My understanding is that yes, this PR does not change user-visible behavior in anyway. Previously, if ETS_TOOLKIT was set to qt, the else branch would have just image as the kiva backend. This PR just makes it explicit.

So, this PR doesn't fix any issues or make any behavioral changes - it just makes existing code more explicit.

I'm okay if we decide that this change isn't warranted and choose to close this PR instead of merging it.

instead explicitly check if the toolkit is qt4 or qt.
changes based on PR review comment.
	modified:   traits/etsconfig/etsconfig.py
@rahulporuri rahulporuri removed their assignment Mar 16, 2021
@mdickinson mdickinson merged commit 2c1a1bf into master Apr 1, 2021
Enthought OSS Q1 2021 automation moved this from In progress to Done Apr 1, 2021
@mdickinson mdickinson deleted the fix/support-qt-as-toolkit-name branch April 1, 2021 10:52
@rahulporuri rahulporuri moved this from Done to Sprint 3 : Feb 8 2021 - Feb 26 2021 in Enthought OSS Q1 2021 Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Enthought OSS Q1 2021
Sprint 3 : Feb 8 2021 - Feb 26 2021
Development

Successfully merging this pull request may close these issues.

None yet

2 participants