Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bug(core/playback/mpv): jukebox mode under windows - #2767 #2774

Merged
merged 11 commits into from
Apr 14, 2024
Merged

bug(core/playback/mpv): jukebox mode under windows - #2767 #2774

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
eb65688
bug(core/playback/mpv): jukebox mode under windows - #2767
apkatsikas Jan 7, 2024
b97bf61
bug(core/playback/mpv): jukebox mode under windows - #2767
apkatsikas Jan 7, 2024
bf29e59
Merge master
apkatsikas Apr 12, 2024
93bb3ce
bug(core/playback/mpv): jukebox mode under windows - navidrome#2767
apkatsikas Apr 12, 2024
0d5d74b
bug(core/playback/mpv): jukebox mode under windows - navidrome#2767
apkatsikas Apr 12, 2024
f3b54f0
bug(core/playback/mpv): jukebox mode under windows - navidrome#2767
apkatsikas Apr 12, 2024
243b529
bug(core/playback/mpv): jukebox mode under windows - navidrome#2767
apkatsikas Apr 12, 2024
f066adc
bug(core/playback/mpv): jukebox mode under windows - navidrome#2767
apkatsikas Apr 14, 2024
e7186a4
bug(core/playback/mpv): jukebox mode under windows - navidrome#2767
apkatsikas Apr 14, 2024
e8a6042
bug(core/playback/mpv): jukebox mode under windows - navidrome#2767
apkatsikas Apr 14, 2024
36568dc
Unexport SocketName, use socketName in NewTrack
apkatsikas Apr 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion core/playback/mpv/track.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ package mpv
import (
"fmt"
"os"
"runtime"
"time"

"github.com/dexterlb/mpvipc"
Expand All @@ -32,7 +33,7 @@ func NewTrack(playbackDoneChannel chan bool, deviceName string, mf model.MediaFi
return nil, err
}

tmpSocketName := utils.TempFileName("mpv-ctrl-", ".socket")
tmpSocketName := utils.RandomSocketOrFileName("mpv-ctrl-", ".socket")

args := createMPVCommand(mpvComdTemplate, deviceName, mf.Path, tmpSocketName)
exe, err := start(args)
Expand Down Expand Up @@ -104,6 +105,11 @@ func (t *MpvTrack) Pause() {
}

func (t *MpvTrack) Close() {
// Windows automatically handles closing
// and cleaning up named pipe
if runtime.GOOS == "windows" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not a good practice to use runtime.GOOS in your files (unless it is a really small check). Usually we create one file for each variation and use build tags to control which version to use.

For an example, see scanner/metadata/taglib/get_filename.go and scanner/metadata/taglib/get_filename_win.go

In your case I'd have two Close() methods in different files, one for windows (do nothing) and the other for all other platforms with the actual code.

Let me know if it makes sense to you or if you want me to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah was wondering about this and hoping to get feedback on what best practice is for this project. makes sense and thank you for the examples - will give it a shot and report back

return
}
log.Debug("Closing resources", "track", t)
t.CloseCalled = true
// trying to shutdown mpv process using socket
Expand Down
2 changes: 1 addition & 1 deletion scanner/metadata/taglib/taglib_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ var _ = Describe("Extractor", func() {
// Only run permission tests if we are not root
RegularUserContext("when run without root privileges", func() {
BeforeEach(func() {
accessForbiddenFile = utils.TempFileName("access_forbidden-", ".mp3")
accessForbiddenFile = utils.RandomSocketOrFileName("access_forbidden-", ".mp3")

f, err := os.OpenFile(accessForbiddenFile, os.O_WRONLY|os.O_CREATE, 0222)
Expect(err).ToNot(HaveOccurred())
Expand Down
4 changes: 3 additions & 1 deletion ui/src/personal/LastfmScrobbleToggle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ const Progress = (props) => {
)
const callbackUrl = `${window.location.origin}${callbackEndpoint}`
openedTab.current = openInNewTab(
`https://www.last.fm/api/auth/?api_key=${localStorage.getItem('lastfm-apikey')}&cb=${callbackUrl}`,
`https://www.last.fm/api/auth/?api_key=${localStorage.getItem(
'lastfm-apikey',
)}&cb=${callbackUrl}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was required to run prettier before I could push due to git hooks.

)
}, [])

Expand Down
11 changes: 9 additions & 2 deletions utils/files.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,17 @@ package utils
import (
"os"
"path/filepath"
"runtime"

"github.com/google/uuid"
)

func TempFileName(prefix, suffix string) string {
return filepath.Join(os.TempDir(), prefix+uuid.NewString()+suffix)
func RandomSocketOrFileName(prefix, suffix string) string {
Copy link
Member

@deluan deluan Apr 13, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather leave this function (TempFileName) alone and create a SocketName in the core/playback/mpv package, also selectable by build tags

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

socketPath := os.TempDir()
// Windows needs to use a named pipe instead of a file for the socket
// see https://mpv.io/manual/master#using-mpv-from-other-programs-or-scripts
if runtime.GOOS == "windows" {
socketPath = `\\.\pipe\mpvsocket`
}
return filepath.Join(socketPath, prefix+uuid.NewString()+suffix)
}