davicorreiajr / spotify-now-playing

Spotify now playing information and control popup for macOS menu bar
MIT License
216 stars 11 forks source link

Feature: Song information in menu bar #30

Closed incognitojam closed 5 years ago

incognitojam commented 5 years ago

What does this PR do?

Adds the option to show song information in the menu bar. screen shot 2019-02-01 at 01 28 52 It can be toggled from the right-click menu: screen shot 2019-02-01 at 01 29 21 If you turn it off it looks like like before!

davicorreiajr commented 5 years ago

hey @IncognitoJam, thank you very much for you contribution!!

About the title in the menu bar, the problem with using simply the tray.setTitle() is that if the title is too big for the current bar, the icon of Spotify - now playing simply disappear and you can't access the app. So let's say you use VSCode (which has a lot of options in the menu bar), put some more icons in the menu bar (in my case, Postgres, battery %, bluetooth, calendar date) and then listen to a music with a huge name and some artists (you can take Young, Wild and Free as example), the app's icon simply disappear.

When I first tried this, I thought to put only the song name. However, this behaviour is REALLY bad and I think it's not worth risking it.

I'm really busy these weeks, otherwise I could help you to find a solution, but I think we need to find a way to limit the tray title size, which I couldn't find when I tried this.

kaufmann42 commented 5 years ago

What if the size was limited and scrolled?

Simple Examples https://jsfiddle.net/keg29a4h/

const artist = 'Drake';
const song = 'One Dance (REMIX)';
const title = `${artist} - ${song}`;
const stringLength = 10;
let start = 0;
let end = stringLength - 1;
let inc = 0;
setInterval(function() {
    tray.setTitle(title.substring(start + inc, end + inc));
  inc = (inc + 1) % (title.length - stringLength + 2);
}, 500);
davicorreiajr commented 5 years ago

Sorry for the long time without answer!

@kaufmann42 this is a really good solution! Some comments:

@IncognitoJam do you continue with this PR?

incognitojam commented 5 years ago

Sure, I'm happy to take another look at it! I've moved to YouTube Music now... but I can install Spotify to test this out :)

incognitojam commented 5 years ago

To tell if the song title changed, I just kept a copy as previousTitle and compared it each time getCurrentPlayback was executed - if it changed I set index back to zero.

What do you think about reducing the constant UPDATE_PERIOD to 1000ms to make the scrolling smoother?

davicorreiajr commented 5 years ago

@IncognitoJam awesome!!

About the period, seems good, I don't know the exact implications, I think it might be worth it trying.

incognitojam commented 5 years ago

Here is what the end result looks like. I'm not completely happy with it because the Spotify icon moves from left to right as the width of the title text changes. Anyone have any ideas to solve this?

davicorreiajr commented 5 years ago

Really good! I don't know how to solve the thing with the icon either, maybe we should just do it this way, and fix in a future if we find a solution.

Some comments though: 1) when the title has length less or equal than stringLength, we don't need to get the substring (otherwise the icon will look like is moving); 2) I think it's better to put this stringLength as a constant in the constants file, don't you think?; 3) the safest way to check if the song has changed is to compare with currentPlaybackURI (kind of the song id), doing data.uri !== currentPlaybackURI, which is also done in shouldShowTrackNotification. So maybe we should create a didSongChange method to be used in both cases, right? And then, I think we need to update currentPlaybackURI every time and not only if(shouldShowTrackNotification(mappedData)).

I think that after fixing these we are ready to go.

incognitojam commented 5 years ago

Hi @davicorreiajr , please could you review the latest commit? Hopefully it matches your requirements and we can get this merged! 😄

davicorreiajr commented 5 years ago

@IncognitoJam amazing, great job!! Sorry for so many comments. I'm gonna merge and deploy asap