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

[mask] minor mask improvements #836

Merged
merged 6 commits into from
Jun 12, 2017
Merged

[mask] minor mask improvements #836

merged 6 commits into from
Jun 12, 2017

Conversation

PiRK
Copy link
Member

@PiRK PiRK commented May 22, 2017

Adds resetSelectionMask to dock widget API.

Fixes also a few minor sphinx warnings.

EDIT: Initially this PR was meant to close #834, but this will need a bit more work than expected.

…one)"

This reverts commit 0ba002a.
Causes unexpected side effects when comparing mask to previous mask in history
@PiRK
Copy link
Member Author

PiRK commented May 22, 2017

Fixing #834 seems to cause unexpected side effects when the empty mask is saved to the mask history stack, so I reverted the initial commit and I'll fix the issue directly in PyMca by avoiding calling setSelectionMask with an empty array.

This PR is now just a minor docstring improvement and exposes resetSelectionStack in the dock widget.

@PiRK PiRK changed the title [mask] make setSelectionMask accept empty list/array and None [mask] minor mask improvements May 22, 2017
@PiRK PiRK mentioned this pull request May 23, 2017
2 tasks
This avoids having to call the parent constructor at very specific stages after the child has defined instance attributes.
@PiRK
Copy link
Member Author

PiRK commented May 24, 2017

Would it be OK to set self._z = self.plot.getActiveImage().getZLayer() + 1 in MaskToolsWidget?

I have a case for which my active image is on z=1, and when it is changed while the mask widget is hidden, the image is redrawn on top of the mask, which is then invisible (vasole/pymca#58 (comment))

@vasole
Copy link
Member

vasole commented May 24, 2017

@t20100

It would be better than to have something hard coded to 1. I would say proposed solution should be the default for the case the application has not explicitly set it.

In any case, an application should be able to specify at what level/layer wants the mask.

@t20100
Copy link
Member

t20100 commented May 29, 2017

+1 for what @PiRK proposed and optionally to set the Z value of the mask.

@PiRK
Copy link
Member Author

PiRK commented May 29, 2017

After looking at the code, tt seems self._z = activeImage.getZValue() + 1 is already implemented in the _activeImageChanged and _activeImageChangedAfterCare slots.

The issue only exists when the mask is set while the mask widget has never been set visible, so neither one of these slots is connected, and the Z layer is set to its init value (1). I will assess whether it is possible to connect _activeImageChangedAfterCare already in __init__ .

@PiRK
Copy link
Member Author

PiRK commented May 29, 2017

Actually it would not help to have the slot connected in init, because _activeImageChangedAfterCare would automatically disconnect itself as soon as the active image is set with a different shape as the current mask (initialized as a (0, 0) shape array).

The broader question is whether we want to change the mask widget to have the same behavior whether is is hidden or shown, or keep it as it is. As it is, it's designed to fully work and accept image changes only when visible. When hidden, it only shows the mask as long as the shape of the active image does not change, else it removes the mask and disconnects itself.

@t20100
Copy link
Member

t20100 commented May 30, 2017

The mask widget was only designed to work with an active image.
It's already be extended to scatter plots and is not ready for synchronization with other mask widgets...
It's probably time to consider a refactoring.

@vasole
Copy link
Member

vasole commented Jun 9, 2017

The synchronization can be achieved by the application as being shown with the work on PyMca.

Can this PR be merged?

@t20100 t20100 merged commit 3536557 into silx-kit:master Jun 12, 2017
@PiRK PiRK deleted the Mask2 branch May 25, 2018 07:18
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.

[mask] setSelectionMask can't process return value of getSelectionMask()
3 participants