arkq / cmusfm

Last.fm standalone scrobbler for the cmus music player
GNU General Public License v3.0
234 stars 5 forks source link

Formatting and notes on macOS #29

Closed jef closed 4 years ago

jef commented 5 years ago

Removed hard wraps with soft wraps for Markdown style guide. Added some helpful instructions for macOS.

arkq commented 5 years ago

Removed hard wraps with soft wraps for Markdown style guide.

Which style guide you are referring? Markdown is an extension of a plain text file, so line wrapping is in fact a good idea, not bad :)

arkq commented 5 years ago

Also, why libffi is required as a (direct) dependency? Is it required by macOS?

jef commented 5 years ago

Which style guide you are referring? Markdown is an extension of a plain text file, so line wrapping is in fact a good idea, not bad :)

My style guide! Haha, I would actually love to see what type of guidelines there are out there. I just know that we can write Markdown both ways and actually the original is easier to read without soft wrap.

Google's style guild could be a good reference. If we follow that, then basically revert all my changes except for the additional macOS stuff 😅

Also, why libffi is required as a (direct) dependency? Is it required by macOS?

It was technical, but easy fix. After installing libnotify and running the configure, I got a Package 'libffi', required by 'gobject-2.0', not found. So I ran find / and got back something like this:

...
/usr/lib/libffi.dylib
find: /.Spotlight-V100: Operation not permitted
find: /Library/Application Support/com.apple.TCC: Operation not permitted
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/lib/libffi.tbd
/Library/Frameworks/Mono.framework/Versions/5.10.1/lib/pkgconfig/libffi.pc
/Library/Frameworks/Mono.framework/Versions/5.10.1/lib/libffi.la
/Library/Frameworks/Mono.framework/Versions/5.10.1/lib/libffi-3.0.13
/Library/Frameworks/Mono.framework/Versions/5.10.1/lib/libffi.dylib
/Library/Frameworks/Mono.framework/Versions/5.10.1/lib/libffi.6.dylib
...

So it seems like it comes with macOS, but for whatever reason pkg_config couldn't pick it up. I didn't try to point PKG_CONFIG_PATH to /usr/lib, but I imagine it would look in there. Could be wrong. That's the only reason I added it.

jef commented 5 years ago

Potentially could help #28 #24.

jef commented 5 years ago

This being said, I think I got everything setup correctly... I scrobble everything great, but libnotify doesn't seem to be working for me.

# authentication
user = "redcated"
key = "redacted"

# regular expressions for name parsers
format-localfile = "^(?A.+) - (?T.+)\.[^.]+$"
format-shoutcast = "^(?A.+) - (?T.+)$"
format-coverfile = "^folder\.jpg$"

now-playing-localfile = "yes"
now-playing-shoutcast = "yes"
submit-localfile = "yes"
submit-shoutcast = "yes"
notification = "yes"

# scrobbling service
service-api-url = "https://ws.audioscrobbler.com/2.0/"
service-auth-url = "https://www.last.fm/api/auth"

Separate issue, but if we can figure it out here it could be good for me to update instructions.

arkq commented 5 years ago

Google's style guild could be a good reference.

Please, follow Google style guide. Writing text in multiple lines allows git to store information about editing more precisely.

After installing libnotify and running the configure, I got a Package 'libffi', required by 'gobject-2.0', not found.

That is really strange. Cmusfm does not requires libffi as a direct dependency, so it should not be listed there. But it might be in the macOS section if it realy causes trouble.

I scrobble everything great, but libnotify doesn't seem to be working for me.

Actually I wanted to ask you whether libnotify is working for you, because once I've been looking for something on the net, and someone has been complaining that there is no good macOS experience unless one uses proper notification library (and libnotify is not right for mac OS). See this: https://github.com/guard/guard/wiki/System-notifications So if libnotify is not for macOS, I would add such information to the macOS section in readme.

jef commented 5 years ago

Please, follow Google style guide. Writing text in multiple lines allows git to store information about editing more precisely.

Great! I'll update and follow what they put there.

That is really strange. Cmusfm does not requires libffi as a direct dependency, so it should not be listed there. But it might be in the macOS section if it realy causes trouble.

Sounds good! I may not even want to include it for build process when using --enable-libnotify as it may not even be a good library. I'll reinstall today actually and see what caveats come along.

... someone has been complaining that there is no good macOS experience unless one uses proper notification library (and libnotify is not right for mac OS). ... So if libnotify is not for macOS, I would add such information to the macOS section in readme.

Let me see if I can add any library to the PR as well and see if we can get it working before we merge the doc.

Thanks for all the great feedback!

arkq commented 5 years ago

Let me see if I can add any library to the PR as well and see if we can get it working before we merge the doc.

That would be great, because I see a lot of macOS users here (which is strange, because cmus is console app which is IMHO Linux oriented).

jef commented 5 years ago

This is an easy fix... We can remove the libnotify and just add a simple line to use if we are using Darwin.

if [[ uname == "Darwin" ]]; then
    osascript -e 'display notification "Lorem ipsum dolor sit amet" with title "Title"'
end

I'll update the PR with some screenshots and tests after I'm done.

Update: Forgot it's written in C. I might make another file based or some more if statements. I have experience in C++, so hopefully not too bad...

Update: We can also use brew install terminal-notifier that can give us more options like icon and other things.

Extra links to look into: Use Applescript (no icon usage):

Use terminal-notifier. Requires dependency for macOS.

arkq commented 5 years ago

I'll go for osascript called via system(). It seems to be the simplest approach :-)

jef commented 5 years ago

Sorry this has gone stale... Anyone that wants to pick up, feel free to. I have been pretty busy with work and personal life.