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

Add a gesture function to the scrollview to insert the zoom-in out function. #873

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

saechimdaeki
Copy link
Contributor

Add a gesture function to the scroll view to insert the zoom-in out function.

Add a gesture function to the scroll view to insert the zoom-in out function.
@saechimdaeki
Copy link
Contributor Author

saechimdaeki commented Nov 17, 2019

This pull_request is an attempt to make this app more convenient. #854

@tcitworld
Copy link
Member

What's android-zoom-view.jar ? https://github.com/Polidea/android-zoom-view ?

@saechimdaeki
Copy link
Contributor Author

saechimdaeki commented Nov 17, 2019

@tcitworld https://code.google.com/archive/p/android-zoom-view/downloads
I did various tests using this library, but I didn't use it in the end. I forgot to delete it. I'm sorry. i will remove that

@saechimdaeki
Copy link
Contributor Author

Unnecessary libraries have been removed.

@di72nn
Copy link
Member

di72nn commented Nov 17, 2019

It's not in the build.gradle, but the jar is still there.

@saechimdaeki
Copy link
Contributor Author

@di72nn Sorry, I forgot to find it in Branch when I did it on another computer. It has now been deleted.

Copy link
Member

@di72nn di72nn left a comment

Choose a reason for hiding this comment

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

I'm sorry, but code-wise it's a mess.

I can give some pointers on how to clean this up, but I don't think we need such a feature in its current form (scaling a ScrollView).

I'd like to hear somebody else's opinion on whether we can use these "scale gestures" in ReadArticleActivity for something.

@saechimdaeki
Copy link
Contributor Author

I usually watch game community sites often.But the files on the site sometimes look too small, so I added them because I needed an extension. I'm sorry for the mess.

@di72nn
Copy link
Member

di72nn commented Nov 19, 2019

So you only need to zoom in on some particular details, not for a whole article?
Maybe it would be better to implement this as a temporary zoom that snaps back to the normal size as soon as you let it go?

@saechimdaeki
Copy link
Contributor Author

Then shall I add it as you say? If that's better and you don't mind.

@di72nn
Copy link
Member

di72nn commented Nov 19, 2019

@tcitworld @Strubbl @NWuensche do you think we need such a feature?

It seems the implementation can be pretty compact, so I have no strong feelings one way or the other.

@saechimdaeki
Copy link
Contributor Author

If you guys say it's okay, I'll try.If not, I won't. Tell me without reserve.

@Strubbl
Copy link
Contributor

Strubbl commented Nov 19, 2019

I like the idea of the feature since i have some articles, which have graphics, where i cannot zoom in from wallabag. So i go to the original website and look for the pic.

Maybe it would be better to implement this as a temporary zoom that snaps back to the normal size as soon as you let it go?

This sounds like a solution to the described problem.

@@ -154,37 +153,58 @@ public void onCreate(Bundle savedInstanceState) {

settings = App.getInstance().getSettings();

if(settings.isFullscreenArticleView()) {
if (settings.isFullscreenArticleView()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice to have a pull request without all the whitespace changes. Otherwise it is hard to review.
Of course, we can have whitespace changes, but please in a different PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry .. It looks like it came from using the reformat function of the Android studio.

@@ -1 +0,0 @@
../values/strings.xml
Copy link
Contributor

Choose a reason for hiding this comment

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

i do not get why this file was changed. can you please explain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deleted it arbitrarily because it was a string.xml file with nothing. Sorry that made it impossible to run on my computer.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a symlink. values-en/strings.xml points to ../values/strings.xml because english translation is our default language.

Copy link
Contributor Author

@saechimdaeki saechimdaeki Nov 19, 2019

Choose a reason for hiding this comment

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

I put it back. but it make all checks have failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here the TRAVIS CI shows all checks have failed. Can I get a review without this file for a while?

@di72nn
Copy link
Member

di72nn commented Nov 19, 2019

@saechimdaeki thank you for your work and your continued effort to fix it, but I think that if we decide to have this feature it would be better if I make a new PR based on yours with some changes I'd like to make. I would of course credit you for the initial idea and implementation.

@Strubbl

I like the idea of the feature since i have some articles, which have graphics, where i cannot zoom in from wallabag. So i go to the original website and look for the pic.

Note that this way of implementing it seems to be doing simple scaling - it scales already rendered image - so no extra pixels. Particularly for images it may be better to try to implement "tap to open" (I think there's an issue for that).

@Strubbl
Copy link
Contributor

Strubbl commented Nov 19, 2019

so no extra pixels

In my use case this would be okay because the image embedded to the article was scaled down. So in theory the pixels are already there if i could zoom in. I do not know if the simple scaling would only zoom into the smaller rendered image or if it zooms into the original image.

@di72nn
Copy link
Member

di72nn commented Nov 19, 2019

would only zoom into the smaller rendered image or if it zooms into the original image

The smaller rendered one.

@Strubbl
Copy link
Contributor

Strubbl commented Nov 19, 2019

In that case the feature is useless.

@saechimdaeki
Copy link
Contributor Author

Speaking of words, I tried.
Now let go of your finger and it will return to its original size.

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.

4 participants