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

tinywallet: migrate from PyQt5 to PySide2 #170

Closed
wants to merge 5 commits into from
Closed

tinywallet: migrate from PyQt5 to PySide2 #170

wants to merge 5 commits into from

Conversation

teknico
Copy link
Collaborator

@teknico teknico commented Apr 17, 2020

This migrates the tinywallet package from PyQt5 to PySide2, while changing its license from ISC to LGPLv3. Needs running poetry install to update the dependencies in the virtual environment.

EDIT: needs running poetry install, not poetry update.

Fixes #166.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Running poetry update in tinywallet updated the poetry.lock file, which is commited. I forget, am I not supposed to run update?

Also, when trying to restore a wallet from seed:

stacktrace
[joe@hyrule tinywallet]$ poetry run python tinywallet/app.py --testnet --loglevel=debug
2020-04-20 15:34:52,417 app INFO initLogging(169) configuration file at /home/joe/.local/share/TinyWallet/tinywallet.conf
2020-04-20 15:34:52,417 app INFO initLogging(170) data directory at /home/joe/.local/share/TinyWallet
<class 'AttributeError'> 'TinyWallet' object has no attribute 'threads' <traceback object at 0x7f084905ba80>
Traceback (most recent call last):
  File "/home/joe/git/tinydecred/tinywallet/tinywallet/screens.py", line 831, in done
    self.callback(self.pwInput.text())
  File "/home/joe/git/tinydecred/tinywallet/tinywallet/screens.py", line 2083, in withpw
    app.waitThread(create, finish, pw)
  File "tinywallet/app.py", line 232, in waitThread
    self.makeThread(run, cb)
  File "/home/joe/git/tinydecred/tinywallet/tinywallet/qutilities.py", line 65, in makeThread
    for oldThread in self.threads:
AttributeError: 'TinyWallet' object has no attribute 'threads'
QThread: Destroyed while thread is still running
Aborted (core dumped)

@teknico
Copy link
Collaborator Author

teknico commented Apr 20, 2020

Sorry, my bad: you need to run poetry install, not poetry update. Looking into the other problem.

@teknico
Copy link
Collaborator Author

teknico commented Apr 20, 2020

Also, when trying to restore a wallet from seed:

The constructor of the Q.ThreadUtilities class is not being invoked. It looks like PySide2 is breaking multiple inheritance via super, while PyQt5 doesn't do that.

An immediate workaround is to call both superclasses' constructors directly rather than using super, see the 08586ab commit.

A better solution, in my opinion, is avoiding multiple inheritance altogether: its headaches are not worth it, and composition is clearer anyway: the TinyWallet class can have instance of the Q.ThreadUtilities class as an attribute.

The only other instance of multiple inheritance in the TinyWallet codebase is screens.SVGWidget. Its constructor also doesn't use super, but calls the first superclass constructor directly. The second superclass constructor is not called, not sure if that's intentional.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Everything seems to be working fine for me now. I don't pretend to understand the need for these changes, but they look good to me.

"License :: OSI Approved :: ISC License (ISCL)",
"License :: OSI Approved :: GNU Lesser General Public License v3 (LGPLv3)",
Copy link
Member

Choose a reason for hiding this comment

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

We may not have to change our license. Check out the source code for PySide2. There is a file called LICENSE.GPLv3-EXCEPT which says

As a special exception you may create a larger work which contains the output of this application and distribute that work under terms of your choice, so long as the work is not otherwise derived from or based on this application and so long as the work does not in itself generate output that contains the output from this application in its original or modified form.

Copy link
Collaborator Author

@teknico teknico Apr 21, 2020

Choose a reason for hiding this comment

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

IANAL, but this text seems geared toward an application or tool rather than a library, so much that it's weird finding it as part of a library.

TinyWallet clearly seems based on PySide2, so I don't think this exception applies anyway.

Copy link
Member

Choose a reason for hiding this comment

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

TinyWallet clearly seems based on PySide2

I'm not sure exactly what that means. Does "based on" have a particular definition that you're using? TinyWallet offers no GUI tookit utilities.

IANAL

Me neither, which is just as much of an argument to not change the license. But regardless, I did read the LGPLv3, and it seems clear that we are an Application, that together with the Library form a Combined Work which we are free to distribute under our terms as long as we don't modify the terms of the Library's license (see section 4). We of course must leave the license file, but PyPI already distributes that for us.

The only place that I'm not sure that we're compliant is where it says.

Give prominent notice with each copy of the Combined Work that the Library is used in it and that the Library and its use are covered by this License.

Copy link
Collaborator Author

@teknico teknico Apr 21, 2020

Choose a reason for hiding this comment

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

I'm not sure exactly what that means. Does "based on" have a particular definition that you're using?

I understand "derived work" and "based on" as something that could not exist without a library it uses. This is debatable given the existence of PyQt5 with the same API, but that has a more restrictive license (GPL).

it seems clear that we are an Application, that together with the Library form a Combined Work which we are free to distribute under our terms

You're probably right. The FSF says:

"using the Lesser GPL permits use of the library in proprietary programs" and if we can do that we can also use a different, much less restrictive license.

The only place that I'm not sure that we're compliant is where it says:

I guess mentioning PySide2 and its license in TinyWallet's README should be prominent enough. We can mention them in the main README too, to be on the safe side.

@teknico teknico requested a review from buck54321 April 22, 2020 14:10
@buck54321
Copy link
Member

I see you still have the LGPLv3 in the tinywallet directory. We do distribute the license with PySide2, but that requirement is satisfied by PyPI, I think.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Lost drop shadows.
image

@teknico
Copy link
Collaborator Author

teknico commented Apr 22, 2020

Lost drop shadows.

Not sure why this is happening. Probably not relevant but this is the only hint I can find in the PySide2 docs:

"Graphics effects are not supported for OpenGL-based widgets, such as QGLWidget , QOpenGLWidget and QQuickWidget ."

@buck54321
Copy link
Member

buck54321 commented Apr 25, 2020

I spent some time poking around, but I couldn't find a solution either. I guess we just get rid of them. Maybe just replace them with a border. Or we create a QWidget subclass with a custom paintEvent and do them ourselves.

Should open an issue @ Qt, but I probably won't.

@teknico teknico closed this by deleting the head repository Jan 10, 2023
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.

tinywallet: license
3 participants