UI for the music player #399

heyhippari · 2020-12-16T10:57:42Z

This adds a working music UI and a basic audio player using Shaka as a backend (to be replaced with Howler or something else in the future).

camc314 · 2020-12-16T13:24:46Z

layouts/default.vue

+    <v-footer v-if="false" app>
+      <music-player />
+    </v-footer>


This needs a higher z-index, otherwise, home sections overlay ontop of it.

a value of 5 works well, and it does not overlay on the nav draw

camc314

Probably a good idea to hide volume controls on narrower screens

ThibaultNocchi · 2020-12-16T16:44:18Z

I do agree, especially when narrower screens are tablets or phones having dedicated volume buttons. But I think that's what the second screenshot show, doesn't it?

camc314 · 2020-12-16T16:53:05Z

I do agree, especially when narrower screens are tablets or phones having dedicated volume buttons. But I think that's what the second screenshot show, doesn't it?

yeah that's right. The second screenshot in the original is a phone. My screenshot is a desktop window at about 50% width

ThibaultNocchi · 2020-12-16T17:39:44Z

Oh yeah you're right, my bad. I don't know if totally hiding is a great idea, maybe only letting the icon and show a vertical slide above when hovering?

heyhippari · 2020-12-27T02:29:05Z

Alright, so this evolved a bit, with the recent merge of the playback manager, and now has a basic Shaka-based audio player (basically a copy of VideoPlayer, but with an <audio> tag. It shall be replaced with the Howler-based player we have planned in the future) and shows information on currently playing audio, as well as allow some basic control and linking to artist/album.

It also adds a few niceties, like an itemHelper mixin, containing a canPlay method, which checks if the current item is playable, and shows the currently playing track in tracklists (I'd like to refine this in the future, so only the source of the currently playing queue highlights this currently playing item, much like Spotify does).

Still no queue button, fullscreen, shuffle or repeat, since that's for Preview Release 2, probably.

KristupasSavickas · 2020-12-27T12:37:22Z

Minor issue I noticed - play button remains in paused state after transitioning to the next playlist item:

ferferga · 2021-01-01T01:21:02Z

components/AudioControls.vue

+          <v-slider
+            :value="currentPosition"
+            hide-details
+            min="0"


Suggested change

min="0"

:min="0"

This shouldn't be necessary anyway

The component accepts number | string, so this isn't needed.

ferferga · 2021-01-01T01:31:04Z

@MrTimscampi After a rebase this is good to go imo. I made some quick small improvements to the slider, so it updates more smoothly (right now it makes a huge jump). I also added a nice label with the selected time and now the bar doesn't act like crazy when clicked or moved:

If you want I can make a commit before your rebase so we can get this changes here or I can make a PR with cumulative improvements later, whatever you prefer :)

camc314

There is an empty file LikeButton.vue that can be removed

camc314 · 2021-01-01T06:37:11Z

components/AudioControls.vue

+              :to="`/item/${getCurrentItem.AlbumId}`"
+              >{{ getCurrentItem.Name }}</nuxt-link
+            >
+            <v-btn class="d-none d-md-inline-flex" icon>


Suggested change

<v-btn class="d-none d-md-inline-flex" icon>

<v-btn class="d-none d-md-inline-flex" icon disabled>

camc314 · 2021-01-01T06:38:24Z

components/AudioControls.vue

+              class="text-truncate link"
+              :to="`/item/${getCurrentItem.AlbumId}`"
+              >{{ getCurrentItem.Name }}</nuxt-link
+            >


Suggested change

class="text-truncate link"

:to="`/item/${getCurrentItem.AlbumId}`"

>{{ getCurrentItem.Name }}</nuxt-link

>

class="text-truncate link"

:to="`/item/${getCurrentItem.AlbumId}`"

>

{{ getCurrentItem.Name }}

</nuxt-link>

camc314 · 2021-01-01T06:39:23Z

components/AudioControls.vue

+          <nuxt-link
+            tag="span"
+            class="text--secondary text-caption text-truncate mt-md-n2 link"
+            :to="`/artist/${getCurrentItem.AlbumArtists[0].Id}`"


Isn't this making the assumption that there is an album artist.

In the case where there isn't, won't this produce an error

I always have an album artist, even for albums from solo artists.

camc314 · 2021-01-01T06:41:51Z

components/AudioPlayer.vue

+<template>
+  <audio
+    ref="audioPlayer"
+    :poster="poster"


Suggested change

:poster="poster"

There is nowhere to show the poster, so this is unneeded.

camc314 · 2021-01-01T06:42:14Z

components/AudioPlayer.vue

+    poster(): string {
+      return this.getImageUrlForElement(ImageType.Backdrop, {
+        itemId:
+          this.getCurrentItem?.ParentBackdropItemId ||
+          this.getCurrentItem?.SeriesId ||
+          this.getCurrentItem?.Id
+      });
+    }


Suggested change

poster(): string {

return this.getImageUrlForElement(ImageType.Backdrop, {

itemId:

this.getCurrentItem?.ParentBackdropItemId ||

this.getCurrentItem?.SeriesId ||

this.getCurrentItem?.Id

});

}

camc314 · 2021-01-01T06:45:41Z

components/TrackList.vue

@@ -38,7 +41,13 @@
            </td>
            <td style="width: 3em" class="pr-0 pl-0 text-center">
              <v-btn icon>


Suggested change

<v-btn icon>

<v-btn icon disabled>

camc314

Settings/metadata is inaccessible on narrow screens:

neopc10 · 2021-01-01T15:42:07Z

Why the fullscreen button? It seems to me that it's a bit of a waste of space. On Tidal, for instance, clicking anywhere on the minibar will go to the full window player. It seems to me that JF would be well served by this. The add to playlist button, too, to perhaps an overflow menu.

heyhippari · 2021-01-01T16:09:53Z

@neopc10 We are planning to have this kind of screen for the fuill screen button:

While the queue button will navigate to the current queue.

@camc314 That's z-index stuff with the navigation drawer. Afaik it's fixed on master, but this branch needs to be rebased.

@KristupasSavickas I've seen this happen with videos too, so it's not specific to this branch. Definitely needs to be investigated, though.

components/AudioControls.vue

hamburglar2160 · 2021-01-02T01:17:09Z

I really like what Tidal does here with their UX seems to borrow at least a bit from that.

Spotify doesn't seem to give two craps about albums in general and I think that's a missed opportunity on their end. I think the Vue client would really benefit from having the album linked in addition to the song title and album artist. as posted above. Tidal actually has a "playing from " link instead of album and I think that's a really nice touch that would be applicable here in the future.

Tidal also has a fantastic "whole window" player UI, which I don't think is being considered for this round of PRs, but certainly deserves serious consideration for the Vue client: the album cover, player, and metadata on the left 50% of the screen and the queue on the right 50%. On Tidal's "whole window" UI there's also a button that leads to a "fullscreen" UI that I would argue is inferior to the Spotify one @MrTim linked above.

Clearly the fullscreen button is placeholding for the Spotify-like fullscreen view. Is there an intention to do a whole window player UI as well? I would argue that the absolute sweet spot for flow in this client would be "play music -> minibar appears -> click anything that's not a button on the minibar to navigate to the tidal-like "whole window" player -> click fullscreen button there to get to a spotify-like fullscreen UI.

I think making the seekbar take up only a fraction of the minibar as opposed to the entire width of the screen is a downgrade from what jf-web has. Having that more granular control I think is useful. I see that Spotify chose to do that though and it certainly works with their fullscreen view.

I'm bringing this up here and now only because the PR is just slightly less of a placeholder than it might have been an hour ago.

ferferga · 2021-01-02T01:35:45Z

@hamburglar2160 All this discussion belongs to #238, please keep this one on topic with this specific feature.

This is also a first round design. Home Header's first PR was really basic and it has gotten better after a few PRs. After all, this whole client is a Work in Progress.

heyhippari · 2021-01-02T12:58:37Z

Indeed, please keep this on topic. PRs are mainly for implementation discussions, not brainstorming future features. We have discussion threads for every major feature specifically for gathering ideas and discussing said features and what we want to do with them.

Also, @hamburglar2160 , please avoid tagging people unrelated to the project. It was probably done accidentally, but it's quite a rude thing to do in general.

codecov-io · 2021-01-02T13:37:50Z

Codecov Report

Merging #399 (71d91c4) into master (1030175) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##           master    #399      +/-   ##
=========================================
- Coverage    0.67%   0.62%   -0.05%     
=========================================
  Files          74      77       +3     
  Lines        1932    2073     +141     
  Branches      273     294      +21     
=========================================
  Hits           13      13              
- Misses       1918    2059     +141     
  Partials        1       1

Impacted Files	Coverage Δ
components/Item/Card.vue	`0.00% <0.00%> (ø)`
components/Layout/AudioControls.vue	`0.00% <0.00%> (ø)`
components/Layout/HomeHeader/HomeHeaderItems.vue	`0.00% <0.00%> (ø)`
components/Layout/VolumeSlider.vue	`0.00% <0.00%> (ø)`
components/Players/AudioPlayer.vue	`0.00% <0.00%> (ø)`
components/Players/PlayerManager.vue	`0.00% <ø> (ø)`
components/Players/TrackList.vue	`0.00% <0.00%> (ø)`
pages/genre/_itemId/index.vue	`0.00% <0.00%> (ø)`
pages/item/_itemId/index.vue	`0.00% <0.00%> (ø)`
store/playbackManager.ts	`0.00% <0.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1030175...71d91c4. Read the comment docs.

ferferga

Minor stuff

components/Layout/AudioControls.vue

components/Layout/VolumeSlider.vue

components/Players/AudioPlayer.vue

components/Players/TrackList.vue

sonarqubecloud · 2021-01-02T14:47:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

camc314 reviewed Dec 16, 2020

View reviewed changes

heyhippari force-pushed the music-player-ui branch 2 times, most recently from 3113e69 to 8a53ebc Compare December 27, 2020 02:27

heyhippari force-pushed the music-player-ui branch from 8a53ebc to bb208c0 Compare December 27, 2020 02:35

ferferga reviewed Jan 1, 2021

View reviewed changes

camc314 reviewed Jan 1, 2021

View reviewed changes

components/AudioControls.vue Outdated Show resolved Hide resolved

ferferga changed the title ~~Add placeholder UI for the music player~~ UI for the music player Jan 1, 2021

heyhippari and others added 2 commits January 2, 2021 14:04

feat(music-player): add music player

d22ae0f

fix(audiocontrol): improve slider behaviour

6ac128b

heyhippari force-pushed the music-player-ui branch 2 times, most recently from c36b0b9 to eac1b8a Compare January 2, 2021 13:35

ferferga reviewed Jan 2, 2021

View reviewed changes

heyhippari force-pushed the music-player-ui branch from eac1b8a to c63ca26 Compare January 2, 2021 13:58

fix(music-player): address review comments

71d91c4

ferferga force-pushed the music-player-ui branch from c63ca26 to 71d91c4 Compare January 2, 2021 14:46

ferferga approved these changes Jan 2, 2021

View reviewed changes

heyhippari merged commit 6bb48a0 into master Jan 2, 2021

heyhippari deleted the music-player-ui branch January 2, 2021 14:52

camc314 mentioned this pull request Jan 2, 2021

feat(video osd): Add OSD for video playback #477

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI for the music player #399

UI for the music player #399

heyhippari commented Dec 16, 2020 •

edited

Loading

camc314 Dec 16, 2020

camc314 Dec 16, 2020

camc314 left a comment

ThibaultNocchi commented Dec 16, 2020 •

edited

Loading

camc314 commented Dec 16, 2020

ThibaultNocchi commented Dec 16, 2020

heyhippari commented Dec 27, 2020

KristupasSavickas commented Dec 27, 2020

ferferga Jan 1, 2021

heyhippari Jan 2, 2021

ferferga commented Jan 1, 2021

camc314 left a comment

camc314 Jan 1, 2021

camc314 Jan 1, 2021

camc314 Jan 1, 2021

heyhippari Jan 2, 2021 •

edited

Loading

camc314 Jan 1, 2021

camc314 Jan 1, 2021

camc314 Jan 1, 2021

camc314 left a comment

neopc10 commented Jan 1, 2021

heyhippari commented Jan 1, 2021 •

edited

Loading

hamburglar2160 commented Jan 2, 2021

ferferga commented Jan 2, 2021 •

edited

Loading

heyhippari commented Jan 2, 2021

codecov-io commented Jan 2, 2021 •

edited

Loading

ferferga left a comment

sonarqubecloud bot commented Jan 2, 2021

	<v-btn class="d-none d-md-inline-flex" icon>
	<v-btn class="d-none d-md-inline-flex" icon disabled>

UI for the music player #399

UI for the music player #399

Conversation

heyhippari commented Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camc314 left a comment

Choose a reason for hiding this comment

ThibaultNocchi commented Dec 16, 2020 • edited Loading

camc314 commented Dec 16, 2020

ThibaultNocchi commented Dec 16, 2020

heyhippari commented Dec 27, 2020

KristupasSavickas commented Dec 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferferga commented Jan 1, 2021

camc314 left a 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

heyhippari Jan 2, 2021 • 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

camc314 left a comment

Choose a reason for hiding this comment

neopc10 commented Jan 1, 2021

heyhippari commented Jan 1, 2021 • edited Loading

hamburglar2160 commented Jan 2, 2021

ferferga commented Jan 2, 2021 • edited Loading

heyhippari commented Jan 2, 2021

codecov-io commented Jan 2, 2021 • edited Loading

Codecov Report

ferferga left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 2, 2021

heyhippari commented Dec 16, 2020 •

edited

Loading

ThibaultNocchi commented Dec 16, 2020 •

edited

Loading

heyhippari Jan 2, 2021 •

edited

Loading

heyhippari commented Jan 1, 2021 •

edited

Loading

ferferga commented Jan 2, 2021 •

edited

Loading

codecov-io commented Jan 2, 2021 •

edited

Loading