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

qml qt6: use ZXing-based camera #2

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

SomberNight
Copy link

This builds on top of spesmilo#8545, to use the old ZXing-based camera code for QR code scanning on Android, instead of QtMultimedia, as the latter seems buggy in Qt6.4.

The p4a branch needs this commit:
SomberNight/python-for-android@f2741f0

TODO:

  • make scanner_layout.xml look reasonable
  • how to avoid hardcoding testnet/mainnet in SimpleScannerActivity.java in the imports?
  • test all scanning code paths

@SomberNight
Copy link
Author

Hmm, opened PR against your repo instead of spesmilo sort of accidentally. But anyway, at least the diff is clearer this way.

@@ -45,7 +45,7 @@ info "apk building phase starts."
# FIXME: changing "APP_PACKAGE_NAME" seems to require a clean rebuild of ".buildozer/",
# to avoid that, maybe change "APP_PACKAGE_DOMAIN" instead.
# So, in particular, to build a testnet apk, simply uncomment:
#export APP_PACKAGE_DOMAIN=org.electrum.testnet
export APP_PACKAGE_DOMAIN=org.electrum.testnet
Copy link
Author

Choose a reason for hiding this comment

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

(temporary)

import com.google.zxing.Result;
import com.google.zxing.BarcodeFormat;

import org.electrum.testnet.electrum.R; // TODO
Copy link
Author

Choose a reason for hiding this comment

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

Any idea how to avoid hardcoding the package name here?
This is annoying because of https://github.com/spesmilo/electrum/blob/ac177577a62134763547ac416d1098b8bf5f87e1/contrib/android/make_apk.sh#L48

} else {
invoiceParser.recipient = data
}
//scanner.destroy() // TODO
Copy link
Author

Choose a reason for hiding this comment

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

Do we need an explicit destroy somewhere to avoid a memleak?
I see most dialogs have onClosed: destroy(), but not sure how to mimic that.
Also see main.qml::scanDialog (line 393).

Copy link
Owner

@accumulator accumulator Oct 15, 2023

Choose a reason for hiding this comment

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

onClosed is a signal handler for Popup. Since you use a bare QObject as the base for QEQRScanner, you don't have the plumbing provided by Popup, and you need to detect when the activity is closed and emit a signal, which can then be used to destroy the instance from QML.

However, you don't really need to wrap the scanner in a Qt object and manage the lifecycle. Since there's at most 1 scanner active, you can basically also spawn the Activity from a @pyqtSlot decorated method in e.g. AppController, and just emit signals from AppController depending on the result. I think the Activity will be auto-deleted by the JVM when it's closed, and you don't need to manage any lifecycle on the QML side.

Copy link
Owner

Choose a reason for hiding this comment

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

The last part would be something like this:

Button {
    onClicked: AppController.showQRScanner()
}

Connections {
    target: AppController
    function onQRScanned(data) {
        // handle data
    }
}

Copy link
Author

Choose a reason for hiding this comment

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

you can basically also spawn the Activity from a @pyqtSlot decorated method in e.g. AppController, and just emit signals from AppController depending on the result

Yes I already had something like that a refactor prior, e.g. see
SomberNight@bc5f596
I've changed to QEQRScanner(QObject) instead as this way I can easily replace both the ScanDialog and the SendDialog use cases. In particular, QEQRScanner now has an API that is a drop-in replacement for the main.qml::scanDialog usages. The AppController signal approach worked nicely for the SendDialog, but I found it not easy to adapt to the ScanDialog usages.

you don't have the plumbing provided by Popup, and you need to detect when the activity is closed and emit a signal, which can then be used to destroy the instance from QML.
However, you don't really need to wrap the scanner in a Qt object and manage the lifecycle

But just to confirm, if we do use this QEQRScanner(QObject) approach, do we need to explicitly call destroy for the object? (/manually manage the lifecycle)
I guess I could just call destroy wherever the code on master calls close/closeDialog.

@accumulator accumulator merged commit 2b4ef4c into accumulator:qt6 Oct 23, 2023
@accumulator
Copy link
Owner

I've merged this branch, but changed the zxing qr scan method to android-only, keeping the qt6 way for desktop/testing use, so we don't have to resurrect this when trying if things have improved in qt6.6+

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.

2 participants