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

Show paste button in qr scanner dialog #4309

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

nicodh
Copy link
Contributor

@nicodh nicodh commented Nov 2, 2024

resolves #3936

move "paste from clipboard" from settings context menu to own button

image

Another option would be: name the paste button "Import" and show the context menu onClick with the 2 options: 'load QR Code image' or 'paste from clipboard'
But buttons that show a context menu on click are a bit unusual

@nicodh nicodh requested review from r10s and WofWca November 2, 2024 08:38
@nicodh
Copy link
Contributor Author

nicodh commented Nov 2, 2024

TODO: We have to adapt the e2e test for pasting the qr code! #4285

Copy link
Collaborator

@WofWca WofWca left a comment

Choose a reason for hiding this comment

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

This removes the "paste from clipboard" menu item from QrReader, but QrReader is also used in ReceiveBackupDialog. Does it mean it's now impossible to paste the backup QR?

packages/frontend/src/components/dialogs/QrCode.tsx Outdated Show resolved Hide resolved
@r10s
Copy link
Member

r10s commented Nov 2, 2024

Another option would be: name the paste button "Import" and show the context menu onClick with the 2 options: 'load QR Code image' or 'paste from clipboard'
But buttons that show a context menu on click are a bit unusual

let's stay with the paste button.

having that hidden at "import" again is not obvious, if is also about the balance copy/paste. there could be a three dot menu beside the paste button for the other options, but that is another effort, and maybe not needed.

@nicodh
Copy link
Contributor Author

nicodh commented Nov 2, 2024

This removes the "paste from clipboard" menu item from QrReader, but QrReader is also used in ReceiveBackupDialog. Does it mean it's now impossible to paste the backup QR?

True, that's another use case not covered in the Issue since the left footer button is already set there. So maybe we should move the Troubleshoot link to another place? @r10s

image

@nicodh nicodh marked this pull request as draft November 2, 2024 09:44
@WofWca
Copy link
Collaborator

WofWca commented Nov 2, 2024

A simple hack would be to keep the "paste from clipboard" menu item just for the Backup dialog. I.e. to add a prop hidePasteFromClipboardMenuOption to QrReader.

@Simon-Laux
Copy link
Member

Simon-Laux commented Nov 2, 2024

I'd say keep the paste button in the menu or move trouble shooting up to the description.
BTW: I think we can remove the experimental label and version requirement text

@r10s
Copy link
Member

r10s commented Nov 2, 2024

let's keep the menu as is, and only add the button. that seems to be the easiest approach to fix the issue at hand.

for the "experimental" hint: we're staying with that for so long, also for reasons, regarding backup/backup2 hickup. let's do not remove that last minute, it is fine. idk, what the exact state of iroh is, let's see that for 1.50 and maybe get it out of "experimental" on all platforms

@Simon-Laux
Copy link
Member

also for reasons, regarding backup/backup2 hickup

wouldn't we need to upgrade the version then or is the first iroh/backup(without the 2) still supported?

@r10s
Copy link
Member

r10s commented Nov 2, 2024

wouldn't we need to upgrade the version then or is the first iroh/backup(without the 2) still supported?

i think it is fine, as long as desktop checks for DC_QR_BACKUP as well as DC_QR_BACKUP2 at all places, on android we forgot one place, and that resulted in a degradation, see deltachat/deltachat-android#3370

resolves #3936

move paste from clipboard from settings context menu to own button
Enables to trigger the handlePasteFromClipboard in QrReader child
component
@nicodh
Copy link
Contributor Author

nicodh commented Nov 5, 2024

I readded the "paste from clipboard" item to settings menu

I also refactored the implementation to avoid duplicate code by using useImperativeHandle
I also removed some existing code - see comment above

@nicodh
Copy link
Contributor Author

nicodh commented Nov 5, 2024

TODO: We have to adapt the e2e test for pasting the qr code! #4285

Done

@nicodh nicodh marked this pull request as ready for review November 5, 2024 18:39
Copy link
Collaborator

@WofWca WofWca left a comment

Choose a reason for hiding this comment

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

With my non-nitpick comments addressed or dismissed, it's good to merge.

I tested it a bit, and looked at the code. It seems fine otherwise.
Though NGL, it's a bit hard to review, with refactoring changes and functional changes being mixed.

packages/frontend/src/components/dialogs/QrCode.tsx Outdated Show resolved Hide resolved
packages/frontend/src/components/QrReader/index.tsx Outdated Show resolved Hide resolved
packages/frontend/src/components/QrReader/index.tsx Outdated Show resolved Hide resolved
packages/frontend/src/components/QrReader/index.tsx Outdated Show resolved Hide resolved
@nicodh
Copy link
Contributor Author

nicodh commented Nov 6, 2024

@r10s since I refactor this now anyway: when do we show the red alert (in our code is named "userFeedback") and when a normal warning dialog?
Since when a dialog is opened, the red message is behind the backdrop which is not so nice:

image

the "AlertDialog" would look like that:

image

which is not really what I would expect from a "AlertDialog" (but that is probably another issue)

@nicodh
Copy link
Contributor Author

nicodh commented Nov 6, 2024

When scanning a qr code for backup import there are even 2 backdrops over the user feedback, so it is almost invisible

image

@nicodh nicodh marked this pull request as draft November 6, 2024 16:12
@WofWca
Copy link
Collaborator

WofWca commented Nov 6, 2024

FYI that user-feedback toast is ancient. Last time it was changed was in 2019: b0134fe.

@nicodh
Copy link
Contributor Author

nicodh commented Nov 6, 2024

FYI that user-feedback toast is ancient. Last time it was changed was in 2019: b0134fe.

So maybe better improve the AlertDialog a bit and use that...

@nicodh nicodh marked this pull request as ready for review November 7, 2024 08:15
Copy link
Collaborator

@WofWca WofWca left a comment

Choose a reason for hiding this comment

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

So, there are a lot of changes in this MR, both functional and refactoring. Not all are strictly related to the title of the MR,

I have verified that the moved fileToBase64 and base64ToImageData functions are unchanged. qrCodeFromClipboard was moved from handlePasteFromClipboard and it's also largely unchanged. The only thing different is that it errors out when the scan result's string is empty, which probably makes sense (but I'm not sure if this can happen).

About the removal of setError(true) from handleError, and the rename of it to setCameraAccessError. I haven't checked what it does exactly.

But what I noticed is that we now handle all scanning errors in one place, in the parent component, which is probably good. But we also invoke the callback after an error is thrown in worker.postMessage(imageData), and postMessage gets called every 250ms. But I guess this can't happen often.

Another things is as was said above, now an AlertDialog is used instead of the userFeedback toast, which is better (at least in common cases).

I have tested this MR a little bit. Works alright.

As to the rest of the changes (that is the "add 'paste' button"): they look good, apart from that one suggestion I left!

packages/frontend/src/components/dialogs/QrCode.tsx Outdated Show resolved Hide resolved
@nicodh
Copy link
Contributor Author

nicodh commented Nov 7, 2024

setCameraAccessError

this error is set if startStream fails so I named it a bit more meaningful

I think we won't merge this into the current release (which will be published tomorrow hopefully)

@@ -158,47 +82,18 @@ export default function QrReader({ onError, onScan }: Props) {
} else {
onError(error.toString())
}

setError(true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this error is set if startStream fails so I named it a bit more meaningful

But it was also removed from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but that resulted in showing the camera_access_failed error in the UI which I consider wrong since it's a generic errorHandler (for example after an error in handleFileInputChange)

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.

placement of "Paste" in "Scan QR code" not obvious
4 participants