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

SegmentEditor used too much space after returning to None effect #942

Closed
wants to merge 1 commit into from
Closed

SegmentEditor used too much space after returning to None effect #942

wants to merge 1 commit into from

Conversation

che85
Copy link

@che85 che85 commented Apr 27, 2018

Before changes

Paint Effect None
before01 before02

After changes

Paint Effect None
after01 after02

@cpinter
Copy link
Member

cpinter commented Apr 27, 2018

Thanks, Christian! The screenshots and the code looks good to me. Please give me a bit of time to build and test this on my computer though, as the layout in Segment Editor has always been very tricky to keep from breaking, and I want to stress-test it before integration.

@cpinter
Copy link
Member

cpinter commented Apr 27, 2018

One comment is that please squash the two commits together, and keep the name of the first commit, with the change that it's an ENH instead of a STYLE (which means coding style)

@fedorov
Copy link
Member

fedorov commented Apr 27, 2018

I am glad you figured it out Christian!

fyi: @chribaue

@che85
Copy link
Author

che85 commented Apr 27, 2018

Sure I can do that. One comment on the commit hook:

For any reason I got over and over

0 files committed, 3 files failed to commit: STYLE: SegmentEditor used too much space after returning to None effect
								
Removed spacer from ui file and setting QSizePolicy to certain parts
warning: CRLF will be replaced by LF in Modules/Loadable/Segmentations/Widgets/Resources/UI/qMRMLSegmentEditorWidget.ui.
The file will have its original line endings in your working directory.
pre-commit hook failure
-----------------------

Modules/Loadable/Segmentations/Widgets/Resources/UI/qMRMLSegmentEditorWidget.ui:220: trailing whitespace.
+      <sizepolicy hsizetype="Preferred" vsizetype="Maximum">

There were no trailing spaces!

As a workaround I commited without that specific line and then edited the file on github directly.

@cpinter
Copy link
Member

cpinter commented Apr 27, 2018

This may be related to line endings. If you open the UI file with a text editor that can show the line endings you'll see inconsistency I think. As long as you could commit it, I believe it's fine though.

Removed spacer from ui file and setting QSizePolicy to certain parts
@che85
Copy link
Author

che85 commented Apr 27, 2018

@cpinter ready for testing

@cpinter
Copy link
Member

cpinter commented Apr 27, 2018

The commit looks good, thanks a lot!

I'm building it right now, and will test it as soon as it's done.

@@ -66,6 +66,9 @@ def setup(self):
self.addObserver(slicer.mrmlScene, slicer.mrmlScene.EndCloseEvent, self.onSceneEndClose)
self.addObserver(slicer.mrmlScene, slicer.mrmlScene.EndImportEvent, self.onSceneEndImport)

if hasattr(self.layout, "addStretch"):
self.layout.addStretch()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the hasattr check? Qt4/Qt5 difference?

Copy link
Author

Choose a reason for hiding this comment

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

Because not all qt layout classes have that method and if you use the SegmentEditor in your own module as follows, setup would crash (e.g. QFormLayout and QGridLayout don't have that method):

from SegmentEditor import SegmentEditorWidget
widget = qt.QWidget()
widget.setLayout(qt.QGridLayout())
segmentEditor = SegmentEditorWidget(parent=widget)
segmentEditor.setup()
widget.show()

@che85
Copy link
Author

che85 commented Apr 28, 2018

@cpinter Did you get to test it?

@cpinter
Copy link
Member

cpinter commented Apr 28, 2018

I just did some testing. I found only one thing: If I switch to an effect that has a long GUI like Logical operators, also click Show details, then switch back to None effect, then the segment list gets really really high, and it cannot be resized to be smaller with the separator either.

If this is not introduced by this change then I guess it can be integrated. Can you test it? If there are no further comments by tomorrow I'll integrate this (I have a feeling that it's urgent for you right?)

@che85
Copy link
Author

che85 commented Apr 29, 2018

I am having the same issue when using the nightly. In my experience qt can be such a pain. Let me check. Maybe I can find a solution for that.

@che85
Copy link
Author

che85 commented Apr 29, 2018

P.S. it's not that really really high in the nightly. Let me investigate.

@jcfr
Copy link
Member

jcfr commented Jun 25, 2018

@che85 Out of curiosity, did you have a chance to finalize your investigation ?

You may want to look at ctkDynamicSpacer

@jcfr
Copy link
Member

jcfr commented Dec 6, 2018

@che85 Ping

@che85
Copy link
Author

che85 commented Dec 6, 2018

Sorry for the late reply. Will look into this in a bit. Need to setup the build environment including QT Creator. Will get back to you then.

@jcfr
Copy link
Member

jcfr commented Jun 21, 2019

Closing. Duplicated of the more recent #1085

@jcfr jcfr closed this Jun 21, 2019
sankhesh pushed a commit to sankhesh/SlicerGitSVNArchive that referenced this pull request Nov 17, 2020
Includes:

Revision: 7210c5bcc37bf1140a04c97bfe1e27142735c6ab
Author: Andras Lasso <[email protected]>
Date: 2020-11-07 9:55:12 PM
Message:
ENH: Reduce storage path length of files stores in DICOM database (Slicer#942)

Study, series, and SOP instance UIDs were used to generate path for data sets copied into the DICOM database.

When database folder path or UIDs were long, this path length could very easily become longer than the file system maximum path limit

(about 256 characters on some systems, for example on Windows or when writing CDs).

Added useShortStoragePath flag so that developers can still switch between full and shortened paths.

---

Revision: 7803ebaeafb241a500ec6072f38b4a343fc5488b
Author: Andras Lasso <[email protected]>
Date: 2020-11-07 9:54:49 PM
Message:
ENH: Allow ctkMessageBox to store any don't-show-again button roles in settings (Slicer#941)

Previously, when dontShowAgain flag was enabled, only buttons with AcceptRole could be saved in settings.

Now the user can customize the list of button roles.

For example Yes and No button choice can be saved, while still not saving anything if user clicks Cancel.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants