Added navigation features to the sagittal and centerline GUI #1545

peristeri · 2017-11-24T18:01:37Z

The user can now navigate using the arrow keys. Also, update the canvas dragging
the mouse along the canvas.

Implements #1501

The user can now nagivate using the arrow keys. Also, update the canvas dragging the mouse along the canvas. Implements #1501

jcohenadad

a few issues here:

* BUG: label_vertebrae: Updated label disc value (fixes #1570) * REF: fixed typo * OPT: label_vertebrae: Added instructions above image for sagittal dialog (fixes #1563)

peristeri · 2018-01-17T17:40:53Z

So in other words:

Navigating the sagittal plane: up/down traverses the axial plane and the left/right traverses the corrinal plane. And navigating the axial plane: up/down traverses the corrinal plane and left/right traverses the sagittal plane.

Is that correct?

jcohenadad · 2018-01-17T23:42:09Z

i would sum up points as follows:

vertebral labeling: in fact, we don't need the axial view. Partly because it is not clear where (SI) it is located (no indication on the sag plane). So only left and right keys could be used to go R and L.
propseg: I don't see the need for using the keys there. Unless someone thinks otherwise? @benjamindeleener?
add indications on the screen about what keys to use and for what

benjamindeleener · 2018-01-18T00:15:16Z

@jcohenadad @peristeri I agree for PropSeg. No need for keys control.

perone

Nice ! I added some comments. I just missed function and classes docstrings documentation.

perone · 2018-03-23T18:49:53Z

spinalcordtoolbox/gui/base.py

        self.setWindowTitle(self.params.dialog_title)

+    def increment_vertical_nav(self):
+        pass


I think it is better to raise a NotImplementedError here and on the other abstract methods.

perone · 2018-03-23T18:50:07Z

spinalcordtoolbox/gui/base.py

+        pass
+
+    def decrement_vertical_nav(self):
+        pass


perone · 2018-03-23T18:50:14Z

spinalcordtoolbox/gui/base.py

+        pass
+
+    def increment_horizontal_nav(self):
+        pass


perone · 2018-03-23T18:50:18Z

spinalcordtoolbox/gui/base.py

+        pass
+
+    def decrement_horizontal_nav(self):
+        pass


perone · 2018-03-23T18:54:26Z

spinalcordtoolbox/gui/base.py

@@ -114,8 +114,26 @@ def _init_ui(self):
        QtGui.QShortcut(QtGui.QKeySequence.Save, self, self.on_save_quit)
        QtGui.QShortcut(QtGui.QKeySequence.Quit, self, self.close)

+        QtGui.QShortcut(QtGui.QKeySequence.MoveToNextChar, self, self.increment_vertical_nav)


There are a lot of shortcuts being added here, maybe it's worth creating mapping list like (keysequence, func) and then iterating over it on a loop to add the shortcuts.

perone · 2018-03-23T19:02:05Z

spinalcordtoolbox/gui/base.py

        self.setWindowTitle(self.params.dialog_title)

+    def increment_vertical_nav(self):


Functions without documentation.

perone · 2018-03-23T19:03:59Z

spinalcordtoolbox/gui/centerline.py

@@ -138,15 +151,15 @@ def _init_controls(self, parent):
        custom_mode.setToolTip('Manually select the axis slice on sagittal plane')
        custom_mode.toggled.connect(self.on_toggle_mode)
        custom_mode.mode = 'CUSTOM'
-        custom_mode.sagittal_title = 'Select an axial slice'
+        custom_mode.sagittal_title = 'Select an axial slice.\n%s' % self.params.subtitle


Consider using the new style string formatting, like the one below where you use the .format().

perone · 2018-03-23T19:15:54Z

tests/test_gui.py

-        expected = [(15, 45, 33, 1), (30, 51, 35, 1), (60, 71, 31, 1)]
-        points = [(x + controller.INTERVAL, y, z) for x, y, z, _ in expected]
+        controller = self._init_auto()
+        expected = [(0, 45, 33, 1), (160, 51, 35, 1), (319, 71, 31, 1)]


Another reason to have namedtuple or something similar here, what is the fourth element on each of these tuples ?

perone · 2018-03-23T19:19:16Z

spinalcordtoolbox/gui/widgets.py

-        for i in self.annotations:
-            i.remove()
-        self.annotations = []
+        if hasattr(self, 'annotations'):


I think it would be better to always have this attribute here. And then you keep it as an empty list when there are no annotations. Then you can do something like:

for i in self.annotations: i.remove() self.annotations.clear()

without having to do the check using hasattr().

perone · 2018-03-23T19:23:12Z

spinalcordtoolbox/gui/widgets.py

+        if self._plot_points:
+            controller = self._parent._controller
+            logger.debug('Plotting points {}'.format(controller.points))
+            points = [x for x in controller.points if x[0] == controller.position[0]]


I think it's better to add some comments on what is happening here, why the except ValueError below is required, etc.

…the code)

…eg because it was previously too large for mode init-mask

…s wrongly calling nz param, which is orientation dependent)

Added navigation features to the sagittal and centerline GUI

aaef7c7

The user can now nagivate using the arrow keys. Also, update the canvas dragging the mouse along the canvas. Implements #1501

peristeri requested review from jcohenadad, benjamindeleener and charleygros November 24, 2017 18:01

Merge branch 'master' into qt-navigation

4148d34

jcohenadad requested changes Jan 15, 2018

View reviewed changes

Bug fix and various improvements (#1571)

4b14a2d

* BUG: label_vertebrae: Updated label disc value (fixes #1570) * REF: fixed typo * OPT: label_vertebrae: Added instructions above image for sagittal dialog (fixes #1563)

peristeri added 2 commits March 22, 2018 15:24

Merge branch 'master' into qt-navigation

1f96dbb

Implemented the changes suggested in pull request comments.

ff8d0be

peristeri added enhancement category: improves performance/results of an existing feature fix:minor labels Mar 23, 2018

peristeri self-assigned this Mar 23, 2018

perone suggested changes Mar 23, 2018

View reviewed changes

peristeri and others added 3 commits March 23, 2018 23:02

Merge branch 'master' into qt-navigation

7ef391e

Merge branch 'master' into qt-navigation

c5e8db2

Implemented the suggestions in the pull request

fd45ddf

perone previously approved these changes Mar 26, 2018

View reviewed changes

jcohenadad added 8 commits March 27, 2018 11:16

Merge branch 'master' into qt-navigation

b0d582b

Fixed few typos

b767e68

removed useless lines (because those variables are assigned later in …

40a4639

…the code)

Fixed: in centerline-custom mode, reset position should start at the top

2e6014b

clarified code

aab9efc

increased number of max points for centerline (previously 11)

0c3d47f

added interval in base config and set absolute value within sct_props…

8d5f6a4

…eg because it was previously too large for mode init-mask

cleaned up debug code

439dee6

jcohenadad dismissed perone’s stale review via 439dee6 March 27, 2018 04:00

jcohenadad added 7 commits March 27, 2018 13:24

fixed typo

e8060f1

adjust aspect ratio based on pixel dimension

d9704a4

changed parameter name for interval_in_mm to avoid confusion

75e3d26

added todo

594c7d6

updated misleading variable name

c0e940c

Fixed identification of axial slice when input is not RPI (propseg wa…

7fca38e

…s wrongly calling nz param, which is orientation dependent)

added checks if initial slice is out of range or is a fraction

fe1a1ff

jcohenadad approved these changes Mar 27, 2018

View reviewed changes

jcohenadad merged commit edd2a08 into master Mar 27, 2018

jcohenadad deleted the qt-navigation branch March 27, 2018 10:57

peristeri added sct_viewer context: and removed card:TO_REVIEW labels Mar 27, 2018

This was referenced Apr 24, 2018

Modifications were overriden by PR #1690

Closed

Concatenate string to keep previously assigned subtitle field #1692

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added navigation features to the sagittal and centerline GUI #1545

Added navigation features to the sagittal and centerline GUI #1545

peristeri commented Nov 24, 2017

jcohenadad left a comment

peristeri commented Jan 17, 2018

jcohenadad commented Jan 17, 2018

benjamindeleener commented Jan 18, 2018

perone left a comment

perone Mar 23, 2018

perone Mar 23, 2018

perone Mar 23, 2018

perone Mar 23, 2018

perone Mar 23, 2018

perone Mar 23, 2018

perone Mar 23, 2018

perone Mar 23, 2018

perone Mar 23, 2018

perone Mar 23, 2018

		self.setWindowTitle(self.params.dialog_title)

		def increment_vertical_nav(self):

Added navigation features to the sagittal and centerline GUI #1545

Added navigation features to the sagittal and centerline GUI #1545

Conversation

peristeri commented Nov 24, 2017

jcohenadad left a comment

Choose a reason for hiding this comment

peristeri commented Jan 17, 2018

jcohenadad commented Jan 17, 2018

benjamindeleener commented Jan 18, 2018

perone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment