From b7714eea14d78088dd3c86d123af1989761f1290 Mon Sep 17 00:00:00 2001 From: ferferga Date: Tue, 23 Jun 2020 19:09:22 +0200 Subject: [PATCH] Address review comments --- src/components/cardbuilder/card.css | 2 +- src/components/cardbuilder/cardBuilder.js | 1 - src/components/nowPlayingBar/nowPlayingBar.js | 81 +++++++++++-------- src/components/playback/playbackmanager.js | 32 ++++++-- src/components/playback/playqueuemanager.js | 39 ++++++--- .../remotecontrol/remotecontrol.css | 1 - src/components/remotecontrol/remotecontrol.js | 61 ++++++++------ src/plugins/chromecastPlayer/plugin.js | 4 +- src/plugins/sessionPlayer/plugin.js | 2 +- src/scripts/serverNotifications.js | 2 +- 10 files changed, 138 insertions(+), 87 deletions(-) diff --git a/src/components/cardbuilder/card.css b/src/components/cardbuilder/card.css index f02f1de6bf..75943a0c45 100644 --- a/src/components/cardbuilder/card.css +++ b/src/components/cardbuilder/card.css @@ -169,7 +169,7 @@ button::-moz-focus-inner { color: inherit; } -.cardImageContainer.cardScalable { +.cardScalable .cardImageContainer { height: 100%; contain: strict; } diff --git a/src/components/cardbuilder/cardBuilder.js b/src/components/cardbuilder/cardBuilder.js index f594a3dafd..b08e5024aa 100644 --- a/src/components/cardbuilder/cardBuilder.js +++ b/src/components/cardbuilder/cardBuilder.js @@ -1534,7 +1534,6 @@ import 'programStyles'; case 'MusicAlbum': return ''; case 'MusicArtist': - return ''; case 'Person': return ''; case 'Audio': diff --git a/src/components/nowPlayingBar/nowPlayingBar.js b/src/components/nowPlayingBar/nowPlayingBar.js index f0b585e9d0..b8a2a5082b 100644 --- a/src/components/nowPlayingBar/nowPlayingBar.js +++ b/src/components/nowPlayingBar/nowPlayingBar.js @@ -165,7 +165,8 @@ define(['require', 'datetime', 'itemHelper', 'events', 'browser', 'imageLoader', return; } playbackManager.seekPercent(0, currentPlayer); - // This is done automatically by playbackManager, however, setting this here gives instant visual feedback + // This is done automatically by playbackManager, however, setting this here gives instant visual feedback. + // TODO: Check why seekPercentage doesn't reflect the changes inmmediately, so we can remove this workaround. positionSlider.value = 0; } else { playbackManager.previousTrack(currentPlayer); @@ -181,11 +182,7 @@ define(['require', 'datetime', 'itemHelper', 'events', 'browser', 'imageLoader', elem.querySelector('.btnShuffleQueue').addEventListener('click', function () { if (currentPlayer) { - if (playbackManager.getQueueShuffleMode(currentPlayer) === 'Sorted') { - playbackManager.setQueueShuffleMode('Shuffle', currentPlayer); - } else { - playbackManager.setQueueShuffleMode('Sorted', currentPlayer); - } + playbackManager.toggleQueueShuffleMode(currentPlayer); } }); @@ -367,16 +364,23 @@ define(['require', 'datetime', 'itemHelper', 'events', 'browser', 'imageLoader', function updateRepeatModeDisplay(repeatMode) { toggleRepeatButtonIcon.classList.remove('repeat', 'repeat_one'); + const cssClass = 'repeatButton-active'; - if (repeatMode === 'RepeatAll') { - toggleRepeatButtonIcon.classList.add('repeat'); - toggleRepeatButton.classList.add('repeatButton-active'); - } else if (repeatMode === 'RepeatOne') { - toggleRepeatButtonIcon.classList.add('repeat_one'); - toggleRepeatButton.classList.add('repeatButton-active'); - } else { - toggleRepeatButtonIcon.classList.add('repeat'); - toggleRepeatButton.classList.remove('repeatButton-active'); + switch (repeatMode) { + case 'RepeatAll': + toggleRepeatButtonIcon.classList.add('repeat'); + toggleRepeatButton.classList.add(cssClass); + break; + case 'RepeatOne': + toggleRepeatButtonIcon.classList.add('repeat_one'); + toggleRepeatButton.classList.add(cssClass); + break; + case 'RepeatNone': + toggleRepeatButtonIcon.classList.add('repeat'); + toggleRepeatButton.classList.remove(cssClass); + break; + default: + throw new TypeError('invalid value for repeatMode'); } } @@ -530,24 +534,26 @@ define(['require', 'datetime', 'itemHelper', 'events', 'browser', 'imageLoader', var nowPlayingItem = state.NowPlayingItem; var textLines = nowPlayingItem ? nowPlayingHelper.getNowPlayingNames(nowPlayingItem) : []; - if (textLines.length > 1) { - textLines[1].secondary = true; - } - + nowPlayingTextElement.innerHTML = ''; if (textLines) { - nowPlayingTextElement.innerHTML = ''; - let itemText = document.createElement('div'); let secondaryText = document.createElement('div'); secondaryText.classList.add('nowPlayingBarSecondaryText'); - if (textLines[0].text) { - let text = document.createElement('a'); - text.innerHTML = textLines[0].text; - itemText.appendChild(text); + let itemText = document.createElement('div'); + if (textLines.length > 1) { + textLines[1].secondary = true; + if (textLines[1].text) { + let text = document.createElement('a'); + text.innerHTML = textLines[1].text; + secondaryText.appendChild(text); + } } - if (textLines[1].text) { - let text = document.createElement('a'); - text.innerHTML = textLines[1].text; - secondaryText.appendChild(text); + + if (textLines[0].text) { + if (textLines[0].text) { + let text = document.createElement('a'); + text.innerHTML = textLines[0].text; + itemText.appendChild(text); + } } nowPlayingTextElement.appendChild(itemText); nowPlayingTextElement.appendChild(secondaryText); @@ -586,11 +592,11 @@ define(['require', 'datetime', 'itemHelper', 'events', 'browser', 'imageLoader', var userData = item.UserData || {}; var likes = userData.Likes == null ? '' : userData.Likes; if (!layoutManager.mobile) { - let contextButton = document.querySelector('.nowPlayingBar').querySelector('.btnToggleContextMenu'); + let contextButton = nowPlayingBarElement.querySelector('.btnToggleContextMenu'); // We remove the previous event listener by replacing the item in each update event let contextButtonClone = contextButton.cloneNode(true); contextButton.parentNode.replaceChild(contextButtonClone, contextButton); - contextButton = document.querySelector('.nowPlayingBar').querySelector('.btnToggleContextMenu'); + contextButton = nowPlayingBarElement.querySelector('.btnToggleContextMenu'); let options = { play: false, queue: false, @@ -637,10 +643,15 @@ define(['require', 'datetime', 'itemHelper', 'events', 'browser', 'imageLoader', let context = nowPlayingBarElement; let toggleShuffleButton = context.querySelector('.btnShuffleQueue'); - if (shuffleMode === 'Sorted') { - toggleShuffleButton.classList.remove('shuffleQueue-active'); - } else if (shuffleMode === 'Shuffle') { - toggleShuffleButton.classList.add('shuffleQueue-active'); + switch (shuffleMode) { + case 'Sorted': + toggleShuffleButton.classList.remove('shuffleQueue-active'); + break; + case 'Shuffle': + toggleShuffleButton.classList.add('shuffleQueue-active'); + break; + default: + throw new TypeError('invalid value for shuffleMode'); } } diff --git a/src/components/playback/playbackmanager.js b/src/components/playback/playbackmanager.js index 38ce9bf9f1..3bd4be9c0c 100644 --- a/src/components/playback/playbackmanager.js +++ b/src/components/playback/playbackmanager.js @@ -3886,7 +3886,7 @@ define(['events', 'datetime', 'appSettings', 'itemHelper', 'pluginManager', 'pla 'GoToSearch', 'DisplayMessage', 'SetRepeatMode', - 'SetQueueShuffleMode', + 'SetShuffleQueue', 'PlayMediaSource', 'PlayTrailers' ]; @@ -3941,9 +3941,7 @@ define(['events', 'datetime', 'appSettings', 'itemHelper', 'pluginManager', 'pla return this._playQueueManager.getRepeatMode(); }; - PlaybackManager.prototype.setQueueShuffleMode = function (value, player) { - - player = player || this._currentPlayer; + PlaybackManager.prototype.setQueueShuffleMode = function (value, player = this._currentPlayer) { if (player && !enableLocalPlaylistManagement(player)) { return player.setQueueShuffleMode(value); } @@ -3952,9 +3950,7 @@ define(['events', 'datetime', 'appSettings', 'itemHelper', 'pluginManager', 'pla events.trigger(player, 'shufflequeuemodechange'); }; - PlaybackManager.prototype.getQueueShuffleMode = function (player) { - - player = player || this._currentPlayer; + PlaybackManager.prototype.getQueueShuffleMode = function (player = this._currentPlayer) { if (player && !enableLocalPlaylistManagement(player)) { return player.getQueueShuffleMode(); } @@ -3962,6 +3958,26 @@ define(['events', 'datetime', 'appSettings', 'itemHelper', 'pluginManager', 'pla return this._playQueueManager.getShuffleMode(); }; + PlaybackManager.prototype.toggleQueueShuffleMode = function (player = this._currentPlayer) { + let currentvalue; + if (player && !enableLocalPlaylistManagement(player)) { + currentvalue = player.getQueueShuffleMode(); + switch (currentvalue) { + case 'Shuffle': + player.setQueueShuffleMode('Sorted'); + break; + case 'Sorted': + player.setQueueShuffleMode('Shuffle'); + break; + default: + throw new TypeError('current value for shufflequeue is invalid'); + } + } else { + this._playQueueManager.toggleShuffleMode(); + } + events.trigger(player, 'shufflequeuemodechange'); + }; + PlaybackManager.prototype.trySetActiveDeviceName = function (name) { name = normalizeName(name); @@ -4030,7 +4046,7 @@ define(['events', 'datetime', 'appSettings', 'itemHelper', 'pluginManager', 'pla case 'SetRepeatMode': this.setRepeatMode(cmd.Arguments.RepeatMode, player); break; - case 'SetQueueShuffleMode': + case 'SetShuffleQueue': this.setQueueShuffleMode(cmd.Arguments.ShuffleMode, player); break; case 'VolumeUp': diff --git a/src/components/playback/playqueuemanager.js b/src/components/playback/playqueuemanager.js index 4eac7d7c76..03f5246b89 100644 --- a/src/components/playback/playqueuemanager.js +++ b/src/components/playback/playqueuemanager.js @@ -60,16 +60,12 @@ define([], function () { PlayQueueManager.prototype.shufflePlaylist = function () { this._sortedPlaylist = []; - let currentPlaylistItem = this._playlist[this.getCurrentPlaylistIndex()]; - for (let item of this._playlist) { + for (const item of this._playlist) { this._sortedPlaylist.push(item); } + const currentPlaylistItem = this._playlist.splice(this.getCurrentPlaylistIndex(), 1)[0]; for (let i = this._playlist.length - 1; i > 0; i--) { - if (this._playlist[i] === currentPlaylistItem) { - this._playlist.splice(i, 1); - continue; - } const j = Math.floor(Math.random() * i); const temp = this._playlist[i]; this._playlist[i] = this._playlist[j]; @@ -216,7 +212,8 @@ define([], function () { }; PlayQueueManager.prototype.setRepeatMode = function (value) { - if (value === 'RepeatOne' || value === 'RepeatAll' || value === 'RepeatNone') { + const repeatModes = ['RepeatOne', 'RepeatAll', 'RepeatNone']; + if (repeatModes.includes(value)) { this._repeatMode = value; } else { throw new TypeError('invalid value provided for setRepeatMode'); @@ -228,12 +225,28 @@ define([], function () { }; PlayQueueManager.prototype.setShuffleMode = function (value) { - if (value === 'Sorted') { - this.sortShuffledPlaylist(); - } else if (value === 'Shuffle') { - this.shufflePlaylist(); - } else { - throw new TypeError('invalid value provided for setShuffleMode'); + switch (value) { + case 'Shuffle': + this.shufflePlaylist(); + break; + case 'Sorted': + this.sortShuffledPlaylist(); + break; + default: + throw new TypeError('invalid value provided to setShuffleMode'); + } + }; + + PlayQueueManager.prototype.toggleShuffleMode = function () { + switch (this._shuffleMode) { + case 'Shuffle': + this.setShuffleMode('Sorted'); + break; + case 'Sorted': + this.setShuffleMode('Shuffle'); + break; + default: + throw new TypeError('current value for shufflequeue is invalid'); } }; diff --git a/src/components/remotecontrol/remotecontrol.css b/src/components/remotecontrol/remotecontrol.css index 69f7025d6e..d4511a9dd7 100644 --- a/src/components/remotecontrol/remotecontrol.css +++ b/src/components/remotecontrol/remotecontrol.css @@ -208,7 +208,6 @@ .layout-mobile .playlistSectionButton .btnSavePlaylist { margin: 0; - padding-right: 0; border-radius: 0; } diff --git a/src/components/remotecontrol/remotecontrol.js b/src/components/remotecontrol/remotecontrol.js index 6626d83c49..89598d09a8 100644 --- a/src/components/remotecontrol/remotecontrol.js +++ b/src/components/remotecontrol/remotecontrol.js @@ -123,7 +123,7 @@ define(['browser', 'datetime', 'backdrop', 'libraryBrowser', 'listView', 'imageL var artistsSeries = ''; var albumName = item.Album; if (item.ArtistItems != null) { - for (let artist of item.ArtistItems) { + for (const artist of item.ArtistItems) { let artistName = artist.Name; let artistId = artist.Id; artistsSeries += `${artistName}`; @@ -135,7 +135,7 @@ define(['browser', 'datetime', 'backdrop', 'libraryBrowser', 'listView', 'imageL // For some reason, Chromecast Player doesn't return a item.ArtistItems object, so we need to fallback // to normal item.Artists item. // TODO: Normalise fields returned by all the players - for (let artist of item.Artists) { + for (const artist of item.Artists) { artistsSeries += `${artist}`; if (artist !== item.Artists.slice(-1)[0]) { artistsSeries += ', '; @@ -176,7 +176,6 @@ define(['browser', 'datetime', 'backdrop', 'libraryBrowser', 'listView', 'imageL maxHeight: 300 * 2 }) : null; - console.debug('updateNowPlayingInfo'); let contextButton = context.querySelector('.btnToggleContextMenu'); // We remove the previous event listener by replacing the item in each update event let contextButtonClone = contextButton.cloneNode(true); @@ -355,18 +354,30 @@ define(['browser', 'datetime', 'backdrop', 'libraryBrowser', 'listView', 'imageL function updateRepeatModeDisplay(repeatMode) { var context = dlg; let toggleRepeatButtons = context.querySelectorAll('.repeatToggleButton'); + const cssClass = 'repeatButton-active'; + let innHtml = ''; + let repeatOn = undefined; - for (let toggleRepeatButton of toggleRepeatButtons) { - if ('RepeatAll' == repeatMode) { - toggleRepeatButton.innerHTML = ""; - toggleRepeatButton.classList.add('repeatButton-active'); - } else if ('RepeatOne' == repeatMode) { - toggleRepeatButton.innerHTML = ""; - toggleRepeatButton.classList.add('repeatButton-active'); + switch (repeatMode) { + case 'RepeatAll': + break; + case 'RepeatOne': + innHtml = ''; + break; + case 'RepeatNone': + repeatOn = null; + break; + default: + throw new TypeError('invalid value for repeatMode'); + } + + for (const toggleRepeatButton of toggleRepeatButtons) { + if (repeatOn === null) { + toggleRepeatButton.classList.remove(cssClass); } else { - toggleRepeatButton.innerHTML = ""; - toggleRepeatButton.classList.remove('repeatButton-active'); + toggleRepeatButton.classList.add(cssClass); } + toggleRepeatButton.innerHTML = innHtml; } } @@ -516,10 +527,15 @@ define(['browser', 'datetime', 'backdrop', 'libraryBrowser', 'listView', 'imageL let shuffleButtons = context.querySelectorAll('.btnShuffleQueue'); for (let shuffleButton of shuffleButtons) { - if ('Sorted' === shuffleMode) { - shuffleButton.classList.remove('shuffleQueue-active'); - } else if ('Shuffle' === shuffleMode) { - shuffleButton.classList.add('shuffleQueue-active'); + switch (shuffleMode) { + case 'Sorted': + shuffleButton.classList.remove('shuffleQueue-active'); + break; + case 'Shuffle': + shuffleButton.classList.add('shuffleQueue-active'); + break; + default: + throw new TypeError('invalid shuffle mode'); } } onPlaylistUpdate(); @@ -708,14 +724,10 @@ define(['browser', 'datetime', 'backdrop', 'libraryBrowser', 'listView', 'imageL playbackManager.fastForward(currentPlayer); } }); - for (let shuffleButton of context.querySelectorAll('.btnShuffleQueue')) { + for (const shuffleButton of context.querySelectorAll('.btnShuffleQueue')) { shuffleButton.addEventListener('click', function () { if (currentPlayer) { - if (playbackManager.getQueueShuffleMode(currentPlayer) === 'Sorted') { - playbackManager.setQueueShuffleMode('Shuffle', currentPlayer); - } else { - playbackManager.setQueueShuffleMode('Sorted', currentPlayer); - } + playbackManager.toggleQueueShuffleMode(currentPlayer); } }); } @@ -728,7 +740,8 @@ define(['browser', 'datetime', 'backdrop', 'libraryBrowser', 'listView', 'imageL return; } playbackManager.seekPercent(0, currentPlayer); - // This is done automatically by playbackManager. However, setting this here gives instant visual feedback + // This is done automatically by playbackManager. However, setting this here gives instant visual feedback. + // TODO: Check why seekPercentage doesn't reflect the changes inmmediately, so we can remove this workaround. positionSlider.value = 0; } else { playbackManager.previousTrack(currentPlayer); @@ -794,7 +807,7 @@ define(['browser', 'datetime', 'backdrop', 'libraryBrowser', 'listView', 'imageL } else { context.querySelector('.playlist').classList.add('hide'); context.querySelector('.btnSavePlaylist').classList.add('hide'); - if (showMuteButton && showVolumeSlider) { + if (showMuteButton || showVolumeSlider) { context.querySelector('.volumecontrol').classList.remove('hide'); } if (layoutManager.mobile) { diff --git a/src/plugins/chromecastPlayer/plugin.js b/src/plugins/chromecastPlayer/plugin.js index 0809fb82c3..6384ce7690 100644 --- a/src/plugins/chromecastPlayer/plugin.js +++ b/src/plugins/chromecastPlayer/plugin.js @@ -652,7 +652,7 @@ define(['appSettings', 'userSettings', 'playbackManager', 'connectionManager', ' 'SetSubtitleStreamIndex', 'DisplayContent', 'SetRepeatMode', - 'SetQueueShuffleMode', + 'SetShuffleQueue', 'EndSession', 'PlayMediaSource', 'PlayTrailers' @@ -897,7 +897,7 @@ define(['appSettings', 'userSettings', 'playbackManager', 'connectionManager', ' options: { ShuffleMode: value }, - command: 'SetQueueShuffleMode' + command: 'SetShuffleQueue' }); }; diff --git a/src/plugins/sessionPlayer/plugin.js b/src/plugins/sessionPlayer/plugin.js index 5ed07bb4ff..6a266941d7 100644 --- a/src/plugins/sessionPlayer/plugin.js +++ b/src/plugins/sessionPlayer/plugin.js @@ -508,7 +508,7 @@ define(['playbackManager', 'events', 'serverNotifications', 'connectionManager'] SessionPlayer.prototype.setQueueShuffleMode = function (mode) { - sendCommandByName(this, 'SetQueueShuffleMode', { + sendCommandByName(this, 'SetShuffleQueue', { ShuffleMode: mode }); }; diff --git a/src/scripts/serverNotifications.js b/src/scripts/serverNotifications.js index c1d9c95e9c..cddd2cf794 100644 --- a/src/scripts/serverNotifications.js +++ b/src/scripts/serverNotifications.js @@ -65,7 +65,7 @@ define(['connectionManager', 'playbackManager', 'syncPlayManager', 'events', 'in case 'SetRepeatMode': playbackManager.setRepeatMode(cmd.Arguments.RepeatMode); break; - case 'SetQueueShuffleMode': + case 'SetShuffleQueue': playbackManager.setQueueShuffleMode(cmd.Arguments.ShuffleMode); break; case 'VolumeUp':