search

sakusaku3939 / DeepLAndroid

Unofficial Android App for DeepL Translator
MIT License
334 stars 18 forks source link

Handle scroll up correctly #64

Closed fm-sys closed 2 years ago

fm-sys commented 2 years ago

Fix glitch described in https://github.com/sakusaku3939/DeepLAndroid/issues/59#issuecomment-1008174735

fix #59

sakusaku3939 commented 2 years ago

Thank you for fixing the bug!

By the way, does this pull request include a solution to the "only close when tapping a dark area" issue?

fm-sys commented 2 years ago

Not yet. It was more difficult than I thought. Webview and Native Android scroll components are unfortunately not 100% compatible... :-/

But I've set this PR to WIP for now, maybe I can find a working solution.

sakusaku3939 commented 2 years ago

I got it. If it's going to take some time, can I release a version that doesn't include these changes? (Resolved #60 and #23)

fm-sys commented 2 years ago

can I release a version that doesn't include these changes?

Yes, go ahead! We can release a second bugfix as soon as I'm ready here...

fm-sys commented 2 years ago

Webview and Native Android scroll components are unfortunately not 100% compatible... :-/

Ooookay... I've tried many different code snippets which I found on the internet. The only NestedScrollWebView implementation which was working together with a bottom sheet is https://gist.github.com/alexmiragall/0c4c7163f7a17938518ce9794c4a5236

@sakusaku3939 Do you think we can use the code for this app?

sakusaku3939 commented 2 years ago

Hmmm... I don't know, the license is not listed. However, given that it is published on a forkable Gist, I think you can use it if you leave the copyright notice.

Can you make a commit that includes this?

fm-sys commented 2 years ago

However, given that it is published on a forkable Gist, I think you can use it if you leave the copyright notice.

From reading the Github ToS, it sounds like everybody is allowed to fork and execute uploaded code. However, we are not allowed to re-distribute code, except if the code is provided with a license which allows doing so.

Maybe it's better to create a Kotlin version "inspired" by the code provided in that gist.

fm-sys commented 2 years ago

I noticed that even the code of that gist will not work correctly for all cases. I really wonder why there really is absolutely no library which can handle this...?!

Anyway, I'm currently thinking about two possible solutions: a) I finally find a way to generally handle scrolling of child elements correctly b) We add a method to the Javascript interface to dynamically disable the popup dragability while the language selector is shown

sakusaku3939 commented 2 years ago

Oh, really? I was also thinking of idea b as a last resort, but I also thought it would be complicated to implement it. I wish there was another way...

sakusaku3939 commented 2 years ago

P.S. On an unrelated note, I restarted a pull request by mistake. I don't know how to get it back.🙃

fm-sys commented 2 years ago

P.S. On an unrelated note, I restarted a pull request by mistake. I don't know how to get it back.🙃

The draft state can be set here (if that's what you mean): Screenshot_20220115_084935

fm-sys commented 2 years ago

I'm making progress... Still not perfect, but being on a good way ;-)

fm-sys commented 2 years ago

From my side, I would be ready. To be honest, I don't fully understand the code myself, but it seems to work 😅

I'm also sure that quite some places could probably be simplified. On the other hand, never change a running system 😉

What do you think about the changes?

sakusaku3939 commented 2 years ago

Good work!

I've checked, and it sometimes doesn't work correctly when scrolling when the language selector is closed. However, this is minor compared to the bug where the language itself is not scrollable, so I think we can merge these. Screenshot_20220117-181211

Also, I think it's possible to make Issues for this bug and fix it later.

Thank you so much! 👍