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

Undo removal of get_mac_context import #586

Merged
merged 1 commit into from
Feb 15, 2021
Merged

Undo removal of get_mac_context import #586

merged 1 commit into from
Feb 15, 2021

Conversation

jwiggins
Copy link
Member

I removed this import in error as part of #558. It's needed by the Qt Quartz backend (enable.qt4.quartz).

@rahulporuri, @aaronayres35: This should be cherry-picked into the 5.0.0 branch.

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

Changes LGTM but i'm not entirely sure why we can't just update the import to use kiva.quartz.mac_context in enable. I guess the import isnt just used in enable but in other places as well e.g. in user code?

Also, looking at enable.qt4.quartz, the import is from kiva.quartz import get_mac_context, ABCGI and im not entirely sure where ABCGI comes from?

@jwiggins
Copy link
Member Author

ABCGI is a module in the kiva.quartz package, so that's how that import works.

I also briefly thought about changing the import in enable.qt4.quartz, but this fix restores behavior that might be depended on by external code, so I think it's best this way.

@jwiggins jwiggins merged commit 552d03a into master Feb 15, 2021
@jwiggins jwiggins deleted the fix/qt-quartz branch February 15, 2021 11:36
jwiggins added a commit that referenced this pull request Feb 15, 2021
@rahulporuri
Copy link
Contributor

ABCGI is a module in the kiva.quartz package, so that's how that import works.

Ahh. Makes sense.

... but this fix restores behavior that might be depended on by external code, so I think it's best this way.

Yeah. I agree.

jwiggins added a commit that referenced this pull request Feb 15, 2021
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