-
Notifications
You must be signed in to change notification settings - Fork 3k
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
new wizard Qt desktop client #8560
Conversation
23febe5
to
eda2d7b
Compare
|
||
|
||
class QEAbstractWizard(QDialog): | ||
_logger = get_logger(__name__) |
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 think it is nicer to inherit Logger, e.g. QEAbstractWizard(QDialog, Logger)
, and then
def __init__(self, config: 'SimpleConfig', app: QApplication, daemon):
Logger.__init__(self)
Especially as QEAbstractWizard
is not a singleton, it could be useful for debugging to have separate instances of it each their own logger. E.g. look at wallet.py/Abstract_Wallet: note it is overriding Logger.diagnostic_name. -- Though if you don't override diagnostic_name, the choice of having a per-class/per-instance logger becomes less important.
I guess this remark also applies to a lot of existing code in the qml GUI.
Anyway, it's more of a nit.
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.
Thanks. I guess this stems from my past experience with C++ and Java, where inheritance is more rigid. Pythonic multiple inheritance and mixins still feels a bit 'loose' to me, and although you could maintain the is a relationship, I'm used to loggers being a composition into a class. I guess the difference in interpretation is that a class using a logger should not imply the class being a full fledged logger within a logging framework.
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.
BTW, diagnostic_name
might not be unique unless I use the id()
of the wizard instance.
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 used to loggers being a composition into a class
You are right that inheritance should not be overused.
Hmm, I guess it is mostly more consistent usage of the Logger across the codebase that I wanted, not necessarily inheritance.
We could change the Logger class to be used with composition instead. diagnostic_name
and LOGGING_SHORTCUT
could be passed as parameters to Logging.__init__
,
and then do e.g. self.logger = Logger(name=self.diagnostic_name(), shortcut=LOGGING_SHORTCUT)
in init.
diagnostic_name
might not be unique unless I use theid()
of the wizard instance
Yes, indeed. See lnpeer.Peer
for an example where we explicitly use id()
(through the transport) -- it was added during/after debugging an issue where multiple Peer objects did have the same colliding diagnostic names which was causing confusion.
Anyway, it's just a small remark; not too important re this PR.
17af9dd
to
c48fc00
Compare
2ae343a
to
538b91a
Compare
cb5758d
to
9dc9a55
Compare
@ecdsa, @SomberNight I've started adding HWW support for devices I can't test.
|
For my information, which HWW allow for initialization within Electrum? For trezor I'm sure (and is implemented), |
320ef47
to
cc841c9
Compare
Removing draft status. Not all HWW that currently support initial device setup are ported, but it's ready to test. |
Can we rename |
Done |
7d006a2
to
c30740a
Compare
qt: call is_finalized before closing the wizard dialog and add a check if wallet can be decrypted with the supplied secret (user pw, hw device)
…nd picked wallets, add abstract method decl HW_PluginBase.wizard_entry_for_device
… report error when exception
…-add rescan button in choose hw device, clear clipboard before confirming seed.
… unexpected keystore type msg
also, consistent single quoting of strings
add auto-proceed to next page after init to trezor, safe_t, keepkey
- always store 'keystore_type' in cosigner data and use same types as main - dont share 'hardware_device' in root of dict, but store for each cosigner - properly return hardware keystore for hardware cosigners
84e5e60
to
2caa8f1
Compare
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.
Added some more comments. Also tested on safe-t, looks ok.
… standard wallets, check hw wallet file decrypt in WCHWUnlock, fix assumption 'wallet_type' exists in wallet open scenario.
6d054d7
to
44a1595
Compare
not directly related, but the Since that is not needed on Android, I think this should be removed from qml |
follow-up 8be3c4d related: #8560 (comment)
Include new wizard Qt desktop client spesmilo#8560 WIP TODO: build binaries, adapt qrcodewidget
Refactor of wizard to new wizard framework (as in qml).
Notes:
Todo: