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

Issue 691 interact mac broken #905

Merged
merged 12 commits into from Nov 20, 2014

Conversation

Projects
None yet
3 participants
@doutriaux1
Member

doutriaux1 commented Nov 18, 2014

@dlonie @aashish24 please review and feel free to suggest better/cleaner solutions.
@ThomasMaxwell wouldn't mind you looking this over as well and testing it against DV3D.

doutriaux1 added some commits Sep 22, 2014

@doutriaux1 doutriaux1 added this to the 2.1 milestone Nov 18, 2014

@ThomasMaxwell

This comment has been minimized.

Contributor

ThomasMaxwell commented Nov 19, 2014

Did a brief interaction/resize test- don't see any problems for the 3D plots.

From: Charles Doutriaux <notifications@github.commailto:notifications@github.com>
Reply-To: UV-CDAT/uvcdat <reply@reply.github.commailto:reply@reply.github.com>
Date: Tuesday, November 18, 2014 6:20 PM
To: UV-CDAT/uvcdat <uvcdat@noreply.github.commailto:uvcdat@noreply.github.com>
Cc: "Maxwell, Thomas P. (GSFC-606.2)[SCIENCE APPLICATIONS INTL CORP]" <thomas.maxwell@nasa.govmailto:thomas.maxwell@nasa.gov>
Subject: [uvcdat] Issue 691 interact mac broken (#905)

@dloniehttps://github.com/dlonie @aashish24https://github.com/aashish24 please review and feel free to suggest better/cleaner solutions.
@ThomasMaxwellhttps://github.com/ThomasMaxwell wouldn't mind you looking this over as well and testing it against DV3D.


You can merge this Pull Request by running

git pull https://github.com/UV-CDAT/uvcdat issue_691_interact_mac_broken

Or view, comment on, or merge it at:

#905

Commit Summary

  • put changes back in
  • merge master in this
  • added AnyEvent to see what is happening (not much on mac)
  • ok this works, needs clean up
  • ok not quite there... no left click segfaults
  • ok works on Mac
  • Merge branch 'master' into issue_691_interact_mac_broken
  • rendering the clickrenderer seg fault on mac if clicked before and resize, commenting out fixes it
  • works on Linux now

File Changes

  • M Packages/vcs/Lib/VTKPlots.pyhttps://github.com//pull/905/files#diff-0 (47)

Patch Links:


Reply to this email directly or view it on GitHubhttps://github.com//pull/905.

if sys.platform == "darwin":
self.AddObserver( "RenderEvent", parent.renderEvent )
#self.AddObserver( "AnyEvent",parent.stdEvent)

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

Just as a general style note, committed commented out code is rarely necessary. If you've removed something, the old implementation is still stored in git, and if it's new code, why add it if it isn't used? Otherwise it just clutters the codebase and gets in the way of readability.

If there is a reason to leave a commented code path in, a comment explaining why is a good idea.

This comment has been minimized.

@doutriaux1

doutriaux1 Nov 20, 2014

Member

nope. It's just that i'm lazy and didn't want to go through git to uncoment it in case we need further development on this issue 😉

@@ -42,11 +45,15 @@ def __init__(self,canvas,renWin=None, debug=False,bg=None):
self.renderer = None
self._plot_keywords = ['renderer',]
self.numberOfPlotCalls = 0
self.numberOfPlotCalls = 0

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

This line isn't needed, it's the same instruction as the line before it.

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

ping!

if renWin is not None:
self.renWin = renWin
if renWin.GetInteractor() is None and self.bg is False:
self.createDefaultInteractor()
self.logo = None
if sys.platform == "darwin":
self.reDO = False

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

What is reDO? Avoid abbreviations when possible, it makes it much easier to read, review, and maintain collaborative code when variables/methods are clearly named.

This comment has been minimized.

@doutriaux1

doutriaux1 Nov 20, 2014

Member

reDO is for DO it again (like undo/redo)

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

But what is being redone? Just seeing "redo" in a complex codebase doesn't really give another developer any idea what the variable is for -- always check through patches for clarity like you're reading the code for the first time -- everyone else will have to ;)

This comment has been minimized.

@doutriaux1

doutriaux1 Nov 20, 2014

Member

ok will rename to reRender, ok?

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

Sounds good. A brief comment explaining why it is needed will help future maintainers understand the purpose, too.

if interactor is None:
warnings.warn("Cannot start interaction. Blank plot?")
return
warnings.warn("Press 'Q' to exit interactive mode and continue script execution")
interactor.Start()
def stdEvent(self,caller,evt):
print evt

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

I don't think this debugging statement should go into the master branch.

This comment has been minimized.

@doutriaux1

doutriaux1 Nov 20, 2014

Member

The whole func can be taken out. Again I left it in there in case we need to enable AnyEvent at some point. Will take it out.

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

This too.

def renderEvent(self,caller,evt):
renwin = self.renWin if (caller == None) else caller
window_size = renwin.GetSize()
#print "Yes we are herE",window_size,self.renderWindowSize

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

This should be taken out before we merge.

This comment has been minimized.

@doutriaux1

doutriaux1 Nov 20, 2014

Member

The commented out line?

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

Yep, just the commented out debug line.

def leftButtonPressEvent(self,obj,event):
#print "We do come here"

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

Same, temporary debugging statements shouldn't be in master, even when commented out.

This comment has been minimized.

@doutriaux1

doutriaux1 Nov 20, 2014

Member

will do. Thanks for forcing me to "do the right thing"

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

No problem :) Thanks for being receptive to my suggestions. But always feel free to push back if you don't agree!

This comment has been minimized.

@doutriaux1

doutriaux1 Nov 20, 2014

Member

oh don't worry about that 😜

self.clickRenderer.Render()
# The following seg fault on Mac if clicking before resizing the window
# commenting out
#self.clickRenderer.Render()

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

If it isn't needed on any platform, why not remove it? If it is needed on other platforms, check sys.platform execute it conditionally.

This comment has been minimized.

@doutriaux1

doutriaux1 Nov 20, 2014

Member

yes I retested on Linux later and it still works will take it out.

return
#print "configuring"

This comment has been minimized.

@allisonvacanti

allisonvacanti Nov 20, 2014

Contributor

debugging statement, should be taken out.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Nov 20, 2014

@dlonie please make sure I got them all now.

@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented Nov 20, 2014

Almost -- check the new diff.

@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented Nov 20, 2014

Looks good to me! Thanks for fixing those up.

There were some conflicts when I went to merge master in. Can you address those before we merge?

@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented Nov 20, 2014

Wait a sec, I think I had an older version checked out....

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Nov 20, 2014

github says it canmerge w/o conflict

@allisonvacanti

This comment has been minimized.

Contributor

allisonvacanti commented Nov 20, 2014

Yeah, my local refs were out of date. Rebuilt and running tests, all passing so far 👍

allisonvacanti added a commit that referenced this pull request Nov 20, 2014

@allisonvacanti allisonvacanti merged commit d426cf4 into master Nov 20, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

@doutriaux1 doutriaux1 deleted the issue_691_interact_mac_broken branch Mar 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment