Louvorg/ReaderForSelfoss-multiplatform
Louvorg/ReaderForSelfoss-multiplatform
3
2
Fork 2
Code Issues 16 Pull Requests Projects 1 Releases 65 Wiki Activity

Swipe down to close images #122

Merged
AmineL merged 10 commits from davidoskky/ReaderForSelfoss-multiplatform:swipe_down into master 2023-01-26 10:42:16 +00:00
Conversation 7 Commits 10 Files Changed 8 +109 -31
davidoskky commented 2023-01-11 21:16:10 +00:00
Contributor
Copy Link

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
AmineL reviewed 2023-01-11 21:24:16 +00:00
CHANGELOG.md Outdated
@ -1,3 +1,7 @@
- feat: Close images by swiping down
AmineL commented 2023-01-11 21:24:16 +00:00
Owner
Copy Link

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

This will be generated automatically. Can you please remove this ?
davidoskky commented 2023-01-11 21:29:11 +00:00
Author
Contributor
Copy Link

Sure, there you go

Sure, there you go
AmineL marked this conversation as resolved
davidoskky force-pushed swipe_down from 1f7a47c614 to 2b6659f4ec 2023-01-11 21:28:59 +00:00 Compare
AmineL commented 2023-01-11 21:33:12 +00:00
Owner
Copy Link

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
Author
Contributor
Copy Link

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
davidoskky commented 2023-01-20 15:41:23 +00:00
Author
Contributor
Copy Link

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.
AmineL commented 2023-01-20 19:56:29 +00:00
Owner
Copy Link

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

It isn't reliable. You can see how weirdly it works on the video attached.
Enregistrement de l’écran 2023-01-20 à 20.53.46.mov
4.6 MiB
davidoskky commented 2023-01-21 13:30:16 +00:00
Author
Contributor
Copy Link

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

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
davidoskky added 1 commit 2023-01-25 01:09:46 +00:00
davidoskky added 1 commit 2023-01-25 09:27:45 +00:00
davidoskky added 1 commit 2023-01-25 09:34:08 +00:00
davidoskky commented 2023-01-25 09:39:30 +00:00
Author
Contributor
Copy Link

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
AmineL commented 2023-01-25 19:33:26 +00:00
Owner
Copy Link

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.
AmineL 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
AmineL commented 2023-01-25 19:35:53 +00:00
Owner
Copy Link

This may be removed.

This may be removed.
👍 1
AmineL marked this conversation as resolved
AmineL 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 {
AmineL commented 2023-01-25 19:40:09 +00:00
Owner
Copy Link

Is this needed ?

Is this needed ?
davidoskky commented 2023-01-25 20:25:51 +00:00
Author
Contributor
Copy Link

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

Ops, sorry didn't even notice that...
AmineL marked this conversation as resolved
AmineL 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 {
AmineL commented 2023-01-25 19:40:12 +00:00
Owner
Copy Link

Is this needed ?

Is this needed ?
davidoskky commented 2023-01-25 20:10:44 +00:00
Author
Contributor
Copy Link

I just checked again, if any of those definitions is removed the build fails.

I just checked again, if any of those definitions is removed the build fails.
AmineL commented 2023-01-25 20:13:35 +00:00
Owner
Copy Link

I meant the empty onclick listener

I meant the empty onclick listener
AmineL marked this conversation as resolved
davidoskky added 1 commit 2023-01-25 19:40:27 +00:00
AmineL reviewed 2023-01-25 19:41:26 +00:00
AmineL left a comment
Owner
Copy Link

I'll merge as soon as everything is ok

I'll merge as soon as everything is ok
davidoskky added 1 commit 2023-01-25 20:25:35 +00:00
AmineL merged commit 2154ff3c33 into master 2023-01-26 10:42:16 +00:00
davidoskky deleted branch swipe_down 2023-01-26 11:52:58 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
Difficulty - Beginner friendly

   
Difficulty = Easy

   
Difficulty = Hard

   
Difficulty = Medium

   
Priority = CRITICAL

   
Priority = High

   
Priority = Low

   
Priority = Normal

   
Priority = Some day

   
Status - Fixed somewhere else

   
Status - No details provided

   
Status = Can't fix

   
Status = Duplicate

   
Status = Help wanted

   
Status = Invalid

   
Status = Need more details

   
Status = Taken

   
Status = Wontfix

   
Type - Selfoss works like this

   
Type = Bug

   
Type = Chore

   
Type = Enhancement

   
Type = Feature

   
Type = Question

   
Type = SELFOSS API ISSUE

   
Type = Tools

   
Type = UX/Design

   
Up For Grabs
No Label
Difficulty - Beginner friendly
Difficulty = Easy
Difficulty = Hard
Difficulty = Medium
Priority = CRITICAL
Priority = High
Priority = Low
Priority = Normal
Priority = Some day
Status - Fixed somewhere else
Status - No details provided
Status = Can't fix
Status = Duplicate
Status = Help wanted
Status = Invalid
Status = Need more details
Status = Taken
Status = Wontfix
Type - Selfoss works like this
Type = Bug
Type = Chore
Type = Enhancement
Type = Feature
Type = Question
Type = SELFOSS API ISSUE
Type = Tools
Type = UX/Design
Up For Grabs
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Louvorg/ReaderForSelfoss-multiplatform#122
No description provided.
Delete Branch "davidoskky/ReaderForSelfoss-multiplatform:swipe_down"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?