Put song title text view into a horizontal scrollview so you can see the full song title on songs with longer song titles #160

theduffster · 2017-05-31T21:13:17Z

There's not a lot of room for the song title in the current song view, which means the song title gets cut off most of the time and it's hard to figure out what the full name of the song is without going into the song details dialog. All of this is very annoying. Putting the textview into a horizontal scroll view is an elegant solution because it doesn't compromise any existing functionality, it doesn't change anything and it is very easy for the user to interact with.

full song title on songs with longer song titles

niririke · 2017-06-07T18:47:42Z

I think I found a problem. Playing a song from e.g. album or playlist does not work sometimes with this change. I think it is because when you tap song's title it tries to scroll it horizontally. This happens even if the title is not long and can't actually be scrolled. Maybe we can use separate xml for current song view because the same file (item_list.xml) is used in both current song view and in playlists and albums (do I understand this correctly?). Though pressing song's title in current song view does not open queue now too.

theduffster · 2017-06-07T19:06:06Z

You're more then right. Actually, it seems like you can't play any song without clicking something that's not the textview. I'll have to see how I can fix this.

Making the text scrollable took away the ripple animations and it’s ability to field the onclicklistener, so I had to recreate and implement ripple animations and on touch from scratch. It works, although not as smoothly as I’d like. Test it out and let me know your thoughts. I’m not sure there’s an easier way to do this, but someone can try and find one.

theduffster · 2017-06-08T04:49:39Z

Making the text scrollable took away the ripple animations and it’s ability to field the onclicklistener, so I had to recreate and implement ripple animations and on touch from scratch. It works, although it would be better if there was a simple way to make a scrollview not mess with the onclicklistener for the rest of the view. Test it out and let me know your thoughts. I’m not sure there’s an easier way to do this, but someone can try and find one.

niririke · 2017-06-08T11:04:05Z

It surely works now! But there is a little (around half a second I guess) delay before ripple animation and actual song playback when I press song's title. Pressing anywhere else works fine (fast) though. It's not that big a deal I guess, but it is still perceptible.

theduffster · 2017-06-08T17:30:57Z

@niririke I agree. I don't think we should merge this until we get it so it's seamless. I am going to try to figure out what's slowing it down.

arkon · 2017-06-08T17:37:02Z

I imagine it's something to do with it trying to register what type of touch it was (i.e. Was it actually a double touch? Better wait a split second to be sure.). Is it possible to add a listener to the ScrollView so that if the scrolling motion is under a certain threshold, it just dispatches the event (tap) to the underlying view instead?

theduffster · 2017-06-08T17:37:55Z

@arkon that's exactly what I'm trying right now.

theduffster · 2017-06-08T21:21:16Z

I GOT IT! Should be up and working soon. Had to create a custom FrameLayout class and intercept the touch event and control whether it gets passed to the scrollview or not.

Added a custom FrameLayout view that intercepts touch events and decides whether to consume them or pass on the touch events to it's children. This gives us control over if the FramLayout or the HorizontalScrollView is the one getting the touch event.

theduffster · 2017-06-09T05:57:01Z

Touch handling in Android is terrible… Anyway, it should work smoothly now, as if there's no difference other then that the text scrolls horizontally (because as far as I can tell there is no difference). I'll merge it in a couple days once a couple people have reviewed it.

theduffster · 2017-06-09T07:39:43Z

So it seems that if you click on the 3 dots to open the menu sometimes the click also registers on the framelayout and opens the song. Gonna have to figure that out tomorrow.

arkon · 2017-06-09T16:46:15Z

.gitignore

@@ -40,3 +40,7 @@ captures/

 # Mac
 .DS_Store
+
+*.gradle


These shouldn't be in the gitignore.

arkon · 2017-06-09T16:48:29Z

build.gradle

@@ -1,9 +1,13 @@
 buildscript {
    repositories {
        jcenter()
+        maven {


You should just leave this out of this PR for now.

arkon · 2017-06-09T16:48:33Z

gradle/wrapper/gradle-wrapper.properties

@@ -1,6 +1,6 @@
-#Tue Apr 11 17:46:33 SGT 2017
+#Wed Jun 07 15:02:52 EDT 2017


You should just leave this out of this PR for now.

arkon · 2017-06-09T16:49:10Z

app/src/main/res/layout/item_list.xml

@@ -20,7 +20,16 @@
        android:tintMode="src_in"
        tools:ignore="ContentDescription" />

+    <View


So is this just a dummy view for intercepting touches?

This is just a regular framelayout, but I've Overrided onInterceptTouch so I can delegate any touches to the right views. In this case either the HorizontalScrollView or this FrameLayout.

theduffster · 2017-06-09T18:32:57Z

It's now 100% working with no drawbacks compared to how the app performs without the scrolling textviews. However, if you test it let me know if you find anything wrong.

arkon · 2017-06-09T18:52:23Z

I think I'm expecting it to register as a regular tap (or other things like long pressing) if I just tap directly on the scrolling textview, but it doesn't. Seems like an unexpected behaviour?

theduffster · 2017-06-09T19:00:06Z

It should just register as a regular tap, no different then if you were to touch another part of the screen. It should only not register a tap if you start scrolling and using the scrollview. That certainly it weird, on my end it seems to be working. @arkon what activity are you in?

arkon · 2017-06-09T19:02:10Z

@theduffmaster It seems to happen everywhere (e.g. album view, songs list, now playing queue).

theduffster · 2017-06-09T19:03:38Z

@arkon so when you tap on the scrolling textview nothing happens? No animation, no click?

kabouzeid · 2017-06-09T19:04:39Z

I'll have a look at this now too

theduffster · 2017-06-09T19:04:59Z

That would be much appreciated @kabouzeid

arkon · 2017-06-09T19:11:29Z

@theduffmaster Here's a video.

theduffster · 2017-06-09T19:13:48Z

@arkon Thanks for the video, it helps alot. If that's how it is on you side, then we have a big problem. That's not how it's supposed to act at all. That's how it acted back on the first commit. Are you sure you've synced the most recent commits?

kabouzeid

I don't quite understand what the purpose of this whole thing is. It seems to be some kind of a workaround?

kabouzeid · 2017-06-09T19:11:36Z

gradle/wrapper/gradle-wrapper.properties

 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-3.5-all.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-4.0-milestone-1-all.zip


Not yet in the production branch

You're right. My git configuration got messed up, I'll remove these

kabouzeid · 2017-06-09T19:12:04Z

build.gradle

    }
    dependencies {
-        classpath 'com.android.tools.build:gradle:2.3.1'
+        classpath 'com.android.tools.build:gradle:3.0.0-alpha3'


Not yet in the production branch

theduffster · 2017-08-01T19:13:43Z

@arkon I removed the invisible view (they were originally there for testing) and changed the text view name to AutoTruncateTextView, I agree a much more suitable name. It seems like at some point the text setting was broken and it now occasionally sets the text in the recyclerview as the previous truncated text. I'm on vacation, but when I get a chance I'll look into this. It wasn't happening before so I wonder what broke it. Scroll something in the songs tab and then scroll up and down and you'll see that same song title appear where other songs should be.

Edit: I think I fixed it
Edit: Nope

theduffster · 2017-08-01T19:40:26Z

It seems the invisible views served to keep the artist page visible. I'll add them back when I get to a computer.

…

On Sat, Jul 29, 2017, 7:05 PM Eugene Cheung ***@***.***> wrote: ***@***.**** commented on this pull request. Looks pretty good to me. I'd consider maybe renaming the TextView to something like AutoTruncateTextView though, since that's more clear as to what it does. Moving the three view classes to a package can keep it obvious that they work with each other too. ------------------------------ In app/src/main/res/layout/item_list.xml <#160 (comment)>: > @@ -20,7 +22,15 @@ ***@***.***/ic_drag_vertical_white_24dp" tools:ignore="ContentDescription" /> + <View So what is this invisible view for again? ------------------------------ In app/src/main/res/layout/activity_artist_detail.xml <#160 (comment)>: > @@ -32,25 +34,38 @@ android:layout_height="wrap_content" android:orientation="vertical"> - <TextView + <View What's this for again? ------------------------------ In app/src/main/java/com/kabouzeid/gramophone/views/TouchInterceptFrameLayout.java <#160 (comment)>: > + public boolean onInterceptTouchEvent(MotionEvent e) { + int x = Math.round(e.getRawX()); + int y = Math.round(e.getRawY()); + + scrollView = getTouchInterceptHorizontalScrollView(); + scrollView.getGlobalVisibleRect(scrollViewRect); + + boolean touchedScrollView = + x > scrollViewRect.left && x < scrollViewRect.right && + y > scrollViewRect.top && y < scrollViewRect.bottom; + + if (scrollView.isScrollable()) { + switch (e.getAction() & MotionEvent.ACTION_MASK) { + case MotionEvent.ACTION_DOWN: + if (!touchedScrollView) { + scrollView.cancelPendingInputEvents(); Is this required? It needs minimum API 19. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#160 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGCrnTD3To0-IRKgtVYH6ftcFNCLN761ks5sS7qvgaJpZM4NsM2W> .

…o keep the title view visible

arkon · 2017-08-02T00:29:32Z

I'm noticing the occasional wrong string too.

arkon · 2017-08-03T02:57:19Z

@theduffmaster I committed something that I think is a better fix for it. Let me know if you still see the issue.

theduffster · 2017-08-03T03:17:13Z

app/src/main/java/com/kabouzeid/gramophone/views/AutoTruncateTextView.java

+        }
+        this.truncatedText = truncatedText;
+
+        final TouchInterceptHorizontalScrollView scrollView = getTouchInterceptHorizontalScrollView();


As long as the scrollview never shows up as null, which I don't think it will because the mParent flag should be set after onMeasure, this will do even better. Good work!

I think that's everything right?

theduffster · 2017-08-05T19:11:15Z

@arkon After a couple days of testing your fix seems to work! Thanks again. Pending a couple more days of testing I think this is about done!

…e reasons and other performance improvements After testing I found that finding the scrollview on the main thread sometimes slowed things down.

…t retruncate

…the full song title on songs with longer song titles kabouzeid/Phonograph#160

Put song title text view into a horizontal scrollview so you can see the

85b2aab

full song title on songs with longer song titles

theduffster requested a review from kabouzeid May 31, 2017 21:13

Merge remote-tracking branch 'kabouzeid/master' into song_name_scroll

7ad72c5

theduffster added 2 commits June 9, 2017 02:22

fixed formatting and removed debugging

c33623d

fixed formatting

b232b39

Fix Menu Item Click being stolen by parent view

1d990f8

arkon reviewed Jun 9, 2017

View reviewed changes

Fixed Longclick not firing in every spot in the view

f07d337

kabouzeid reviewed Jun 9, 2017

View reviewed changes

fixed wrong string not truncating (I think)

780f0d7

theduffster and others added 2 commits August 1, 2017 19:35

Reverted invisible view removal because it turns out those are used t…

e9596ed

…o keep the title view visible

Minor edits

caa3d2c

theduffster and others added 2 commits August 2, 2017 00:44

Possibly fixed wrong re truncating text

a617668

Possibly better fix for wrong text

ac46c30

theduffster commented Aug 3, 2017

View reviewed changes

arkon approved these changes Aug 5, 2017

View reviewed changes

theduffster and others added 9 commits August 7, 2017 12:25

Fixed the occasional long click on scroll

4237452

Don't need this anymore

a24bd98

Code cleanup

b2971b3

Reverted back to old method of checking for retruncate for performanc…

0c68210

…e reasons and other performance improvements After testing I found that finding the scrollview on the main thread sometimes slowed things down.

Better way of canceling unwarranted long clicks

6f600b6

Code cleanup

1800182

Addressed invalid pointer id error

f09c145

Reverted scrollview because that fixes the occasional unwarranted tex…

8bef5cc

…t retruncate

Use animator listener to retruncate text instead of timeout handler

3b3104e

arkon removed needs changes needs review labels Aug 12, 2017

Added further documentation

9eb14b6

theduffster added the reviewing for merge label Aug 13, 2017

Corrected code inconsistency

78890be

theduffster mentioned this pull request Aug 14, 2017

Album name is click through #204

Open

Merge remote-tracking branch 'kabouzeid/master' into song_name_scroll

26548dc

AdrienPoupa added a commit to VinylMusicPlayer/VinylMusicPlayer that referenced this pull request Jan 28, 2019

Put song title text view into a horizontal scrollview so you can see …

ba0dfae

…the full song title on songs with longer song titles kabouzeid/Phonograph#160

		@@ -1,6 +1,6 @@
		#Tue Apr 11 17:46:33 SGT 2017
		#Wed Jun 07 15:02:52 EDT 2017

Put song title text view into a horizontal scrollview so you can see the full song title on songs with longer song titles #160

Are you sure you want to change the base?

Put song title text view into a horizontal scrollview so you can see the full song title on songs with longer song titles #160

Conversation

theduffster commented May 31, 2017

niririke commented Jun 7, 2017

theduffster commented Jun 7, 2017

theduffster commented Jun 8, 2017 • edited Loading

niririke commented Jun 8, 2017

theduffster commented Jun 8, 2017

arkon commented Jun 8, 2017

theduffster commented Jun 8, 2017

theduffster commented Jun 8, 2017

theduffster commented Jun 9, 2017 • edited Loading

theduffster commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theduffster commented Jun 9, 2017

arkon commented Jun 9, 2017

theduffster commented Jun 9, 2017

arkon commented Jun 9, 2017

theduffster commented Jun 9, 2017

kabouzeid commented Jun 9, 2017

theduffster commented Jun 9, 2017

arkon commented Jun 9, 2017

theduffster commented Jun 9, 2017 • edited Loading

kabouzeid left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theduffster commented Aug 1, 2017 • edited Loading

theduffster commented Aug 1, 2017 via email

arkon commented Aug 2, 2017

arkon commented Aug 3, 2017

theduffster Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

theduffster commented Aug 5, 2017 • edited Loading

theduffster commented Jun 8, 2017 •

edited

Loading

theduffster commented Jun 9, 2017 •

edited

Loading

theduffster commented Jun 9, 2017 •

edited

Loading

kabouzeid left a comment •

edited

Loading

theduffster commented Aug 1, 2017 •

edited

Loading

theduffster Aug 3, 2017 •

edited

Loading

theduffster commented Aug 5, 2017 •

edited

Loading