Ignore the default empty layout objects when loading a .ui file #217

tbttfox · 2017-07-13T00:37:13Z

Lets try this again, this time without the phantom user:

The default empty layout object bool(QMainWindow().layout()) evaluates to True in PySide and False in PyQt4. So I added a check to see if an existing layout is empty.

tbttfox · 2017-07-13T00:48:23Z

Ok, I'm getting a Travis build failure on the PyQt implementations. From what I can track down, calling QMainWindowLayout::count() emits the warning qWarning("QMainWindowLayout::count: ?");, and that warning is what's causing the failure from inside uic.loadUi. However, I haven't the foggiest idea how to either fix it, or have the test ignore that warning.

mottosso · 2017-07-13T06:01:30Z

Thanks @tbttfox.

I've tracked down the original commit from which this clause got added, I think it'd be helpful to future readers if we included a comment on what it's doing there.

e4a736a#diff-cd1d7c0a982bce680146d5b248151256R861

The reason being to mimic PyQt throwing an exception. Which now that I think of it sounds a little backwards. It's PyQt who's supposed to mimic PySide2.

Do you think it's possible to have PyQt support this feature, as opposed to PySide inheriting shortcomings of PyQt? It might not be, in which case this would be fine.

tbttfox · 2017-07-13T20:44:05Z

From what I can see, PyQt does support this feature. Nosepipe is just confused by the qwarning (that doesn't actually break anything), and I'm not familiar enough with nosepipe to tell it that the warning is expected, because it's not a normal python error.

And FYI:
My test passes all cases if I remove the layout existence check (the other tests fail, of course). ~~So I'm guessing the act of checking for a layout on a new QMainWindow instance creates the layout~~ [edit: did more testing, and I was totally wrong there]. PyQt4 then emits that qwarning when the ui is loaded.

MHendricks · 2017-08-23T19:50:01Z

From my testing for both PyQt and PySide:

QMainWindow().layout() returns a QLayout with a count of 0.
QDialog().layout() returns None.
QDockWidget().layout() returns a QLayout with a count of 2. It stores the close and float buttons used internally.

And if you call bool(widget.layout()) or if widget.layout():

QMainWindow returns False in PyQt, and True for PySide
QDialog returns False in both PyQt and PySide
QDockWidget returns True in both PyQt and PySide

This is consistent in both Qt4 and and Qt5 versions.

This means that due to the artificial check, you can't use Qt.QtCompat.loadUi on QDockWidget at all, and only in PySide/2 you cant use loadUI on QMainWindow.

I've done some testing and PyQt4/5 do not error if you use PyQt5.uic.loadUI on a Widget that already has a layout assigned to it. I used the code from the unit test to verify that. I only have access to PySide2 from inside Maya so I haven't verified this in PySide.

Qt's c++ code for QLayout raises a qWarning 'QLayout: Attempting to add QLayout "" to QMainWindow "", which already has a layout' I'm assuming this goes to stderr, and nose detects it as a error.

This is a c++ output, so I'm not sure if you can redirect its stderr output away from nose, but I think this is the original reason for raising the RuntimeError in Qt.QtCompat.loadUi. Just so the unittests didn't error out.

Here is a simple example of the problem, in this case it was run with PyQt4 from a windows command prompt:

>>> import sys
>>> from Qt import QtWidgets
>>> app = QtWidgets.QApplication(sys.argv)
>>> win = QtWidgets.QMainWindow()
>>> layout = QtWidgets.QVBoxLayout(win)
QLayout: Attempting to add QLayout "" to QMainWindow "", which already has a layout
>>>

No error was raised, but "QLayout: Attempting ..." is written to the command prompt, not python. I haven't seen it in Maya, but I assume its there just not being shown.

Is it possible to make nose ignore stderr from c++ for these specific tests? Or make Qt not log qWarnings? I don't think the check added in e4a736a is needed at all and it actually breaks things.

MHendricks · 2017-08-23T21:52:44Z

I did a little more searching and I think we could control the qWarning output using this function already exposed in Qt.py

http://doc.qt.io/qt-5/qtglobal.html#qInstallMessageHandler

Some example code that looks promising: https://stackoverflow.com/a/25681472

mottosso · 2017-08-24T07:34:24Z

Thanks for the excellent investigatory notes, @MHendricks.

I don't think the check added in e4a736a is needed at all and it actually breaks things.

Would you be able to take the lead on resolving that? I don't use loadUi myself so it's hard for me to find out about these kinds of problems.

MHendricks · 2017-08-24T21:36:19Z

It may be a few days before I can get to this (production support gets in the way). You don't need to test with loadUi, just attempt to create a layout and pass a parent widget(that already has a layout) into its constructor like I did in the code sample above.

I'll try to find some time to give it a try, or I can help @tbttfox debug it if he has time.

CLAassistant · 2017-09-12T23:02:05Z

All committers have signed the CLA.

…i-layout-fix

houndci-bot · 2017-09-13T04:08:20Z

tests.py

+
+
+if binding("PyQt5") or binding("PySide2"):
+    def ignoreQtMessageHandlerFactory(msgs):


redefinition of unused 'ignoreQtMessageHandlerFactory' from line 155

houndci-bot · 2017-09-13T04:08:20Z

tests.py

+
+
+if binding("PyQt5") or binding("PySide2"):
+    def ignoreQtMessageHandlerFactory(msgs):


redefinition of unused 'ignoreQtMessageHandlerFactory' from line 155

houndci-bot · 2017-09-13T04:11:00Z

tests.py

+		if msg in msgs:
+			return
+		sys.stderr.write("{0}\n".format(msg))
+	return ictxtMgr


indentation contains tabs

houndci-bot · 2017-09-13T04:11:00Z

tests.py

+			msg = msg.decode()
+		if msg in msgs:
+			return
+		sys.stderr.write("{0}\n".format(msg))


indentation contains tabs

houndci-bot · 2017-09-13T04:11:00Z

tests.py

+		if binding("PySide2"):
+			msg = msg.decode()
+		if msg in msgs:
+			return


indentation contains tabs

houndci-bot · 2017-09-13T04:11:01Z

tests.py

+		msg = args[-1]
+		if binding("PySide2"):
+			msg = msg.decode()
+		if msg in msgs:


indentation contains tabs

houndci-bot · 2017-09-13T04:11:01Z

tests.py

+	def ictxtMgr(*args):
+		msg = args[-1]
+		if binding("PySide2"):
+			msg = msg.decode()


indentation contains tabs

houndci-bot · 2017-09-13T04:11:01Z

tests.py

+def ignoreQtMessageHandlerFactory(msgs):
+	def ictxtMgr(*args):
+		msg = args[-1]
+		if binding("PySide2"):


indentation contains tabs

houndci-bot · 2017-09-13T04:11:01Z

tests.py

@@ -122,6 +151,17 @@ def binding(binding):
    return os.getenv("QT_PREFERRED_BINDING") == binding


+def ignoreQtMessageHandlerFactory(msgs):
+	def ictxtMgr(*args):
+		msg = args[-1]


indentation contains tabs

houndci-bot · 2017-09-13T04:11:01Z

tests.py

@@ -122,6 +151,17 @@ def binding(binding):
    return os.getenv("QT_PREFERRED_BINDING") == binding


+def ignoreQtMessageHandlerFactory(msgs):
+	def ictxtMgr(*args):


indentation contains tabs
indentation contains mixed spaces and tabs

tbttfox · 2017-09-13T04:33:19Z

Ok, so @MHendricks and I got together and got this working, but there's some extra stuff we found out

In the Qt4 bindings, the message handler is called qInstallMsgHander
In the Qt5 bindings, it's called qInstallMessageHandler
In both cases, they take exactly 1 function as an argument... however, that function requires a different signature between 4 and 5 (Two args in Qt4, vs. Three args in Qt5)

So, for now, we've added qInstallMessageHandler as a misplaced member, and it's up to the implementation to handle the different signatures. For the tests we just grab *args and parse from there.

There is probably a bigger discussion to be had on how to handle the message handlers, but I think we can do that in a separate PR.

houndci-bot · 2017-09-13T04:55:29Z

tests.py

+def test_load_ui_mainwindow():
+    """Tests to see if the baseinstance loading loads main windows properly"""
+    import sys
+    from Qt import QtWidgets, QtCompat, QtCore


'Qt.QtCore' imported but unused

houndci-bot · 2017-09-13T04:55:29Z

tests.py

+        msgs: list of message strings to ignore
+    """
+    from Qt import QtCore
+    def messageOutputHandler(*args):


expected 1 blank line before a nested definition, found 0

fredrikaverpil · 2017-09-13T09:22:11Z

I'm sorry for the silence here. I'm very busy at work and my wife is expecting in about a week... so I'm not sure exactly when I'll be able to review this.

…i-layout-fix

tbttfox · 2017-12-02T00:20:29Z

I pulled some commits from @MHendricks.
The layout existence check in _loadUi was removed. Having that check was causing failures on Linux where the code would run just fine otherwise. The test checking for this was removed.

Tests for running loadUi on QMainWindow QDialog and QDockWidget were added.

MHendricks · 2017-12-02T01:10:22Z

There is some inconsistency between the windows and Linux builds of Qt4 when dealing with QDockWidgets.

>>> from Qt.QtWidgets import QDockWidget
>>> dw = QDockWidget()
>>> dw.layout().count()
2
>>> dw.layout().isEmpty()
True

For QDockWidgets on Windows, isEmpty() returns True, but on Linux it returns False. I'm not sure why the layout existence check in _loadUi was added in the first place so I removed it.

QMainWindow has a default .layout() object and return False when .layout().isEmpty() is called on a new instance. This seems consistent for both Linux and Windows.

QDialog and QWidget do not have a default layout.

Also, thanks @tbttfox for cleaning up the unfinished mess I left on my test repo.

mottosso · 2017-12-04T07:54:07Z

For QDockWidgets on Windows, isEmpty() returns True, but on Linux it returns False.

Maybe we could add a QtCompat.isEmpty(my_widget) to account for the differences?

I'm not sure why the layout existence check in _loadUi was added in the first place so I removed it.

If you select a line here on GitHub and press the "Blame" button, it'll tell you the commit message for that line. In this case, I found:

Added exception to PySide loadUi if there is an existing layout to mimic PyQt4 behavior
reference

Not sure what this means given the changes in this PR. @dgovil do you remember what issues you where having before that commit? Does this resolve those issues?

QtCore.qInstallMessageHandler

Not sure how this slipped in here, there hasn't been any mention of this so far? I'm happy to keep it though, it looks well thought out and documented.

tbttfox · 2017-12-04T09:03:32Z

QtCore.qInstallMessageHandler

Not sure how this slipped in here, there hasn't been any mention of this so far? I'm happy to keep it though, it looks well thought out and documented.

It's been mentioned a couple times, but just for some explicit background: When checking for the non-existent layouts (in the check we removed), Qt would call a qWarning in some cases. This writes to the C++ stdout, which we couldn't seem to access from Python. So we had to use QtCore.qInstallMessageHandler to to either redirect, or silence that warning, otherwise NosePipe would get unexpected output, and fail.

Now that the check and the associated tests have been removed, it's not a strictly necessary part of this PR, but it's definitely worth having.

dgovil · 2017-12-04T19:58:57Z

@mottosso It seems PyQt4 would error if you tried to load a ui file on to a widget that already had a layout assigned, which was the reason for that warning.

However I've recently discovered that it really depends on the PyQt version whether that is an issue or not. Some versions do not care while others do.

I'm not sure what's the best way to proceed here? IIRC the version that the tests download will fail, but the version we have at work does not. Not sure what's entirely different and noone seems to have answered me.

Maybe it's best to ignore them and if the test fails, acknowledge that it's expected behavior?

tbttfox · 2017-12-05T00:07:45Z

So I added a test to see that error. And it wasn't an actual error, it was just nosepipe freaking out about a qWarning again. I wrapped the test in a message handler, and everything passes!

tbttfox · 2017-12-05T05:00:13Z

After some thought, we decided that qInstallMessageHandler should be moved to QtCompat because of the handler signatures are different, and the types passed to the handlers are different.

And with that, I think this beast is finally ready, pending review.

mottosso · 2017-12-05T08:38:36Z

Well done everyone. I think it looks great. I've bumped the version and will merge and release once the tests do one final round.

mottosso · 2017-12-20T06:17:39Z

Merging this, sorry for the delay!

Ignore the default empty layout objects when loading a .ui file

b9f9eb6

tbttfox mentioned this pull request Jul 13, 2017

QtCompat.loadUi layout check Passes PyQt4, fails PySide #215

Closed

Add a handler to ignore the QMessage

d2bb8d4

tbttfox force-pushed the loadUi-layout-fix branch from 2e0face to d2bb8d4 Compare September 12, 2017 23:44

tbttfox added 2 commits September 12, 2017 21:06

Merge branch 'master' of /~https://github.com/mottosso/Qt.py into loadU…

7adb186

…i-layout-fix

Create/install qmessage handlers for dealing with QWarnings

f106974

houndci-bot reviewed Sep 13, 2017

View reviewed changes

Release the hounds

05ce6ea

houndci-bot reviewed Sep 13, 2017

View reviewed changes

tbttfox added 2 commits September 12, 2017 21:12

sit, ubu, sit. good boy.

f5d45b0

Decode the bytestring for PyQt4 as well

94ea9fe

Change the handler factory to a contextmanager

e3dacd8

houndci-bot reviewed Sep 13, 2017

View reviewed changes

tbttfox added 2 commits September 12, 2017 21:57

Silence the hound

7b03167

Add an entry to the caveats.md talking about qInstallMessageHandler

dd78c53

tbttfox and others added 4 commits September 13, 2017 10:30

doctest formatting

f57b751

Create tests for common ui files.

580ab71

Layout error check

74c09fa

Merge branch 'master' of /~https://github.com/mottosso/Qt.py into loadU…

3b05297

…i-layout-fix

tbttfox added 3 commits December 1, 2017 15:30

Merge branch 'master' of /~https://github.com/mottosso/Qt.py into loadU…

305c2e2

…i-layout-fix

Woof (comment styling)

94bd267

Remove superfluous comments

a03c4fd

tbttfox added 3 commits December 4, 2017 15:18

test commit for layout overwriting

30396b0

Add handler ignore

ada47c6

added a test for loadLayout onto an existing layout

b5d2d11

tbttfox added 10 commits December 4, 2017 16:22

Added tests for the other window types

57ee23d

tab/space fix

da43374

Add the ignore strings per type tested

eff29dc

fix widget type

cbbb2f4

Fix message, and appease the hound

05ea7a1

Add the tests for the other top-level widget types

47ff4fa

Move messageHanlder to qtcompat

e7e3e1a

remove functools wrap

728d6e7

Remove the messagehandler caveat

5a0d9ea

Merge branch 'loadUi-layout-fix-dev' into loadUi-layout-fix

4cb8247

Version bump

01a61d3

Version bump

4e43f77

Merge branch 'master' into loadUi-layout-fix

9531a0f

mottosso merged commit 952b91d into mottosso:master Dec 20, 2017

MHendricks mentioned this pull request Jan 31, 2018

Loading QMainWindow with QtCompat.loadUi() #274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore the default empty layout objects when loading a .ui file #217

Ignore the default empty layout objects when loading a .ui file #217

tbttfox commented Jul 13, 2017

tbttfox commented Jul 13, 2017 •

edited

Loading

mottosso commented Jul 13, 2017

tbttfox commented Jul 13, 2017 •

edited

Loading

MHendricks commented Aug 23, 2017

MHendricks commented Aug 23, 2017

mottosso commented Aug 24, 2017

MHendricks commented Aug 24, 2017

CLAassistant commented Sep 12, 2017 •

edited

Loading

houndci-bot Sep 13, 2017

houndci-bot Sep 13, 2017

houndci-bot Sep 13, 2017

houndci-bot Sep 13, 2017

houndci-bot Sep 13, 2017

houndci-bot Sep 13, 2017

houndci-bot Sep 13, 2017

houndci-bot Sep 13, 2017

houndci-bot Sep 13, 2017

houndci-bot Sep 13, 2017

tbttfox commented Sep 13, 2017

houndci-bot Sep 13, 2017

houndci-bot Sep 13, 2017

fredrikaverpil commented Sep 13, 2017

tbttfox commented Dec 2, 2017

MHendricks commented Dec 2, 2017

mottosso commented Dec 4, 2017

tbttfox commented Dec 4, 2017

dgovil commented Dec 4, 2017 •

edited

Loading

tbttfox commented Dec 5, 2017

tbttfox commented Dec 5, 2017

mottosso commented Dec 5, 2017

mottosso commented Dec 20, 2017



		if binding("PyQt5") or binding("PySide2"):
		def ignoreQtMessageHandlerFactory(msgs):

Ignore the default empty layout objects when loading a .ui file #217

Ignore the default empty layout objects when loading a .ui file #217

Conversation

tbttfox commented Jul 13, 2017

tbttfox commented Jul 13, 2017 • edited Loading

mottosso commented Jul 13, 2017

tbttfox commented Jul 13, 2017 • edited Loading

MHendricks commented Aug 23, 2017

MHendricks commented Aug 23, 2017

mottosso commented Aug 24, 2017

MHendricks commented Aug 24, 2017

CLAassistant commented Sep 12, 2017 • edited Loading

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

tbttfox commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikaverpil commented Sep 13, 2017

tbttfox commented Dec 2, 2017

MHendricks commented Dec 2, 2017

mottosso commented Dec 4, 2017

tbttfox commented Dec 4, 2017

dgovil commented Dec 4, 2017 • edited Loading

tbttfox commented Dec 5, 2017

tbttfox commented Dec 5, 2017

mottosso commented Dec 5, 2017

mottosso commented Dec 20, 2017

tbttfox commented Jul 13, 2017 •

edited

Loading

tbttfox commented Jul 13, 2017 •

edited

Loading

CLAassistant commented Sep 12, 2017 •

edited

Loading

dgovil commented Dec 4, 2017 •

edited

Loading