SBU-BMI / SlicerPathology

3D Slicer extension for Pathology segmentation tools
http://bmi.stonybrookmedicine.edu
Other
5 stars 8 forks source link

BUG: Fix run time error due to uninitialized Editor module #40

Closed naucoin closed 8 years ago

naucoin commented 8 years ago

Check that the EditorWidget exists before trying to remove or add back shortcuts. slicer.modules.EditorWidget is only created when you enter the Editor modle.

Suggested-by: Steve Pieper pieper@bwh.harvard.edu

Issue #35

fedorov commented 8 years ago

Thanks Nicole!

I think the fix is incomplete though, since in effect (no pun intended) the effect will not be functional unless the user enters Editor prior to using SlicerPathology.

I think the right way to address the issue is to create Editor widget if it does not exist, the same way as it would be created when entering Editor for the first time. @pieper please comment if I did not understand it correctly.

naucoin commented 8 years ago

@fedorov I think this fixes the problem as reported, the access of the EditorWidget is only done to remove and add back shortcuts so that they don't interfere with the Pathology module's instantiation of an internal Editor. If the Editor module hasn't been entered, no need to remove shortcuts.

fedorov commented 8 years ago

Oh, ok - I was thinking that the code that adds shortcuts is not enabled, and the shortcuts would not work. You tested this and it works as expected, right?

naucoin commented 8 years ago

@fedorov I haven't tested it yet because I'm having problems building Slicer + SlicerOpenCV. I tested the code in the python console so it's syntactically correct at least. @ebremer can you verify my interpretation of the shortcut remove/re-add?

fedorov commented 8 years ago

@naucoin ok, note however that you should be able to test by simply replacing ShortCutEffect/ShortCutEffect.py in the extension install tree of the binary Slicer. Can you test using that approach and report the result here?

naucoin commented 8 years ago

The binary Slicer isn't starting due to the opencv logic issue.

fedorov commented 8 years ago

Interesting. Yesterday's nightly Slicer with SlicerPathology installed from ExtensionManager is starting without problems for me.

You might want to move out of the way .config/www.na-mic.org and try again.

naucoin commented 8 years ago

@fedorov: I misspoke: I meant that the quick tcga effect isn't working for me due to run time loading errors, Slicer will start up but I don't have full SlicerPathology functionality yet (the quick tcga effect fails at the point of: import vtkSlicerQuickTCGAModuleLogicPython ImportError: No module named vtkSlicerQuickTCGAModuleLogicPython) and the short cut effect with:

 File "/Users/nicole/Desktop/Slicer.app/Contents/Extensions-25043/SlicerPathology/lib/Slicer-4.5/qt-scripted-modules/ShortCutEffect.py", line 130, in create
    slicer.util.showStatusMessage(slicer.modules.TCGAEditorBot.logic.currentMessage)
AttributeError: 'TCGASegBot' object has no attribute 'logic'
fedorov commented 8 years ago

Did you try both effects buttons? The shortcut problem is separate from the issue you refer to, which was reported in https://github.com/SBU-BMI/SlicerPathology/issues/36

naucoin commented 8 years ago

Yes, I tried both effects buttons.

fedorov commented 8 years ago

Can you confirm you tried the second effect button after fresh restart of Slicer?

fedorov commented 8 years ago

Steps to reproduce problem that this patch is aiming to address, using Slicer binary install:

  1. Start Slicer
  2. Go to SlicerPathology
  3. Enter login etc, load data
  4. Hit the first button that has "G" and pencil
  5. Hit the big button without any text.
  6. Observe the python console

image

fedorov commented 8 years ago

After replacing the file in question with the patched version, the error changes to the one below, so I guess it might be considered as improvement in the context ...

image

I am going to merge this one, please investigate the missing import.

fedorov commented 8 years ago

it might be considered as improvement in the context

I take this back. There is no way to know whether this works or not until all of the issues are sorted out.

naucoin commented 8 years ago

@fedorov yes, I was working on the missing libraries and while waiting for clean builds to finish I did this quick fix.