-
Notifications
You must be signed in to change notification settings - Fork 11
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.
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
Allow Discord status to NOT update when stopped/paused #14
base: master
Are you sure you want to change the base?
Conversation
- Add setting for "SendStatusWhenPausedOrStopped" that instructs Discord that the application is no longer 'active'. Defaulted to false which matches Spotify behavior; only reporting something when actively playing. - Settings dialog updated for this setting with checkbox. - Default setting stored in settings.ini - Discord RPC disconnected when in pause/stop setting and reconnected. - Connection status reported by Discord callback itself.
Thanks for doing this! Overall looks good, but I'll take a closer look this weekend when I'm on my dev computer. |
No problem! Hopefully it is to your standards; I tried using the same code formatting and style and modified as little as possible to achieve this. If my other PR is merged I will rebase this against |
Bleh, I was trying to use GitHub Desktop to merge and remove the VS Code related PR that was merged into master but looks like I need to rebase to remove that extra clutter from this PR. Still getting the hang of that... |
} | ||
|
||
void ReportCurrentSongStatus(PlaybackState playbackState) | ||
{ | ||
if (!g_presenceInfo.HasDiscordModuleLoaded()) | ||
return; | ||
assert(playbackState != Stopped); | ||
|
||
if (g_pluginSettings.ApplicationID == "0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment about pre-existing- not a requested action
Been meaning to change this. I hate unnecessary string compares
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you talking about the ApplicationID check?
I'd like to expose a 'validate' function or similar that gate the connection... or even whisk all of the 'get connected' or 'get disconnected' into a wrapper function to encapsulate that logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the ApplicationID check. For some reason it highlighted the wrong line. Not trying to de-rail your PR, this is probably another thing for another day.
DiscordRichPresence/SettingsFile.cpp
Outdated
//std::wstring stemp = s2ws(executablePath); | ||
//MessageBox(0, stemp.c_str(), L"", MB_OK); | ||
//https://stackoverflow.com/a/27296 | ||
std::wstring s2ws(const std::string& s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I need to put that into a personal dev scratchpad. I used it to show the folder paths as my settings.ini
can't be updated by the plugin and I was diagnosing that (and ran into the evil/fun aspects of C++ and strings). Unable to solve my setting file save issues though... :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha. No worries
{ | ||
if (!g_presenceInfo.HasDiscordModuleLoaded()) | ||
{ | ||
if (g_pluginSettings.ApplicationID == "0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eesh, indeed.
VS Code isn't formatting and I am doing it by hand. Missed this.
I did a quick test of this change locally and it looks good! Thank you @derekgates :) |
Indeed. I have this problem at work as well; if you chain a PR on another pending PR then the history gets crazy. This time I used GitHub Desktop and made a branch from a previous branch, I expected to be able to rebase against |
That's a tough one. How I would fix it personally is do a branch reset back to where the branch was created off of the remote, then create 1 or 2 new clean commits, then force push. All to your private branch. This has the power to effectively delete commits and re-write history. Re-writing history could cause weirdness with the PR but that's okay. I know how to do the above in Git Extensions, that's the client I'm used to, but unfamiliar with other clients. If you prefer I can merge this new and fix it up on your behalf. |
I haven't forgotten about this PR or project. I cleaned up the PR for recommendations and have been attempting some git rebase work to try to clean up the history. At this point I think a squash would be helpful... the merge to |
Hey, coming back to this- I'm still interested in taking this PR- if it's alright with you, I was going to try and merge it this evening. Yeah, as a squash commit. Although in general i'm not an extreme stickler for clean commit history esp for small projects like this |
I used my previous branch work for VS Code integration (as that is how I am developing this), so this PR looks a little funky until that is merged. My apologies.
There is a new screenshot of the dialog included in Images that can be linked in the README.md (and the link to COLLABORATING.md also needs fixing I realized).
fixes #12