From 705c40371dd60521f8ecb748a66c3e57cb31dfc3 Mon Sep 17 00:00:00 2001 From: aidewoode Date: Fri, 26 Apr 2024 21:24:10 +0800 Subject: [PATCH] Add new playing behavior on songs in album or playlist Right now click song in album or playlist will add the specific song to the current playlist to play. Change the behavior to add all songs from the album or playlist and stated to play the specific song instead. --- .../songs/albums_controller.rb | 6 ++- .../songs/playlists_controller.rb | 5 +- .../current_playlist/songs_controller.rb | 4 +- .../current_playlist_songs_controller.js | 13 +++-- .../playlist_songs_bridge_controller.js | 26 ++++++++-- app/javascript/native_bridge.js | 27 ++++++++--- app/views/albums/show.html.erb | 47 ++++++++++++------- .../current_playlist/songs/_list.html.erb | 3 -- .../current_playlist/songs/_song.html.erb | 2 +- .../songs/create.turbo_stream.erb | 4 +- .../current_playlist/songs/index.html.erb | 8 +++- .../favorite_playlist/songs/index.html.erb | 10 ++-- app/views/playlists/songs/_song.html.erb | 37 ++++++++++----- app/views/playlists/songs/index.html.erb | 10 ++-- app/views/songs/_song.html.erb | 2 +- config/locales/en.yml | 3 +- .../songs/albums_controller_test.rb | 4 +- .../songs/playlists_controller_test.rb | 4 +- test/system/album_test.rb | 2 +- test/system/favorite_playlist_test.rb | 2 +- test/system/playlist_test.rb | 2 +- 21 files changed, 143 insertions(+), 78 deletions(-) delete mode 100644 app/views/current_playlist/songs/_list.html.erb diff --git a/app/controllers/current_playlist/songs/albums_controller.rb b/app/controllers/current_playlist/songs/albums_controller.rb index 84b7953c..e07060ee 100644 --- a/app/controllers/current_playlist/songs/albums_controller.rb +++ b/app/controllers/current_playlist/songs/albums_controller.rb @@ -7,7 +7,11 @@ class CurrentPlaylist::Songs::AlbumsController < ApplicationController def update @current_playlist.replace(@album.song_ids) - redirect_to current_playlist_songs_path(should_play_all: true) + + redirect_to current_playlist_songs_path( + should_play: params[:should_play], + song_id: params[:song_id] + ) end private diff --git a/app/controllers/current_playlist/songs/playlists_controller.rb b/app/controllers/current_playlist/songs/playlists_controller.rb index 00f4998a..f6fb54c9 100644 --- a/app/controllers/current_playlist/songs/playlists_controller.rb +++ b/app/controllers/current_playlist/songs/playlists_controller.rb @@ -6,7 +6,10 @@ class CurrentPlaylist::Songs::PlaylistsController < ApplicationController def update @current_playlist.replace(@playlist.song_ids) - redirect_to current_playlist_songs_path(should_play_all: true) + redirect_to current_playlist_songs_path( + should_play: params[:should_play], + song_id: params[:song_id] + ) end private diff --git a/app/controllers/current_playlist/songs_controller.rb b/app/controllers/current_playlist/songs_controller.rb index b3e0d8d5..9c1744fe 100644 --- a/app/controllers/current_playlist/songs_controller.rb +++ b/app/controllers/current_playlist/songs_controller.rb @@ -7,6 +7,8 @@ class CurrentPlaylist::SongsController < Playlists::SongsController def index @songs = @playlist.songs_with_favorite + @should_play = params[:should_play] == "true" + @should_play_song_id = params[:song_id].to_i if @should_play end def create @@ -21,7 +23,7 @@ def create flash.now[:success] = t("notice.added_to_playlist") - redirect_to action: "index", should_play_all: params[:should_play] if @playlist.songs.count == 1 + redirect_to action: "index", should_play: params[:should_play] if @playlist.songs.count == 1 rescue ActiveRecord::RecordNotUnique flash.now[:error] = t("error.already_in_playlist") render turbo_stream: render_flash diff --git a/app/javascript/controllers/current_playlist_songs_controller.js b/app/javascript/controllers/current_playlist_songs_controller.js index 8ce612fa..2f01982a 100644 --- a/app/javascript/controllers/current_playlist_songs_controller.js +++ b/app/javascript/controllers/current_playlist_songs_controller.js @@ -6,7 +6,7 @@ export default class extends Controller { static targets = ['item'] static values = { - shouldPlayAll: Boolean + shouldPlay: Boolean } initialize () { @@ -16,21 +16,24 @@ export default class extends Controller { itemTargetConnected (target) { const song = JSON.parse(target.dataset.songJson) - const shouldPlay = target.dataset.shouldPlay === 'true' + const songShouldPlay = target.dataset.shouldPlay === 'true' const targetIndex = this.itemTargets.indexOf(target) this.playlist.insert(targetIndex, song) - if (shouldPlay) { + if (songShouldPlay) { this.player.skipTo(targetIndex) + delete target.dataset.shouldPlay + this.shouldPlayValue = false } } connect () { - if (this.shouldPlayAllValue) { + if (this.shouldPlayValue) { + // If no particular song is set to play, play the first song this.player.skipTo(0) - this.shouldPlayAllValue = false + this.shouldPlayValue = false } this.handleEvent('click', { diff --git a/app/javascript/controllers/playlist_songs_bridge_controller.js b/app/javascript/controllers/playlist_songs_bridge_controller.js index ee943f6b..db771d97 100644 --- a/app/javascript/controllers/playlist_songs_bridge_controller.js +++ b/app/javascript/controllers/playlist_songs_bridge_controller.js @@ -7,6 +7,11 @@ export default class extends Controller { return isNativeApp() } + static values = { + resourceType: String, + resourceId: Number + } + initialize () { installEventHandler(this) } @@ -14,7 +19,13 @@ export default class extends Controller { connect () { this.handleEvent('click', { on: this.element, - with: this.playSong, + with: this.playResourceBeginWith, + delegation: true + }) + + this.handleEvent('click', { + on: this.element, + with: this.playNow, delegation: true }) @@ -31,13 +42,18 @@ export default class extends Controller { }) } - playAll ({ params }) { - App.nativeBridge.playAll(params.resourceType, params.resourceId) + playResource () { + App.nativeBridge.playResource(this.resourceTypeValue, this.resourceIdValue) + } + + playResourceBeginWith = (event) => { + const { songId } = event.target.closest('[data-song-id]').dataset + App.nativeBridge.playResourceBeginWith(this.resourceTypeValue, this.resourceIdValue, songId) } - playSong (event) { + playNow (event) { const { songId } = event.target.closest('[data-song-id]').dataset - App.nativeBridge.playSong(songId) + App.nativeBridge.playNow(songId) } playNext (event) { diff --git a/app/javascript/native_bridge.js b/app/javascript/native_bridge.js index 3a9c9752..dbbbc148 100644 --- a/app/javascript/native_bridge.js +++ b/app/javascript/native_bridge.js @@ -1,30 +1,45 @@ import { isAndroidApp, isiOSApp } from './helper' class NativeBridge { - playAll (resourceType, resourceId) { + playResource (resourceType, resourceId) { if (isiOSApp()) { window.webkit.messageHandlers.nativeApp.postMessage({ - name: 'playAll', + name: 'playResource', resourceType, resourceId: Number(resourceId) }) } if (isAndroidApp()) { - window.NativeBridge.playAll(resourceType, Number(resourceId)) + window.NativeBridge.playResource(resourceType, Number(resourceId)) } } - playSong (songId) { + playResourceBeginWith (resourceType, resourceId, songId) { if (isiOSApp()) { window.webkit.messageHandlers.nativeApp.postMessage({ - name: 'playSong', + name: 'playResourceBeginWith', + resourceType, + resourceId: Number(resourceId), + songId: Number(songId) + }) + } + + if (isAndroidApp()) { + window.NativeBridge.playResourceBeginWith(resourceType, Number(resourceId), Number(songId)) + } + } + + playNow (songId) { + if (isiOSApp()) { + window.webkit.messageHandlers.nativeApp.postMessage({ + name: 'playNow', songId: Number(songId) }) } if (isAndroidApp()) { - window.NativeBridge.playSong(Number(songId)) + window.NativeBridge.playNow(Number(songId)) } } diff --git a/app/views/albums/show.html.erb b/app/views/albums/show.html.erb index 1f4dadcf..c523406a 100644 --- a/app/views/albums/show.html.erb +++ b/app/views/albums/show.html.erb @@ -1,6 +1,6 @@ <% page_title_tag @album.name %> -
+
<%= cover_image_tag @album, class: "c-card__image u-image-medium", data: {"test-id" => "album_image"} %>
@@ -13,8 +13,8 @@
<%= button_to( - t("button.play_all"), - current_playlist_album_path(@album), + t("button.play"), + current_playlist_album_path(@album, should_play: true), method: :put, form_class: "u-display-inline-block", class: "c-button c-button--primary", @@ -22,9 +22,7 @@ data: { "disabled-on-native" => "true", "turbo-frame" => "turbo-playlist", - "action" => "playlist-songs-bridge#playAll", - "playlist-songs-bridge-resource-type-param" => "album", - "playlist-songs-bridge-resource-id-param" => @album.id + "action" => "playlist-songs-bridge#playResource" } } ) %> @@ -44,12 +42,13 @@
  • <%= button_to( - current_playlist_songs_path(song_id: song.id, should_play: true), + current_playlist_album_path(@album, should_play: true, song_id: song.id), + method: :put, class: "c-button c-button--link u-w-100", form_class: "o-flex__item--grow-1", form: { data: { - "delegated-action" => "turbo:submit-start->playlist-songs#checkBeforePlay click->playlist-songs-bridge#playSong", + "delegated-action" => "turbo:submit-start->playlist-songs#checkBeforePlay click->playlist-songs-bridge#playResourceBeginWith", "turbo-frame" => "turbo-playlist", "disabled-on-native" => "true" } @@ -75,16 +74,17 @@ <%= icon_tag "more-vertical", size: "small", title: t("label.more") %>
    - <%= link_to( - t("label.go_to_artist"), - artist_path(song.artist), - class: "c-dropdown__item" - ) %> - <%= link_to( - t("label.add_to_playlist"), - dialog_playlists_path(song_id: song.id, referer_url: current_url), - data: {"turbo-frame" => ("turbo-dialog" unless native_app?)}, - class: "c-dropdown__item" + <%= button_to( + t("button.play_now"), + current_playlist_songs_path(song_id: song.id, should_play: true), + form_class: "c-dropdown__item", + form: { + data: { + "turbo-frame" => "turbo-playlist", + "delegated-action" => "turbo:submit-start->playlist-songs#checkBeforePlay click->playlist-songs-bridge#playNow", + "disabled-on-native" => "true" + } + } ) %> <%= button_to( t("button.play_next"), @@ -110,6 +110,17 @@ } } ) %> + <%= link_to( + t("label.add_to_playlist"), + dialog_playlists_path(song_id: song.id, referer_url: current_url), + data: {"turbo-frame" => ("turbo-dialog" unless native_app?)}, + class: "c-dropdown__item" + ) %> + <%= link_to( + t("label.go_to_artist"), + artist_path(song.artist), + class: "c-dropdown__item" + ) %>
    diff --git a/app/views/current_playlist/songs/_list.html.erb b/app/views/current_playlist/songs/_list.html.erb deleted file mode 100644 index c4f4392f..00000000 --- a/app/views/current_playlist/songs/_list.html.erb +++ /dev/null @@ -1,3 +0,0 @@ -
      - <%= render partial: "current_playlist/songs/song", collection: songs, locals: {playlist: playlist} %> -
    diff --git a/app/views/current_playlist/songs/_song.html.erb b/app/views/current_playlist/songs/_song.html.erb index 538f0ff1..38e09afb 100644 --- a/app/views/current_playlist/songs/_song.html.erb +++ b/app/views/current_playlist/songs/_song.html.erb @@ -3,7 +3,7 @@ data-current-playlist-songs-target='item' data-song-id='<%= song.id %>' data-song-json='<%= song_json_builder(song).target! %>' - data-should-play='<%= params[:should_play] %>' + data-should-play='<%= local_assigns[:should_play] ? should_play : false %>' draggable='true' data-test-id='current_playlist_song'> diff --git a/app/views/current_playlist/songs/create.turbo_stream.erb b/app/views/current_playlist/songs/create.turbo_stream.erb index 7bfa3352..cfe2c788 100644 --- a/app/views/current_playlist/songs/create.turbo_stream.erb +++ b/app/views/current_playlist/songs/create.turbo_stream.erb @@ -2,10 +2,10 @@ <%= turbo_stream.after "#{dom_id(@playlist)}_#{dom_id(@playlist.songs.second_to_last)}", partial: 'current_playlist/songs/song', locals: { song: @song, playlist: @playlist } %> <% else %> <% if @current_song_position.zero? %> - <%= turbo_stream.before "#{dom_id(@playlist)}_#{dom_id(@playlist.songs.second)}", partial: 'current_playlist/songs/song', locals: { song: @song, playlist: @playlist } %> + <%= turbo_stream.before "#{dom_id(@playlist)}_#{dom_id(@playlist.songs.second)}", partial: 'current_playlist/songs/song', locals: { song: @song, playlist: @playlist, should_play: params[:should_play] } %> <% else %> <%# add song next to the current_song %> - <%= turbo_stream.after "#{dom_id(@playlist)}_song_#{params[:current_song_id]}", partial: 'current_playlist/songs/song', locals: { song: @song, playlist: @playlist } %> + <%= turbo_stream.after "#{dom_id(@playlist)}_song_#{params[:current_song_id]}", partial: 'current_playlist/songs/song', locals: { song: @song, playlist: @playlist, should_play: params[:should_play] } %> <% end %> <% end %> diff --git a/app/views/current_playlist/songs/index.html.erb b/app/views/current_playlist/songs/index.html.erb index d8226e40..9d62fb99 100644 --- a/app/views/current_playlist/songs/index.html.erb +++ b/app/views/current_playlist/songs/index.html.erb @@ -1,7 +1,7 @@
    + data-current-playlist-songs-should-play-value='<%= @should_play %>'> <% if @songs.empty? %> <%= empty_alert_tag %> <% else %> @@ -17,6 +17,10 @@
    • - <%= render partial: "current_playlist/songs/list", locals: {playlist: @playlist, songs: @songs} %> +
      + <% @songs.each do |song| %> + <%= render partial: "current_playlist/songs/song", locals: {song: song, playlist: @playlist, should_play: @should_play_song_id == song.id} %> + <% end %> +
    <% end %>
    diff --git a/app/views/favorite_playlist/songs/index.html.erb b/app/views/favorite_playlist/songs/index.html.erb index eab17eb1..0aa7dcd9 100644 --- a/app/views/favorite_playlist/songs/index.html.erb +++ b/app/views/favorite_playlist/songs/index.html.erb @@ -1,5 +1,5 @@
    -
    +

    <%= @playlist.name %>

    @@ -12,8 +12,8 @@
    <% unless @songs.blank? %> <%= button_to( - t("button.play_all"), - current_playlist_playlist_path(@playlist), + t("button.play"), + current_playlist_playlist_path(@playlist, should_play: true), method: :put, class: "c-button c-button--primary", form_class: "u-display-inline-block", @@ -21,9 +21,7 @@ data: { "disabled-on-native" => "true", "turbo-frame" => "turbo-playlist", - "action" => "playlist-songs-bridge#playAll", - "playlist-songs-bridge-resource-type-param" => "playlist", - "playlist-songs-bridge-resource-id-param" => @playlist.id + "action" => "playlist-songs-bridge#playResource" } } ) %> diff --git a/app/views/playlists/songs/_song.html.erb b/app/views/playlists/songs/_song.html.erb index 4f0e44e2..e84d727f 100644 --- a/app/views/playlists/songs/_song.html.erb +++ b/app/views/playlists/songs/_song.html.erb @@ -5,12 +5,13 @@ <% end %> <%= button_to( - current_playlist_songs_path(song_id: song.id, should_play: true), + current_playlist_playlist_path(playlist, should_play: true, song_id: song.id), + method: :put, class: "c-button c-button--link u-w-100", form_class: "o-flex__item--grow-1", form: { data: { - "delegated-action" => "turbo:submit-start->playlist-songs#checkBeforePlay click->playlist-songs-bridge#playSong", + "delegated-action" => "turbo:submit-start->playlist-songs#checkBeforePlay click->playlist-songs-bridge#playResourceBeginWith", "turbo-frame" => "turbo-playlist", "disabled-on-native" => "true" } @@ -31,16 +32,17 @@
    <%= icon_tag "more-vertical", size: "small", title: t("label.more") %>
    - <%= link_to( - t("label.go_to_artist"), - artist_path(song.artist), - class: "c-dropdown__item" - ) %> - <%= link_to( - t("label.add_to_playlist"), - dialog_playlists_path(song_id: song.id, referer_url: current_url), - data: {"turbo-frame" => ("turbo-dialog" unless native_app?)}, - class: "c-dropdown__item" + <%= button_to( + t("button.play_now"), + current_playlist_songs_path(song_id: song.id, should_play: true), + form_class: "c-dropdown__item", + form: { + data: { + "turbo-frame" => "turbo-playlist", + "delegated-action" => "turbo:submit-start->playlist-songs#checkBeforePlay click->playlist-songs-bridge#playNow", + "disabled-on-native" => "true" + } + } ) %> <%= button_to( t("button.play_next"), @@ -72,6 +74,17 @@ method: :delete, form_class: "c-dropdown__item" ) %> + <%= link_to( + t("label.add_to_playlist"), + dialog_playlists_path(song_id: song.id, referer_url: current_url), + data: {"turbo-frame" => ("turbo-dialog" unless native_app?)}, + class: "c-dropdown__item" + ) %> + <%= link_to( + t("label.go_to_artist"), + artist_path(song.artist), + class: "c-dropdown__item" + ) %>
    diff --git a/app/views/playlists/songs/index.html.erb b/app/views/playlists/songs/index.html.erb index 6733c230..8f76199e 100644 --- a/app/views/playlists/songs/index.html.erb +++ b/app/views/playlists/songs/index.html.erb @@ -1,7 +1,7 @@ <% page_title_tag @playlist.name %>
    -
    +

    <%= @playlist.name %>

    @@ -14,8 +14,8 @@
    <% unless @songs.blank? %> <%= button_to( - t("button.play_all"), - current_playlist_playlist_path(@playlist), + t("button.play"), + current_playlist_playlist_path(@playlist, should_play: true), method: :put, class: "c-button c-button--primary", form_class: "u-display-inline-block", @@ -23,9 +23,7 @@ data: { "disabled-on-native" => "true", "turbo-frame" => "turbo-playlist", - "action" => "playlist-songs-bridge#playAll", - "playlist-songs-bridge-resource-type-param" => "playlist", - "playlist-songs-bridge-resource-id-param" => @playlist.id + "action" => "playlist-songs-bridge#playResource" } } ) %> diff --git a/app/views/songs/_song.html.erb b/app/views/songs/_song.html.erb index 71cd8295..47bd2e51 100644 --- a/app/views/songs/_song.html.erb +++ b/app/views/songs/_song.html.erb @@ -6,7 +6,7 @@ data: {"test-id" => "song_item"}, form: { data: { - "delegated-action" => "turbo:submit-start->playlist-songs#checkBeforePlay click->playlist-songs-bridge#playSong", + "delegated-action" => "turbo:submit-start->playlist-songs#checkBeforePlay click->playlist-songs-bridge#playNow", "turbo-frame" => "turbo-playlist", "disabled-on-native" => "true" } diff --git a/config/locales/en.yml b/config/locales/en.yml index 4afea2e3..033e1c68 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -60,12 +60,13 @@ en: button: login: 'Login' logout: 'Logout' - play_all: 'Play All' + play: 'Play' clear: 'Clear' delete: 'Delete' save: 'Save' sync: 'Sync' syncing: 'Syncing...' + play_now: 'Play Now' play_next: 'Play Next' play_last: 'Play Last' diff --git a/test/controllers/current_playlist/songs/albums_controller_test.rb b/test/controllers/current_playlist/songs/albums_controller_test.rb index 6bb009ff..c9b6b6af 100644 --- a/test/controllers/current_playlist/songs/albums_controller_test.rb +++ b/test/controllers/current_playlist/songs/albums_controller_test.rb @@ -9,9 +9,9 @@ class CurrentPlaylist::Songs::AlbumControllerTest < ActionDispatch::IntegrationT end test "should replace all songs with album songs" do - put current_playlist_album_url(albums(:album1)) + put current_playlist_album_url(albums(:album1), should_play: true) - assert_redirected_to current_playlist_songs_url(should_play_all: true) + assert_redirected_to current_playlist_songs_url(should_play: true) assert_equal albums(:album1).song_ids, @playlist.song_ids end diff --git a/test/controllers/current_playlist/songs/playlists_controller_test.rb b/test/controllers/current_playlist/songs/playlists_controller_test.rb index 3706558d..6836296a 100644 --- a/test/controllers/current_playlist/songs/playlists_controller_test.rb +++ b/test/controllers/current_playlist/songs/playlists_controller_test.rb @@ -11,9 +11,9 @@ class CurrentPlaylist::Songs::PlaylistsControllerTest < ActionDispatch::Integrat test "should replace all songs with playlist songs" do playlist = @user.playlists.create(name: "test", song_ids: [1, 2, 3]) - put current_playlist_playlist_url(playlist) + put current_playlist_playlist_url(playlist, should_play: true) - assert_redirected_to current_playlist_songs_url(should_play_all: true) + assert_redirected_to current_playlist_songs_url(should_play: true) assert_equal playlist.song_ids, @current_playlist.song_ids end diff --git a/test/system/album_test.rb b/test/system/album_test.rb index f1f49bd0..e64dfb5a 100644 --- a/test/system/album_test.rb +++ b/test/system/album_test.rb @@ -23,7 +23,7 @@ class AlbumSystemTest < ApplicationSystemTestCase login_as users(:visitor1) visit album_url(@album) - click_on "Play All" + click_on "Play" # assert current playlist have all songs in album @album.songs.each do |song| diff --git a/test/system/favorite_playlist_test.rb b/test/system/favorite_playlist_test.rb index cc9fe29c..530c1fe8 100644 --- a/test/system/favorite_playlist_test.rb +++ b/test/system/favorite_playlist_test.rb @@ -19,7 +19,7 @@ class FavoritePlaylistSystemTest < ApplicationSystemTestCase end test "play all songs in playlist" do - click_on "Play All" + click_on "Play" # assert current playlist have all songs in playlist assert_selector(:test_id, "current_playlist", visible: true) diff --git a/test/system/playlist_test.rb b/test/system/playlist_test.rb index 16f73f7c..12848386 100644 --- a/test/system/playlist_test.rb +++ b/test/system/playlist_test.rb @@ -16,7 +16,7 @@ class PlaylistSystemTest < ApplicationSystemTestCase end test "play all songs in playlist" do - click_on "Play All" + click_on "Play" # assert current playlist have all songs in playlist assert_selector(:test_id, "current_playlist", visible: true)