-
Notifications
You must be signed in to change notification settings - Fork 14
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
Conversation
There was a problem hiding this 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)
Sorry, my bad: you need to run |
The constructor of the An immediate workaround is to call both superclasses' constructors directly rather than using A better solution, in my opinion, is avoiding multiple inheritance altogether: its headaches are not worth it, and composition is clearer anyway: the The only other instance of multiple inheritance in the |
There was a problem hiding this 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.
tinywallet/pyproject.toml
Outdated
"License :: OSI Approved :: ISC License (ISCL)", | ||
"License :: OSI Approved :: GNU Lesser General Public License v3 (LGPLv3)", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ." |
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 Should open an issue @ Qt, but I probably won't. |
This migrates the
tinywallet
package from PyQt5 to PySide2, while changing its license from ISC to LGPLv3. Needs runningpoetry install
to update the dependencies in the virtual environment.EDIT: needs running
poetry install
, notpoetry update
.Fixes #166.