Add support secondary arguments support to load_ui #81
Conversation
This reverts commit c61b33d.
|
In that example, it would be a dictionary of class name: class objects. I think the most common use is to just import all the custom widget classes into your module and then just pass There might be a better way to do this though, because I can test and make an example, but probably not until the weekend. |
That would be great, thanks. 👍 |
|
What are our thoughts on this?
|
Hm. That seems almost six years old and this is the first time I've seen this being mentioned. I wonder if we can find any info on whatever issues they're referring to ever got fixed. I'm noticing they refer to To be honest, I've always used The problem with I'd like to propose the following: If we see issues with |
|
It seems at least some bug fixing activity was made after 2010:
In the Google Groups post, bug 552 ("Segmentation fault when using QUiLoader and QTabWidget") was mentioned, which they have an old test for, created in 2012: Also, they say they added a warning to the documentation on QUiLoader: This warning is nowhere to be seen today: |
@bpabel In case you have the possibility to write an example, that would be great. We've written our tests around def test_load_ui_into_self_pyside():
"""load_ui: Load widgets into self using PySide"""
with ui() as fname:
with pyside():
from Qt import QtWidgets, load_ui
class MainWindow(QtWidgets.QMainWindow):
def __init__(self, parent=None):
QtWidgets.QMainWindow.__init__(self, parent)
load_ui(fname, self)
app = QtWidgets.QApplication(sys.argv)
window = MainWindow()
# Inherited from .ui file
assert hasattr(window, "pushButton")You can view all 4 tests here. We would need one test for PySide and one test for PyQt4 on the custom widgets attribute. |
|
@mottosso For some reason, I'm getting issues with the docker tests with this PR. Do you have any idea why that is – and, do you see this on your end as well? Travis doesn't get this error. |
I remember you asking this elsewhere, and I was about to answer it there but couldn't find it. :S Yes, I've got an idea, it's because for a QApplication to be instantiated, there needs to be an X-server running and we haven't started one in our Docker image. On Travis, one is being setup here. As Travis doesn't have access to a graphics card or display, this one is a virtual display, called Xvfb. We should be able to use this in Docker too. |
|
Ah, I've been searching and I did find references to xvfb. Do you think that's the best way forward? |
|
Need to somehow suppress all that stuff Xvfb spews out. This is from my OS X desktop, running Docker for Mac 1.12.0-rc-beta20: There's also a strange error appearing on my OS X laptop (running Docker for Mac 1.12 stable): |
I got this as well, I replaced the .ui contents with this instead, that I got from an empty Qt Designer with a single pushbutton, and then never saw the error again. Not entirely sure it was the cause, possibly the lacking newline at the end, possibly the encoding or xml version. Haven't had the opportunity to investigate much further. source = """\
<?xml version="1.0" encoding="UTF-8"?>
<ui version="4.0">
<class>Form</class>
<widget class="QWidget" name="Form">
<property name="geometry">
<rect>
<x>0</x>
<y>0</y>
<width>400</width>
<height>300</height>
</rect>
</property>
<property name="windowTitle">
<string>Form</string>
</property>
<widget class="QPushButton" name="pushButton">
<property name="geometry">
<rect>
<x>140</x>
<y>120</y>
<width>90</width>
<height>27</height>
</rect>
</property>
<property name="text">
<string>PushButton</string>
</property>
</widget>
</widget>
<resources/>
<connections/>
</ui>
""" |
Yes, without a graphics card or physical display we'll need to go virtual.
You can run GUIs with Xvfb too, they will simply be rendered to an in-memory location (that you could potentially render to a file or redirect to a display). To redirect GUIs directly, we'll need an X-server to redirect to, or something like an X-server emulator and VNC service like noVNC. In a desktop environment, that's possible. But it's less automatic and headless, which is what we need for tests. |
|
Ok, let's go with Xvfb then. 👍 |
| @@ -7,21 +7,16 @@ RUN apt-get update && apt-get install -y \ | |||
| xvfb | |||
|
|
|||
| # Nose is the Python test-runner | |||
| RUN pip install nose nosepipe | |||
| RUN pip install nose nosepipe xvfbwrapper | |||
There was a problem hiding this comment.
About this, have a look at xvfb-run. It does the same thing, but without infecting our tests. This might limit tests from running on anything but Linux, and adds that additional indentation to a lot of lines.
$ xvfb-run docker run ...
|
Tests with docker seems to run smoothly now: |
(Referring to this line note) Hm, I'm not too keen on that since it'll make it impossible to run that on OS X which I like to use. |
Nice! Added a variation of that as a test.
I performed a few local tests and yes, I'm now skipping anything with double leading underscores. I also cleaned up the descriptions in tests.py as well as adjusted the README accordingly. |
|
I think we're making great strides here. Based on the new tests, and the fact that this simplification of manually augmenting another class with a ui file works, I've got a suggestion on how to boil it all down into a minimal addition to the project. How about we leave the additional arguments out, and make these the official methods for working with ui-files and Qt.py? Method 1 import Qt
widget = Qt.load_ui("my.ui")
widget.show()Method 2 Class = type(setup_ui())
class MyWidget(Class):
pass
widget = MyWidget()
widget.show()If it so happens that this covers all use cases, then we'd be adding one hell of a feature, at the cost of an ultra-minimal amount of code (i.e. 0 lines) and a tiny amount of documentation. |
|
I love your suggestion, but I don't understand how Your first example is what we have today. But I don't understand your second example. The test I added which uses What I wish to achieve with the base instance argument in import sys
from Qt import QtWidgets, load_ui
def setup_ui(base_instance=None):
# assumes this PR is accepted as-is right now
return load_ui('ui.ui', base_instance)
app = QtWidgets.QApplication(sys.argv)
Class = type(setup_ui())
class MyWidget(Class):
def __init__(self, parent=None):
super(MyWidget, self).__init__(parent)
setup_ui(self)
widget = MyWidget()
widget.show()
app.exec_()If we were to add 0 lines of code and skip this PR you'd need to do something like this ... working example: import sys
from Qt import QtWidgets, load_ui
def setup_ui(base_instance=None):
ui = load_ui('ui.ui')
if not base_instance:
return ui
else:
for member in dir(ui):
if not member.startswith('__') and member is not 'staticMetaObject':
setattr(base_instance, member, getattr(ui, member))
return ui
app = QtWidgets.QApplication(sys.argv)
Class = type(setup_ui())
class MyWidget(Class):
def __init__(self, parent=None):
super(MyWidget, self).__init__(parent)
setup_ui(self)
widget = MyWidget()
widget.show()
app.exec_()Perhaps it is time to decide whether or not we should at all provide a this second argument to I'm all for presenting the above example code instead of accepting this PR, don't get me wrong. But I believe we've reached a point where if we can't collectively decide on which of all the different routes which have been presented so far the end-user is better off deciding themselves on which route to take. If this is the case, I'd like to propose we create an "examples" folder or something, where we can document usage and where anyone can contribute their hacks which utilises Qt.py. For instance, we could add all of the proposed solutions on how to load the .ui in PySide to mimic the behavior in PyQt4. We can add comments and valuable lessons learned etc. This would enable usage and guidance without requiring maintainability on our side (and make it easier to turn down such PRs in the future, directing such PRs to the "examples" instead). |
|
Ok, looks like we've caught a few different understanding of things, that's a good thing. This is the part I'm having trouble with. Class = type(setup_ui())
class MyWidget(Class):
def __init__(self, parent=None):
super(MyWidget, self).__init__(parent)
setup_ui(self)If you inherit from the GUI built from the What does |
Class = type(setup_ui())
class MyWidget(Class):
passis the same as class MyWidget(QtWidget.QMainWindow):
pass...and therefore it doesn't set up the ui from the file. I added the This means that you could do this to also set up the ui: def setup_ui(base_instance=None):
return load_ui('ui.ui', base_instance)
Class = type(setup_ui())
class MyWidget(Class):
def __init__(self, parent=None):
super(MyWidget, self).__init__(parent)
setup_ui(base_instance=self)But all of this would require this PR to be accepted for that to work, as the def setup_ui(base_instance=None):
return load_ui(sys.modules[__name__].ui_qmainwindow, base_instance)However, if the end-user provided their own def setup_ui(base_instance=None):
ui = load_ui('ui.ui')
if not base_instance:
return ui
else:
for member in dir(ui):
if not member.startswith('__') and member is not 'staticMetaObject':
setattr(base_instance, member, getattr(ui, member))
return ui |
|
Sorry for all the editing of my previous post. I'm done now. :) |
Aha! This is what I didn't understand. I would have thought that the instance returned by E.g. def load_ui():
class MyCustomUi(QMainWindow):
pass
return MyCustomUi()And in that way, getting the class from the instance would retrieve the original, that still contained the ui; i.e. The fact that it instead returns an instance of QMainWindow is utterly strange. :S |
Yeah, it's a bit odd. I agree 😕 |
|
But anyway, even if that would have worked... that approach wouldn't had worked if you wanted to load your ui into something else which already existed. |
That sounds backwards to me. Are you building off of a ui file, or is the ui file building off of your Python class? Treating the ui file as a class from which to inherit sounds at least reasonable, but again I haven't used this feature so I don't know. |
|
Today the functionality overwrites any existing widget: >>> widget = QtWidgets.QWidget()
>>> widget = load_ui(fname)
>>> print(type(widget))
<class 'PyQt5.QtWidgets.QMainWindow'>The second argument is intended to add the ui elements to the already existing widget. So when you provide However, I'm not 100% sure of where things are being overwritten and where they are not. So perhaps my code snippet is a bit misleading on what I'm trying to communicate...
Both, I guess. Consider this... you may be writing an UI which also uses .ui files as "modules" within that UI. So, okay, this is not the best example, but bare (bear) with me... You're making some kind of image viewer in Qt Designer to show the overall interface. So you base your main app/UI against the .ui file. Then you would perhaps want to code a widget to represent each instance of the actual image thumbnail. Ok. Let's call each such instance a "module". Now, what if you didn't make a simple image viewer, but some other kind of viewer which would show more complex "modules". I've resorted to using Qt Designer to design such modules. This means I use an .ui for the main interface but then I also load .ui files which are not supposed to serve the main interface so to speak. Perhaps this was overly explanatory and perhaps not answering what you were asking...? |
|
The thing to focus on, is really that when you load an .ui you often want to control into which location the UI's widgets end up. This is the main reason why I started this PR. Right now (master branch) you get new widgets everytime you load an .ui. With a So, instead of calling |
|
Ok, I've thought about it, and I don't think I'm not in a great position to make this call. I think what will need to happen is that we finish implementing the tests such that known corner cases are at least documented and tested, and then you'll decide whether we add it or not, @fredrikaverpil. This can go 1 of 3 ways.
Here's what I think so far.
What I don't like about it is simply how this is used. The fusing of an automatically generated GUI with something written by hand. But, having read up on it, this is how the Qt developers intended, so it's only fair we do the same. I would however like to offer one final challenge to the additional argument before fully embracing it. You mention using one ui file to augment your subclass, and then using minor ones as standalone additions to it. What is the below example missing. class Gui(object):
def __init__(self):
ui = load_ui("main.ui")
layout = ui.body.layout()
for page in ("page1.ui", "page2.ui"):
widget = load_ui(page)
layout.addWidget(page)
# Store reference
self.ui = ui
def show(self):
self.ui.show()
gui = Gui()
gui.show()Let me know if you need a full runnable example, or whether this communicates the point fully. And have a think about whether a dedicated |
Yes, I believe that was the design intent. At least that's how it's being used by end-users.
In your example, you first load the main UI into I think that's what's missing, with our discussions as context. However, I don't personally think there's anything wrong with your code example and I'm not on any kind of crusade here. I think your comments on this is sound and makes sense. I'm rewriting a lot of code at the moment using
Let me propose a fourth way :) (which I actually did propose in some other issue or PR, can't remember) How about we store testable examples of how to use the current functionality of I think back on #101 and what you said there...
I feel So, I'd say we have four options:
|
|
That approach feels comfortable to me. It means we won't have to make too many contraptions or add to much opinion to the codebase, but instead leave that for suggestions via examples and best practices. It will leave more code for the end-user. At first. But potentially save code long-term, considering obscurities and subtle differences unaccounted for or worked around in our codebase that would result in additional in code that way. I'd say it'd be a defining direction for the project to take. It will not sit well with everyone. Some may choose to build something in parallel to better suit their needs and that project may very well overshadow ours. Or not. Either way, I think it might ultimately help guide the way, if only to find which route not to take. So, reducing the four technical options into two, we're left with these high-level options.
If possible, I'd like to hear from @bpabel and @hdd before dropping the hammer. |
Having given this some thought, I'm personally leaning towards this. It's also in line with #101 |
|
Hi , sorry for the delay on joining the chat , just come back from a quick break. To me , and how I did started approaching the patches, the best way to go, is to start with the new api in mind (so, relying on QtWidgets), and provide patched methods to be able to run the legacy code (PySide/PyQt) (eg: #98 ). This allow the users to start using , and getting used to the new api, but having the ability to have their code working with not much to change. We should, though, warn the users, that they are using these patched methods (whether are forward or backward compatibility), so they are aware. My2c. |
Could you open an issue about this? I see a number of conversation topics on how to achieve this technically. |
|
done, see #109 |
|
Ok, let's commit to the goals discussed in this issue! Henceforth, Qt.py is about linking together commonalities between PySide2 and other bindings, and documenting differences, so as to foster a mindful developer of cross-compatible software. This is in contrast to wrapping all functionality that differs PySide2 with our own ad-hoc implementations, which we believe will cause more problems than it solves long-term. For alternative ideals, see the following forks and projects.
Any more? Let us know! |
|
For anyone reading this enormous thread, check out this pull request: #119 |
Fix #80
We can now support the base instance argument and the custom widgets argument in
load_uithanks to the custom class in this gist.Let's discuss.
How do we deal with the copyright message at the top of the gist?
We should give Sebastian Wiesner (lunaryorn@gmail.com) credit for this.
@bpabel - do you know how to use the third (custom widgets) argument?
I've added support for it but I'm not sure about its use case. I'd like to see an example of it being used so that I can add tests for it.
@mottosso I was unable to remove QStatusBar and QMenuBar from the tests UI without crash.