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

I fear we messed up #73

Closed
ueen opened this issue Apr 20, 2021 · 11 comments
Closed

I fear we messed up #73

ueen opened this issue Apr 20, 2021 · 11 comments
Labels
bug Something isn't working

Comments

@ueen
Copy link
Contributor

ueen commented Apr 20, 2021

After the recent update with the js injection right into the original snapdrop.net i often encounter the app in a state where it is just displaying the page without the js injections, which of course messes up functionality like copying.
I suspect that it may have something to do with loadAgain it seems somewhat hacky to me, could you explain why you implemented it and if it could be removed?

This often appears when started new (onCreate) and i'm sometimes able to fix it by moving the app to the background and then to the foreground again (onResume).

@ueen ueen added the bug Something isn't working label Apr 20, 2021
@ueen
Copy link
Contributor Author

ueen commented Apr 20, 2021

#74

@fm-sys
Copy link
Owner

fm-sys commented Apr 20, 2021

I suspect that it may have something to do with loadAgain it seems somewhat hacky to me, could you explain why you implemented it and if it could be removed?

For sure this is a very hacky workaround. Actually I tried to remove that workaround before releasing the new version but somehow the app shows itself as peer which is gone after a website reload. But I can try to debug into it again...

@ueen
Copy link
Contributor Author

ueen commented Apr 20, 2021

The PR fixes the injection isssue.
Yea the showing itself seem to be due to this code

if (onlinePastThreeMin()) {
            WebStorage.getInstance().deleteAllData();

            cookieManager.setCookie("https://snapdrop.net/",
                    "peerid=" + UUID.randomUUID().toString() + ";" +
                            "path=/server;" +
                            "max-age=86400;"
            );
        }

if removed it doesnt show itself anymore...well sometimes it does if you quit and reopen fast...but only once - with this code sometimes the app is creating 3 peers or more
Is this code requiered for anything else?

@fm-sys
Copy link
Owner

fm-sys commented Apr 20, 2021

Is this code requiered for anything else?

It was required when using my github fork, but probably not when using the original website. Thanks for finding the fix 😃👍

I will try your PR and report back...

@ueen
Copy link
Contributor Author

ueen commented Apr 20, 2021

I have to say it seems no different if this code is remved or not and i also encountered the app showing itself sometimes with loadAgain and the cookies so i think we can remove that code, then at least the js injection works, but the showing itself issue remains...let me see if i find something int the snapdrop repo

@ueen
Copy link
Contributor Author

ueen commented Apr 20, 2021

here it is RobinLinus/snapdrop#215
i'm not sure though how to interpret this...maybe we need to send the goodbye to the server before when the app i closed for good?

@fm-sys
Copy link
Owner

fm-sys commented Apr 20, 2021

I tried to do that in the onDestroy method via leaving the website which works in most cases rather well

@ueen
Copy link
Contributor Author

ueen commented Apr 20, 2021

yea i see, i feel like that doesnt work at all, bacause onDestroy isnt always called...

@ueen
Copy link
Contributor Author

ueen commented Apr 20, 2021

Maybe this is only an issue if the app is force closed and reopened, which should be rare under real life conditions, i'll test some more, but i think removing the cookie and loadAgain is a good idea either way, because they do not fully prevent the app from showing itself and add other bugs.

EDIT: yea that seems to be the case, there are no duplicates if the app is

  • in the background long enough
  • the app is closed and opened after a few minutes (2-3)

so you can safely merge :)

@ueen ueen closed this as completed Apr 20, 2021
@fm-sys
Copy link
Owner

fm-sys commented Apr 20, 2021

yea i see, i feel like that doesnt work at all, bacause onDestroy isnt always called...

Yes, when reinstalling at the emulator it doesn't get called, but under real life conditions it's mostly fine.

BTW, I really appreciate your continuous work for Snapdrop for Android - reduce the code/maintain complexity and making it better from user perspective. Thanks for your help!

@ueen
Copy link
Contributor Author

ueen commented Apr 20, 2021

sure no problem, this was really bugging me :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants