Highlight the current song on the album detail view #133
Conversation
|
|
||
| override fun equals(other: Any?): Boolean { | ||
| if (this === other) return true | ||
| if (other == null || this::class != other::class) return false | ||
|
|
||
| other as Song | ||
|
|
||
| if (id != other.id) return false | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| override fun hashCode(): Int { | ||
| return id.hashCode() | ||
| } |
There was a problem hiding this comment.
I don't think it's a good idea to override the default behaviour of equals/hashcode here, as a convenience for comparing Song id's.
1.) This means that Song no longer behaves like a regular data class, which could be confusing / lead to unexpected behaviour
2.) I'm not sure if we're relying on the existing equality contract in other places, which could be broken by this change
3.) It's easy to just compare song.id == otherSong.id
There was a problem hiding this comment.
I was a bit overwhelmed when a wrote that, so I don't think I noticed it was a data class. Anyway, I didn't remember about the data classes having already a proper equals ;-)
Fixed in 3a82613.
Data classes already implement equals with the constructor fields, so it's better not to override it without a good reason.
| getCurrentSong()?.let { | ||
| view?.onCurrentSongChanged(it) | ||
| getCurrentSong()?.let { currentSong -> | ||
| view?.setData([email protected], currentSong) |
There was a problem hiding this comment.
Out of curiosity. Why is the qualifier needed in [email protected]? I've tried and it seems to work without it.
By the way, nice work! These changes make it much simpler. Thanks!
There was a problem hiding this comment.
Oh, this@ is not needed, that looks like a copy paste error :D
Until now, when playing an album, there was no way to view on the album detail view which song was playing. With these changes, now the current playing song is highlighted with a light blue background, like in the queue list. This way it's easier for the user to find the current point in the album.