Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions electrum/gui/qml/qeaddresslistmodel.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,9 @@ def __init__(self, wallet: 'Abstract_Wallet', parent=None):
self._filterModel = None

self.register_callbacks()
self.destroyed.connect(lambda: self.on_destroy())
self.destroyed.connect(self.on_destroy)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why were the lambdas there before?
are you sure they are not needed?

ref https://stackoverflow.com/questions/16842955/widgets-destroyed-signal-is-not-fired-pyqt#comment58357066_35304400 :

Sure it can work: just use a lambda, as I originally suggested. That is, you can just do: self.destroyed.connect(lambda: self.method()). The closure will keep self alive, as well as all its attributes and methods (although you must be careful which ones you access whilst the object is being deleted). Of course, if destroyed is directly connected to an instance method, it cannot work, because the connection will be automatically broken if the method is deleted.


(1) I tested with the following patch (on master), opening the addresses list and then closing the app:

diff --git a/electrum/gui/qml/qeaddresslistmodel.py b/electrum/gui/qml/qeaddresslistmodel.py
index fa7a546a6..6cd551214 100644
--- a/electrum/gui/qml/qeaddresslistmodel.py
+++ b/electrum/gui/qml/qeaddresslistmodel.py
@@ -123,7 +123,7 @@ class QEAddressCoinListModel(QAbstractListModel, QtEventListener):
         self._filterModel = None
 
         self.register_callbacks()
-        self.destroyed.connect(lambda: self.on_destroy())
+        self.destroyed.connect(self.on_destroy)
 
         QEConfig.instance.freezeReusedAddressUtxosChanged.connect(lambda: self.setDirty())
 
@@ -131,6 +131,7 @@ class QEAddressCoinListModel(QAbstractListModel, QtEventListener):
         self.initModel()
 
     def on_destroy(self):
+        print(f"heyheyhey. QEAddressCoinListModel.on_destroy() running")
         self.unregister_callbacks()
 
     @qt_event_listener

The print statement does not seem to run.


(2) I also tested without changing the logic, on master, and the print statement is visible on stdout:

diff --git a/electrum/gui/qml/qeaddresslistmodel.py b/electrum/gui/qml/qeaddresslistmodel.py
index fa7a546a6..1415683f7 100644
--- a/electrum/gui/qml/qeaddresslistmodel.py
+++ b/electrum/gui/qml/qeaddresslistmodel.py
@@ -131,6 +131,7 @@ class QEAddressCoinListModel(QAbstractListModel, QtEventListener):
         self.initModel()
 
     def on_destroy(self):
+        print(f"heyheyhey. QEAddressCoinListModel.on_destroy() running")
         self.unregister_callbacks()
 
     @qt_event_listener

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The lambdas were basically everywhere, as (years ago) I noticed without the lambda the on_destroy method would not be called in all cases, so using lambdas as default solution seemed to solve that issue.

While working on the boilerplate to test QObjects, the strong-ref issue with lambdas became apparent.

What I understood to be the case when diving deeper, is that the python destroyed signal handler gets called when the C++ object is deleted before the python wrapper (e.g. due to a Qt parent deleting its children on destruction) , and it will fail if the wrapper is deleted first (e.g. no strong refs left on the wrapper)

So the lambda style signal handler will

  • always trigger on_destroy
  • prohibit the wrapper from being garbage collected

And the class method signal handler will

  • trigger on_destroy only if C++ object gets deleted first
  • garbage collect the wrapper


QEConfig.instance.freezeReusedAddressUtxosChanged.connect(lambda: self.setDirty())
QEConfig.instance.freezeReusedAddressUtxosChanged.connect(self.setDirty)

self._dirty = True
self.initModel()
Expand Down
2 changes: 1 addition & 1 deletion electrum/gui/qml/qechanneldetails.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def __init__(self, parent=None):
self._is_closing = False

self.register_callbacks()
self.destroyed.connect(lambda: self.on_destroy())
self.destroyed.connect(self.on_destroy)

@event_listener
def on_event_channel(self, wallet: 'Abstract_Wallet', channel: 'AbstractChannel'):
Expand Down
2 changes: 1 addition & 1 deletion electrum/gui/qml/qechannellistmodel.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def __init__(self, wallet: 'Abstract_Wallet', parent=None):
# methods of this class only, and specifically not be
# partials, lambdas or methods of subobjects. Hence...
self.register_callbacks()
self.destroyed.connect(lambda: self.on_destroy())
self.destroyed.connect(self.on_destroy)

@qt_event_listener
def on_event_channel(self, wallet, channel):
Expand Down
2 changes: 1 addition & 1 deletion electrum/gui/qml/qefx.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def __init__(self, fxthread: FxThread, config: SimpleConfig, parent=None):
self.fx = fxthread
self.config = config
self.register_callbacks()
self.destroyed.connect(lambda: self.on_destroy())
self.destroyed.connect(self.on_destroy)

def on_destroy(self):
self.unregister_callbacks()
Expand Down
2 changes: 1 addition & 1 deletion electrum/gui/qml/qeinvoice.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def __init__(self, parent=None):
self._updating_max = False

self.register_callbacks()
self.destroyed.connect(lambda: self.on_destroy())
self.destroyed.connect(self.on_destroy)

def on_destroy(self):
self.unregister_callbacks()
Expand Down
4 changes: 2 additions & 2 deletions electrum/gui/qml/qeinvoicelistmodel.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ class QEInvoiceListModel(QEAbstractInvoiceListModel, QtEventListener):
def __init__(self, wallet, parent=None):
super().__init__(wallet, parent)
self.register_callbacks()
self.destroyed.connect(lambda: self.on_destroy())
self.destroyed.connect(self.on_destroy)

_logger = get_logger(__name__)

Expand Down Expand Up @@ -217,7 +217,7 @@ class QERequestListModel(QEAbstractInvoiceListModel, QtEventListener):
def __init__(self, wallet, parent=None):
super().__init__(wallet, parent)
self.register_callbacks()
self.destroyed.connect(lambda: self.on_destroy())
self.destroyed.connect(self.on_destroy)

_logger = get_logger(__name__)

Expand Down
2 changes: 1 addition & 1 deletion electrum/gui/qml/qenetwork.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def __init__(self, network: 'Network', parent=None):
self._height = network.get_local_height() # init here, update event can take a while
self._server_height = network.get_server_height() # init here, update event can take a while
self.register_callbacks()
self.destroyed.connect(lambda: self.on_destroy())
self.destroyed.connect(self.on_destroy)

QEConfig.instance.useGossipChanged.connect(self.on_gossip_setting_changed)

Expand Down
10 changes: 6 additions & 4 deletions electrum/gui/qml/qeqrscanner.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import os

from PyQt6.QtCore import pyqtProperty, pyqtSignal, pyqtSlot, QObject, Qt
from PyQt6.QtCore import pyqtProperty, pyqtSignal, pyqtSlot, QObject, Qt, QMetaObject
from PyQt6.QtGui import QGuiApplication

from electrum.gui.qml.qetypes import QEBytes
Expand All @@ -25,13 +25,11 @@ class QEQRScanner(QObject):

foundText = pyqtSignal(str)
foundBinary = pyqtSignal(QEBytes)

finished = pyqtSignal()

def __init__(self, parent=None):
super().__init__(parent)
self._hint = _("Scan a QR code.")
self.finished.connect(self._unbind, Qt.ConnectionType.QueuedConnection)

self.destroyed.connect(lambda: self.on_destroy())

Expand Down Expand Up @@ -78,6 +76,11 @@ def on_qr_activity_result(self, requestCode, resultCode, intent):
send_exception_to_crash_reporter(e)
finally:
self.finished.emit()
self.unbind()

def unbind(self):
# submit unbind request queued
QMetaObject.invokeMethod(self, '_unbind', Qt.ConnectionType.QueuedConnection)

@pyqtSlot()
def _unbind(self):
Expand All @@ -88,4 +91,3 @@ def _scan_qr_non_android(self):
data = QGuiApplication.clipboard().text()
self.foundText.emit(data)
self.finished.emit()
return
2 changes: 1 addition & 1 deletion electrum/gui/qml/qeserverlistmodel.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def __init__(self, network, parent=None):
self.network = network
self.initModel()
self.register_callbacks()
self.destroyed.connect(lambda: self.unregister_callbacks())
self.destroyed.connect(self.unregister_callbacks)

@qt_event_listener
def on_event_network_updated(self):
Expand Down
14 changes: 8 additions & 6 deletions electrum/gui/qml/qeswaphelper.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from typing import Union, Optional, TYPE_CHECKING, Sequence

from PyQt6.QtCore import (pyqtProperty, pyqtSignal, pyqtSlot, QObject, QTimer, pyqtEnum, QAbstractListModel, Qt,
QModelIndex, QVariant)
QModelIndex, QVariant, QMetaObject)
from PyQt6.QtGui import QColor

from electrum.i18n import _
Expand Down Expand Up @@ -154,7 +154,6 @@ class State(IntEnum):
error = pyqtSignal([str], arguments=['message'])
undefinedNPub = pyqtSignal()
offersUpdated = pyqtSignal()
requestTxUpdate = pyqtSignal()

def __init__(self, parent=None):
super().__init__(parent)
Expand Down Expand Up @@ -192,7 +191,6 @@ def __init__(self, parent=None):
self._fwd_swap_updatetx_timer = QTimer(self)
self._fwd_swap_updatetx_timer.setSingleShot(True)
self._fwd_swap_updatetx_timer.timeout.connect(self.fwd_swap_updatetx)
self.requestTxUpdate.connect(self.tx_update_pushback_timer)

self.offersUpdated.connect(self.on_offers_updated)
self.transport_task: Optional[asyncio.Task] = None
Expand Down Expand Up @@ -606,10 +604,14 @@ def swap_slider_moved(self):
else:
# update tx only if slider isn't moved for a while
self.valid = False
# trigger tx_update_pushback_timer through signal, as this might be called from other thread
self.requestTxUpdate.emit()
self.requestTxUpdate()

def tx_update_pushback_timer(self):
def requestTxUpdate(self):
# trigger _tx_update_pushback_timer from qt thread, as this might be called from other thread
QMetaObject.invokeMethod(self, '_tx_update_pushback_timer', Qt.ConnectionType.QueuedConnection)

@pyqtSlot()
def _tx_update_pushback_timer(self):
self._fwd_swap_updatetx_timer.start(250)

def check_valid(self, send_amount, receive_amount):
Expand Down
11 changes: 6 additions & 5 deletions electrum/gui/qml/qetransactionlistmodel.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from datetime import datetime, timedelta
from typing import TYPE_CHECKING, Dict, Any

from PyQt6.QtCore import pyqtProperty, pyqtSignal, pyqtSlot
from PyQt6.QtCore import pyqtProperty, pyqtSignal, pyqtSlot, QMetaObject
from PyQt6.QtCore import Qt, QAbstractListModel, QModelIndex

from electrum.logging import get_logger
Expand All @@ -27,8 +27,6 @@ class QETransactionListModel(QAbstractListModel, QtEventListener):
_ROLE_MAP = dict(zip(_ROLE_KEYS, [bytearray(x.encode()) for x in _ROLE_NAMES]))
_ROLE_RMAP = dict(zip(_ROLE_NAMES, _ROLE_KEYS))

requestRefresh = pyqtSignal()

def __init__(self, wallet: 'Abstract_Wallet', parent=None, *, onchain_domain=None, include_lightning=True):
super().__init__(parent)
self.wallet = wallet
Expand All @@ -38,8 +36,7 @@ def __init__(self, wallet: 'Abstract_Wallet', parent=None, *, onchain_domain=Non
self.tx_history = []

self.register_callbacks()
self.destroyed.connect(lambda: self.on_destroy())
self.requestRefresh.connect(lambda: self.initModel())
self.destroyed.connect(self.on_destroy)

self._dirty = True
self.initModel()
Expand Down Expand Up @@ -87,6 +84,10 @@ def on_event_labels_received(self, wallet, labels):
if wallet == self.wallet:
self.initModel(True) # TODO: be less dramatic

def requestRefresh(self):
# ensure execute on qt thread
QMetaObject.invokeMethod(self, 'initModel', Qt.ConnectionType.QueuedConnection)

def rowCount(self, index):
return len(self.tx_history)

Expand Down
86 changes: 30 additions & 56 deletions electrum/gui/qml/qetxdetails.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ class QETxDetails(QObject, QtEventListener):

def __init__(self, parent=None):
super().__init__(parent)
self.register_callbacks()
self.destroyed.connect(lambda: self.on_destroy())

self._wallet = None # type: Optional[QEWallet]
self._txid = ''
Expand Down Expand Up @@ -73,6 +71,9 @@ def __init__(self, parent=None):
self._header_hash = ''
self._short_id = ""

self.register_callbacks()
self.destroyed.connect(self.on_destroy)

def on_destroy(self):
self.unregister_callbacks()

Expand Down Expand Up @@ -407,6 +408,23 @@ def update_mined_status(self, tx_mined_info: TxMinedInfo):
self._header_hash = tx_mined_info.header_hash
self._short_id = tx_mined_info.short_id() or ""

def on_sign_success(self):
self._logger.debug('on_sign_success')
self.update()

def on_sign_failed(self, failure):
self._logger.debug('on_sign_failed')

def on_broadcast_success(self, *args):
self._logger.debug('on_broadcast_success')
self._can_broadcast = False
self.detailsChanged.emit()

def on_broadcast_failed(self, *args):
self._logger.debug('on_broadcast_failed')
self._can_broadcast = True
self.detailsChanged.emit()

@pyqtSlot()
def signAndBroadcast(self):
self._sign(broadcast=True)
Expand All @@ -416,69 +434,25 @@ def sign(self):
self._sign(broadcast=False)

def _sign(self, broadcast):
# TODO: connecting/disconnecting signal handlers here is hmm
try:
if broadcast:
self._wallet.broadcastSucceeded.disconnect(self.onBroadcastSucceeded)
self._wallet.broadcastFailed.disconnect(self.onBroadcastFailed)
except Exception:
pass

if broadcast:
self._wallet.broadcastSucceeded.connect(self.onBroadcastSucceeded)
self._wallet.broadcastFailed.connect(self.onBroadcastFailed)
self._wallet.sign_and_broadcast(self._tx, on_success=self.on_signed_tx)
else:
self._wallet.sign(self._tx, on_success=self.on_signed_tx)

# side-effect: signing updates self._tx
# we rely on this for broadcast
def on_sign_success():
self._logger.debug('on_sign_success, broadcasting')
self.on_sign_success() # indicate sign success
if self._tx.is_complete():
self._wallet.broadcast(self._tx, on_success=self.on_broadcast_success, on_failure=self.on_broadcast_failed)
else:
self._logger.warning('tx not complete, not broadcasting')

def on_signed_tx(self, tx: Transaction):
self._logger.debug('on_signed_tx')
self.update()
sign_success_cb = on_sign_success if broadcast else self.on_sign_success
self._wallet.sign(self._tx, on_success=sign_success_cb, on_failure=self.on_sign_failed)

@pyqtSlot()
def broadcast(self):
assert self._tx.is_complete()

try:
self._wallet.broadcastFailed.disconnect(self.onBroadcastFailed)
except Exception:
pass
self._wallet.broadcastFailed.connect(self.onBroadcastFailed)

self._can_broadcast = False
self.detailsChanged.emit()

self._wallet.broadcast(self._tx)

@pyqtSlot(str)
def onBroadcastSucceeded(self, txid):
if txid != self._txid:
return

self._logger.debug('onBroadcastSucceeded')
try:
self._wallet.broadcastSucceeded.disconnect(self.onBroadcastSucceeded)
except Exception:
pass

self._can_broadcast = False
self.detailsChanged.emit()

@pyqtSlot(str, str, str)
def onBroadcastFailed(self, txid, code, reason):
if txid != self._txid:
return

try:
self._wallet.broadcastFailed.disconnect(self.onBroadcastFailed)
except Exception:
pass

self._can_broadcast = True
self.detailsChanged.emit()
self._wallet.broadcast(self._tx, on_success=self.on_broadcast_success, on_failure=self.on_broadcast_failed)

@pyqtSlot()
@pyqtSlot(bool)
Expand Down
8 changes: 4 additions & 4 deletions electrum/gui/qml/qetxfinalizer.py
Original file line number Diff line number Diff line change
Expand Up @@ -572,15 +572,15 @@ def sign(self):

self._wallet.sign(self._tx, on_success=partial(self.on_signed_tx, True), on_failure=self.on_sign_failed)

def on_signed_tx(self, save: bool, tx: Transaction):
def on_signed_tx(self, save: bool):
self._logger.debug('on_signed_tx')
saved = False
if save and self._tx.txid():
if self._wallet.save_tx(self._tx):
saved = True
else:
self._logger.error('Could not save tx')
self.finished.emit(True, saved, tx.is_complete())
self.finished.emit(True, saved, self._tx.is_complete())

def on_sign_failed(self, msg: str = None):
self._logger.debug('on_sign_failed')
Expand Down Expand Up @@ -612,7 +612,7 @@ def __init__(self, parent=None):
self._txid = ''

self.register_callbacks()
self.destroyed.connect(lambda: self.on_destroy())
self.destroyed.connect(self.on_destroy)

def on_destroy(self):
self.unregister_callbacks()
Expand Down Expand Up @@ -1210,7 +1210,7 @@ def update(self):
self._valid = True
self.validChanged.emit()

self.on_signed_tx(False, tx)
self.on_signed_tx(False)

@pyqtSlot()
def send(self):
Expand Down
Loading