search

status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.9k stars 988 forks source link

Chat's QRCode scanner dont exits after scan #9381

Closed 3esmit closed 4 years ago

3esmit commented 4 years ago

Bug Report

Problem

When a qrcode is scanned it brings to result screen, but it is kept open in background, when tapping back it shows the camera open, however it wont work for scanning qr codes anymore.

Expected behavior

After scan it should close camera and scanner.

Actual behavior

After scan camera is kept open in background

Reproduction

  1. Open Status

  2. Open QR code scanner from Chat Tab

  3. a) scan a contact code: image

  4. b) scan an EIP 681 link image

  5. Tap back (in top left corner)

Additional Information

gitcoinbot commented 4 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 1000.0 SNT (40.55 USD @ $0.04/SNT) attached to it as part of the status-im fund.

gitcoinbot commented 4 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 2 weeks, 1 day from now. Please review their action plans below:

1) acolytec3 has been approved to start work.

Fix along with additional work already being done on QR scanner coffee.

Learn more on the Gitcoin Issue Details page.

gitcoinbot commented 4 years ago

@acolytec3 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

acolytec3 commented 4 years ago

@gitcoinbot Yes, still working on it.

gitcoinbot commented 4 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 1000.0 SNT (13.71 USD @ $0.01/SNT) has been submitted by:

  1. @acolytec3

@3esmit please take a look at the submitted work:

3esmit commented 4 years ago

Still happens in Commit: 1cdfff0

3esmit commented 4 years ago

I notice that, when using the Send button from the wallet, it works fine.

How to reproduce:

  1. Open Status
  2. (From the Chats Tab) tap on the + plus button
  3. Tap on Scan QR code
  4. It dont works.
acolytec3 commented 4 years ago

Hmm, l'll take another look. It always navigates back to the send transaction screen for me.

On Mon, Nov 11, 2019, 4:57 PM Ricardo Guilherme Schmidt < notifications@github.com> wrote:

Still happens in Commit: 1cdfff0 https://github.com/status-im/status-react/commit/1cdfff01d06661682eb3a1630016349c23dafb8e

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/status-im/status-react/issues/9381?email_source=notifications&email_token=AEENFXBKQBRCHN7UTPSKWMLQTHIK5A5CNFSM4JILDP22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDYISOA#issuecomment-552634680, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEENFXFYKDIVCDMIGQIF4ADQTHIK5ANCNFSM4JILDP2Q .

3esmit commented 4 years ago

It is kept open in background, when you go to chat tab there is open a qr code scanner with camera on, but not actually working as scanner.

How to reproduce:

  1. Open Status
  2. (From the Chats Tab) tap on the + plus button
  3. Tap on Scan QR code
  4. Scan some code
  5. Tap back on the transaction screen
  6. Tap Chat to navigate to chat screen
  7. QR code scanner is open with camera but non functional

Expected that in 7. the chat screen appears, and that the QR code scanner is closed in step 4.

acolytec3 commented 4 years ago

Thanks. I'm not about to recreate and will push a fix tonight or tomorrow.

acolytec3 commented 4 years ago

@3esmit @yenda Now that I've played with this a little bit, I think this is just one symptom of a potentially bigger issue. Basically, what's going on is that every time you navigate between any of the 4 top-level menu items (chat/wallet/settings/dapps), the navigate-to event is firing and going to the last view in the navigation-stack for that top-level menu item. In this particular scenario, the last item for chat is the :qr-scanner. @yenda , is this a known behavior and would you consider it an issue? If not, I can probably do some special handling just for the chat stack where we pop the :qr-scanner event so it instead navigates back to the top-level chat window.

yenda commented 4 years ago

@3esmit are you trying to scan EIP681 or status contact codes? Does scanning contact codes work? Because I think the camera in the Chat screen is for adding chats only, so it won't work for anything else by design. (could be changed though)

ok tried by myself and it's indeed scanning EIP681 but not when you go back. I think that there by need to be a navigation-reset here instead of navigate to

3esmit commented 4 years ago

@yenda It seems to detect the transaction link when scanned from chat screen, but it don't works because of #9380

I see that is out of context, but if scanner detects that is a transaction request, why error, instead of shortcut.

If the support of transaction request in chat qr code scanner is a bug, we might need to open an issue to remove it, and set #9380 as invalid.

acolytec3 commented 4 years ago

@yenda @3esmit I could see it going either way in terms of should you allow the chat link to QR process eip681 links . Should I just push a commit doing that navigate -reset approach so that you won't land back in the QR scanner when going back to chat and call it done there?

3esmit commented 4 years ago

Yes, this is a bug regardless of the supporting of EIP681 in chat qrcode scanner scanner, as happens when scanning contact codes aswell.

acolytec3 commented 4 years ago

@yenda @3esmit I'm going to work this one in a separate PR since it doesn't seem to be really related to the work on #9240.

@yenda to resolve this, it seems like firing navigate-reset isn't the right approach since this actually grabs the navigation as part of the process and I think all we really want to do is pop the :qr-scanner spot off the navigation-stack so it doesn't navigate back to the qr-scanner the next time the user taps on the chat item. My thought was to update the set-qr-code effect to something like below (mirroring navigate-back minus setting the view-id though Clojure is complaining about spec errors on the navigation-stack when this event is fired. Any suggestions?

(fx/defn set-qr-code
  [{:keys [db]} context data]
  (let [[navigation-stack'] (pop (get-in db [:navigation-stack]))]
    (merge {:db (-> db
                    (update :qr-codes dissoc context)
                    (dissoc :current-qr-context)
                    (assoc :navigation-stack navigation-stack'))}
           (when-let [qr-codes (get-in db [:qr-codes context])]
             {:dispatch [(:handler qr-codes) context data (dissoc qr-codes :handler)]}))))
acolytec3 commented 4 years ago

@yenda Fixed the spec error by changing the assoc to update when updating the navigation stack. That said, it hasn't solved the problem. I modified the code slightly to just build a brand new navigation stack per below (let [[navigation-stack']({:chat-stack :home})] and I verified in re-frisk that the :qr-scanner keyword is being removed from the stack after I successfully scan a QR code and navigate to the target page. That said, once I tap "chat" again from the main bottom navigation, instead of navigating to :chat-stack :home, it's going back to the qr-scanner. Is there another place that I should be updating the navigation-stack? I don't see any other places in the app-db where it's being set.

yenda commented 4 years ago

I'll try to look tomorrow but you cannot just alter the navigation-stack like that. There is also a stack in react-navigation as well and they need to stay synced. That's why I was suggesting navigate-reset which let's you define a new stack for both

acolytec3 commented 4 years ago

That makes sense. I saw some references in the code to the react navigation stack so will look at that as an alternative to what I have now.

On Sat, Nov 16, 2019, 10:16 PM yenda notifications@github.com wrote:

I'll try to look tomorrow but you cannot just alter the navigation-stack like that. There is also a stack in react-navigation as well and they need to stay synced. That's why I was suggesting navigate-reset which let's you define a new stack for both

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/status-im/status-react/issues/9381?email_source=notifications&email_token=AEENFXGBP7EUUGHOSF6FBFDQUCZQ3A5CNFSM4JILDP22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEIAR7I#issuecomment-554699005, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEENFXAXHGYKOY54UB5NRY3QUCZQ3ANCNFSM4JILDP2Q .

acolytec3 commented 4 years ago

@3esmit #9476 fixes this now. @yenda Thanks for the guidance on the react-native navigation-stack updates. I found the navigate-reset utility method that does what is needed and updated my PR accordingly.

gitcoinbot commented 4 years ago
Bug Squasher ⚡️ A *Bug Squasher* Kudos has been sent to @acolytec3 for this issue from @3esmit. ⚡️ Nice work @acolytec3! Your Kudos has automatically been sent in the ETH address we have on file.
gitcoinbot commented 4 years ago

⚡️ A tip worth 1000.00000 SNT (12.91 USD @ $0.01/SNT) has been granted to @acolytec3 for this issue from @3esmit. ⚡️

Nice work @acolytec3! Your tip has automatically been deposited in the ETH address we have on file.

gitcoinbot commented 4 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 1000.0 SNT (12.91 USD @ $0.01/SNT) attached to this issue has been approved & issued to @acolytec3.

Additional Tips for this Bounty:

flexsurfer commented 4 years ago

will be fixed here https://github.com/status-im/status-react/pull/9499

acolytec3 commented 4 years ago

Good by me. Thanks all!