Swipe down to close images #122

2023-01-11T21:16:10Z

davidoskky commented

2023-01-11 21:16:10 +00:00

Types of changes

I have read the CONTRIBUTING document.
My code follows the code style of this project.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
This is NOT translation related.

This is implements feature #13

Images are closed by swiping down, no graphical clue is given however.
Eventually I decided to use the SwipeRefreshLayout since it already implements the correct gesture, this way there's no need to implement a specific way to detect the gesture.

## Types of changes - [x] I have read the **CONTRIBUTING** document. - [x] My code follows the code style of this project. - [x] I have updated the documentation accordingly. - [ ] I have added tests to cover my changes. - [x] All new and existing tests passed. - [x] This is **NOT** translation related. This is implements feature #13 Images are closed by swiping down, no graphical clue is given however. Eventually I decided to use the SwipeRefreshLayout since it already implements the correct gesture, this way there's no need to implement a specific way to detect the gesture.

davidoskky force-pushed swipe_down from 9109eb36c1 to 1f7a47c614

2023-01-11 21:17:00 +00:00

Compare

AmineB reviewed 2023-01-11 21:24:16 +00:00

CHANGELOG.md Outdated

						
				@@ -1,3 +1,7 @@

				- feat: Close images by swiping down

AmineB commented

2023-01-11 21:24:16 +00:00

This will be generated automatically. Can you please remove this ?

davidoskky commented

2023-01-11 21:29:11 +00:00

Sure, there you go

AmineB marked this conversation as resolved

davidoskky force-pushed swipe_down from 1f7a47c614 to 2b6659f4ec

2023-01-11 21:28:59 +00:00

Compare

AmineB commented

2023-01-11 21:33:12 +00:00

I'm not sure if anyone will discover this feature if there is no way to tell if the image is being closed.

I can't merge this like this.

I'm not sure if anyone will discover this feature if there is no way to tell if the image is being closed. I can't merge this like this.

davidoskky commented

2023-01-11 21:44:09 +00:00

I agree, a graphical indication that the image will be closed would be proper.
I'm unsure how to introduce it though: the best thing would be an animation that shows the image closing.
I'll try to look into that already, I'm quite sure all libraries doing that are outdated, but I may find some easy way to do it.

I agree, a graphical indication that the image will be closed would be proper. I'm unsure how to introduce it though: the best thing would be an animation that shows the image closing. I'll try to look into that already, I'm quite sure all libraries doing that are outdated, but I may find some easy way to do it.

davidoskky added 1 commit 2023-01-20 15:36:58 +00:00

Animate swipe down to close images 68bbf5b2d3

davidoskky commented

2023-01-20 15:41:23 +00:00

Alright, I found an easy way to animate it. It's simple and it works fairly well with the viewpager as well. While dragging down the image it shrinks towards the bottom of the screen and then it closes and you're back to the article. This does not happen if you zoomed into the image, so you're safe to drag it around. The only thing is that while dragging down the image you get a white background, which isn't very nice.
I'll check if it is possible to have the article itself as background when that's happening, but I figure it could be complex and I might have to move the image fragments to be instantiated in the article fragment itself. Thus, if that's too complex I guess this could be merged as is, it currently works fine.

Alright, I found an easy way to animate it. It's simple and it works fairly well with the viewpager as well. While dragging down the image it shrinks towards the bottom of the screen and then it closes and you're back to the article. This does not happen if you zoomed into the image, so you're safe to drag it around. The only thing is that while dragging down the image you get a white background, which isn't very nice. I'll check if it is possible to have the article itself as background when that's happening, but I figure it could be complex and I might have to move the image fragments to be instantiated in the article fragment itself. Thus, if that's too complex I guess this could be merged as is, it currently works fine.

AmineB commented

2023-01-20 19:56:29 +00:00

It isn't reliable. You can see how weirdly it works on the video attached.

Enregistrement de l’écran 2023-01-20 à 20.53.46.mov

4.6 MiB

davidoskky commented

2023-01-21 13:30:16 +00:00

Damn, I didn't have this happening to me before. I'll try reproducing that.

davidoskky added 3 commits 2023-01-25 00:54:59 +00:00

Close the image fragment only if the image has been dragged down ddc72d85b0

Resolve issues when swiping down to close images ae927ebc57

Adapt the style of the image activity to the rest of the application cef3b2e593

davidoskky added 1 commit 2023-01-25 01:09:46 +00:00

Rename activity style to avoid interferences be7cae365a

davidoskky added 1 commit 2023-01-25 09:27:45 +00:00

Add a dark hue to the underlying article when swiping to close images a549169a7c

davidoskky added 1 commit 2023-01-25 09:34:08 +00:00

Adjust the image closing animation 3f1f86a78e

davidoskky commented

2023-01-25 09:39:30 +00:00

Alright, this is finished. The image can be swiped down to be closed and there is no interference with the viewpager or the zooming of the image. When swiping down, the image resizes to a small square at the bottom of the screen, completing the animation the activity closes, otherwise the image animates back to full screen automatically.
When swiping down, the article is visible in the background, slightly obscured by a dark color.

I have found no other major usability issues. I've been playing around with it for a while. It happened to me once that the animation stopped in the middle, tapping on the image made it start again, I was unable to reproduce this however. It is possible to swipe the image down mid way, stop swiping and start swiping horizontally to change image while it is not full screen; this doesn't appear to me as a big issue since it is not impeding the usability of the functionalities.

Alright, this is finished. The image can be swiped down to be closed and there is no interference with the viewpager or the zooming of the image. When swiping down, the image resizes to a small square at the bottom of the screen, completing the animation the activity closes, otherwise the image animates back to full screen automatically. When swiping down, the article is visible in the background, slightly obscured by a dark color. I have found no other major usability issues. I've been playing around with it for a while. It happened to me once that the animation stopped in the middle, tapping on the image made it start again, I was unable to reproduce this however. It is possible to swipe the image down mid way, stop swiping and start swiping horizontally to change image while it is not full screen; this doesn't appear to me as a big issue since it is not impeding the usability of the functionalities.

👍 1

AmineB commented

2023-01-25 19:33:26 +00:00

Just tested it, it works pretty good.

I'll review the code, now, and merge as soon as everything is closed.

Just tested it, it works pretty good. I'll review the code, now, and merge as soon as everything is closed.

AmineB reviewed 2023-01-25 19:35:53 +00:00

androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/fragments/ImageFragment.kt Outdated

						
				@@ -4,6 +4,7 @@ import android.os.Bundle

				import android.view.LayoutInflater

				import android.view.View

				import android.view.ViewGroup

				import androidx.constraintlayout.motion.widget.MotionLayout

AmineB commented

2023-01-25 19:35:53 +00:00

This may be removed.

👍 1

AmineB marked this conversation as resolved

AmineB reviewed 2023-01-25 19:40:09 +00:00

androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/ImageActivity.kt

						
				@@ -34,0 +34,4 @@

				        val transitionListener = object : MotionLayout.TransitionListener {

				            override fun onTransitionStarted(motionLayout: MotionLayout?, startId: Int, endId: Int) {

				                binding.root.setOnClickListener {

AmineB commented

2023-01-25 19:40:09 +00:00

Is this needed ?

davidoskky commented

2023-01-25 20:25:51 +00:00

Ops, sorry didn't even notice that...

AmineB marked this conversation as resolved

AmineB reviewed 2023-01-25 19:40:13 +00:00

androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/ImageActivity.kt

						
				@@ -34,0 +34,4 @@

				        val transitionListener = object : MotionLayout.TransitionListener {

				            override fun onTransitionStarted(motionLayout: MotionLayout?, startId: Int, endId: Int) {

				                binding.root.setOnClickListener {