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

Make sure loadUi is available #27

Merged
merged 9 commits into from
Jun 14, 2016
Merged

Conversation

astrofrog
Copy link
Contributor

@astrofrog astrofrog commented Jun 9, 2016

Glue makes use of loadUi extensively, and there are a number of available patches going around to provide this in PySide. This PR adds such a patch (adapted from qt-helpers, which itself came from a previous solution).

This works fine, but one thing that all solutions seem to require is that for PySide, custom widgets need to be explicitly specified. However, this is not ideal because it provides an API that is not consistent with PyQt4/5 (where the custom widgets are auto-loaded).

One solution would be to parse the XML ourselves and load the custom widgets specified in the ui files (since the ui files provide the path to import the class). I can implement this, but I wanted to check first whether there are any objections to this?

As for #25, I included copies of the licenses for the adapted code, but there might be a better way to do this (for example we could include this in LICENSE, or have a separate file describing where various parts of the code were taken from and what their licenses were?)

Closes #16

cc @Nodd

@astrofrog astrofrog changed the title Make sure loadUi is available WIP: Make sure loadUi is available Jun 9, 2016
@ccordoba12 ccordoba12 added this to the v1.1 milestone Jun 9, 2016
@ccordoba12
Copy link
Member

@Nodd, could you review this one please?

@goanpeca
Copy link
Member

goanpeca commented Jun 9, 2016

@astrofrog I disabled the complaint on non pythonic names (which is not good advice when working with Qt)

But the other suggestions are valid:

  1. Consider documenting your class(es)Disable
    1 issue in 1 file.
  2. Consider documenting your function(s)
    1 issue in 1 file.

@astrofrog
Copy link
Contributor Author

@goanpeca @ccordoba12 - I've added fixes, but I think in this case the coding guidelines are being over-strict. The class that was missing a docstring was private (starting with _) and the function that was missing a docstring was a test.

@goanpeca
Copy link
Member

goanpeca commented Jun 9, 2016

@astrofrog yes probably, but even if private a docstring is always a good idea :-p. For the test one, yes I agree it is kinda too much, but thanks for including the changes 👍

@astrofrog astrofrog force-pushed the patch-load-ui branch 2 times, most recently from e5cbe60 to e354091 Compare June 10, 2016 15:19

__all__ = ['loadUi']

if os.environ[QT_API] in PYQT5_API:
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the constants here: PYQT5 and so on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@Nodd
Copy link
Contributor

Nodd commented Jun 11, 2016

The logic in createWidget is a bit different from python_qt_binding (https://github.com/ros-visualization/python_qt_binding/blob/kinetic-devel/src/python_qt_binding/binding_helper.py#L195). From what I understood it' calling QUiLoader.createWidget in more cases, but I don't know if it matters in practice.

@astrofrog Regarding PySide, has any other project tried to load the custom classes from the xml directly ?
The feature is very interesting, but for now I think it's better to mark the failing test as xfail if Pyside is used, and eventually fix it in another PR.
python_qt_binding has the following comment:


        # instead of passing the custom widgets, they should be registered using QUiLoader.registerCustomWidget(),
        # but this does not work in PySide 1.0.6: it simply segfaults...
        #loader = CustomUiLoader(baseinstance)
        #custom_widgets = custom_widgets or {}
        #for custom_widget in custom_widgets.values():
        #    loader.registerCustomWidget(custom_widget)

Can it be useful with a newer version of PySide ?

# TypeError, if customWidgets is None
try:
widget = self.customWidgets[class_name](parent)
except (TypeError, KeyError):
Copy link
Contributor

Choose a reason for hiding this comment

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

If self.customWidgets is set to an empty dict if None (in the constructor), you would only catch KeyError here. It would be easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@astrofrog
Copy link
Contributor Author

Can it be useful with a newer version of PySide ?

Yes but we'd still need to get the custom widgets from the XML, which I think should be reasonably easy. However, I can indeed do it in a separate PR.

@Nodd
Copy link
Contributor

Nodd commented Jun 11, 2016

@goanpeca Can you disable "Avoid using "non-Pythonic" variable names" in QuantifiedCode, please ?

@ccordoba12
Copy link
Member

However, I can indeed do it in a separate PR.

Yep, please leave it for another PR.

@astrofrog
Copy link
Contributor Author

@ccordoba12 - done!

@astrofrog astrofrog changed the title WIP: Make sure loadUi is available Make sure loadUi is available Jun 11, 2016
@ccordoba12
Copy link
Member

@Nodd, please merge this one when you consider is ready.


class _QComboBoxSubclass(QComboBox):
"""
This is a private class that is used only for testing purposes.
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's only needed for testing, the class can be defined in test_uic.py, to leave uic.py clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that it needs to be part of an importable module. I could create a fake module for the duration of the test though, so I can investigate that.

Copy link
Contributor

Choose a reason for hiding this comment

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

The test file itself is importable, isn't it sufficient ?
Otherwise I think that having a separate file in the test directory containing the class is fine. Your workaround works but seems needlessly complex to me.

Copy link
Contributor Author

@astrofrog astrofrog Jun 14, 2016

Choose a reason for hiding this comment

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

I tried having a file in the tests directory, but this causes issues in Python 2, because it seems that when I adjust sys.path, the class I import from the tests appears to be different from the one that PyQt4/5 imports. I tried working around that, but in the end, the solution in this PR is the cleanest (even though it may look complex).

Copy link
Contributor Author

@astrofrog astrofrog Jun 14, 2016

Choose a reason for hiding this comment

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

By the way, the reason I have to mess with sys.path and why this is complicated is because the tests need to also be runnable from outside the tests directory, so I want to be able to do py.test tests/ for instance.

Copy link
Contributor Author

@astrofrog astrofrog Jun 14, 2016

Choose a reason for hiding this comment

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

Here's an example of the type of error I get if I try the approach you are suggesting:

>       assert isinstance(ui.comboBox, _QComboBoxSubclass)
E       assert isinstance(<test_uic._QComboBoxSubclass object at 0x11882f5e8>, _QComboBoxSubclass)
E        +  where <test_uic._QComboBoxSubclass object at 0x11882f5e8> = <PyQt5.QtWidgets.QWidget object at 0x11882de58>.comboBox

somehow it seems to think there are two different _QComboBoxSubclass classes.

@Nodd
Copy link
Contributor

Nodd commented Jun 13, 2016

Sorry for the last-minute nitpick, otherwise it's ready to go !

@astrofrog
Copy link
Contributor Author

@Nodd - I think this is good to go - I know the solution here seems a little strange, but see my replies to your comments above: I can't figure out a way to get this to work if the subclass lives in the test file.

@Nodd
Copy link
Contributor

Nodd commented Jun 14, 2016

It's OK, there are problems in python if the same module is imported from 2 different paths. Thank you for your work !

@Nodd Nodd merged commit f96bdb6 into spyder-ide:master Jun 14, 2016
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.

4 participants