re-write #36
re-write #36
Conversation
|
Looks good, could you add the timer pause feature when the song gets paused tho? Also, when I do anything on Spotify, it takes around 10 seconds to update on Discord. |
app.js
Outdated
| }); | ||
| } | ||
| try { | ||
| const path = process.platform === 'win32' ? 'C:\\Windows\\System32\\drivers\\etc\\hosts' : '/hosts/etc'; |
There was a problem hiding this comment.
Not everyone has it on C drive, plus there's one more win32 system that allows to change install path + it's /etc/hosts for Linux
app.js
Outdated
| activity.smallImageKey = image; | ||
| if (song) { | ||
| activity.details = `🎵 ${song.title}`; | ||
| activity.state = `👤 ${song.artist.name}`; |
There was a problem hiding this comment.
What if song.artist is undefined, never happens on normal songs, but would happen on local files.
app.js
Outdated
| activity.details = `🎵 ${song.title}`; | ||
| activity.state = `👤 ${song.artist.name}`; | ||
| activity.largeImageText = `🔗 ${song.id}`; | ||
| activity.smallImageText = `💿 ${song.album.id}`; |
There was a problem hiding this comment.
Same as song.artist, this can cause troubles on local files.
app.js
Outdated
| log(`Song state updated (playing: ${song.playing})`) | ||
| client.on('ready', () => { | ||
| log(`Connected to Discord! (${cfg.id})`); | ||
| setInterval(() => client.setActivity(activity), 15e3); |
There was a problem hiding this comment.
What if the user changes song? It would create quite a noticeable gap.
|
|
||
| class Song { | ||
| constructor(data) { | ||
| this.title = data.track.track_resource.name; |
There was a problem hiding this comment.
Add checks for data incase of local files.
Previous method of setting the presence every 15 seconds was removed, instead it just checks how long it's been since you last updated it.
cernodile
left a comment
cernodile
left a comment
There was a problem hiding this comment.
You've mitigated the primary issues. There are some still problems remaining, good job though.
app.js
Outdated
| **/ | ||
| try { | ||
| const path = process.platform === 'win32' ? 'C:\\Windows\\System32\\drivers\\etc\\hosts' : '/hosts/etc'; | ||
| const path = process.platform === 'win32' ? 'C:\\Windows\\System32\\drivers\\etc\\hosts' : '/etc/hosts'; |
There was a problem hiding this comment.
What about win32, not every installation is at C:\ nor \Windows. Use %SystemRoot% to your advantage.
app.js
Outdated
| time = Date.now(); | ||
| }, timeLeft); | ||
| } else { | ||
| client.setActivity(activity); |
There was a problem hiding this comment.
"else it'll set a timeout until we can", yet it sets activity either way?
There was a problem hiding this comment.
nvm, OP clarified this to me in Discord. I misunderstood.
cernodile
left a comment
cernodile
left a comment
There was a problem hiding this comment.
Other than spotify.js, this rewrite seems clean enough now.
|
Will test & merge tomorrow :) |
|
Timer still doesn't disappear when paused for me & theres still a delay when I do anything. |
4 participants
tell me if you dont like anything ( or dont like everything )