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.

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

Fix discord clearActivity, menu, listen along option #380

Merged
merged 8 commits into from
Oct 18, 2021
Merged

Fix discord clearActivity, menu, listen along option #380

merged 8 commits into from
Oct 18, 2021

Conversation

cpiber
Copy link
Contributor

@cpiber cpiber commented Aug 15, 2021

The callback sends multiple events, in particular two pause when going to the next song, so the timeout wasn't properly cleared. I have documented this.

Added menu buttons for the two options. Currently for the timeout time I simply open the config, to allow the user to properly input a number here a library is needed.

Now also clears activity directly if timeout enabled and set to 0.

The callback sends multiple events, in particular two pause when going to the
next song, so the timeout wasn't properly cleared.
Add menu buttons for the two options
@cpiber
Copy link
Contributor Author

cpiber commented Aug 26, 2021

Currently we also don't handle (see #132 (reply in thread))

  • Discord not running when starting (never connects)
  • Discord closing (silently does nothing), does not reconnect

Supporting those would however require quite a bit of rewriting, is this in-scope? Should I do it?

@Araxeus
Copy link
Collaborator

Araxeus commented Aug 26, 2021

  • Discord not running when starting (never connects)
  • Discord closing (silently does nothing), does not reconnect

It seems to me that it would require constant listening to other process / (which is too much IMO)
unless you have a nice and easy solution?

(I can't really find documentation about events at https://discord.js.org/#/docs/rpc/master/class/RPCClient)

@cpiber
Copy link
Contributor Author

cpiber commented Aug 26, 2021

The RPC library emits a disconnected event

For the reconnecting, I would simply add a button to the menu, I agree that checking if discord is running periodically is too much

For the events, you kinda have to look through https://github.com/discordjs/RPC/blob/master/src/client.js, but most of the events are just carried on from discord itself

@Araxeus
Copy link
Collaborator

Araxeus commented Aug 26, 2021

A button for reconnecting sounds neat 😎

@cpiber
Copy link
Contributor Author

cpiber commented Aug 27, 2021

Currently, if the plugin fails to connect, there is no message to the user (neither on start nor on reconnect). Is this behaviour desirable or do you think an alert would be better?

@Araxeus
Copy link
Collaborator

Araxeus commented Aug 27, 2021

Maybe show a message only when the reconnect button is clicked and it fails
(which would require some distinction between connect() and reconnect() or maybe just take an arg that indicate that its currently an initial connection or reconnect)

I don't think its necessary on startup, since you could easily start youtube-music with discord off - and it shouldn't be an error
(but if you explicitly click on reconnect button and no discord is active - seems like an error)

Great work btw :)

config/store.js Show resolved Hide resolved
@cpiber cpiber changed the title Fix discord clearActivity, menu Fix discord clearActivity, menu, listen along option Oct 11, 2021
@avarayr
Copy link

avarayr commented Oct 18, 2021

@th-ch could you please review and merge?

Copy link
Owner

@th-ch th-ch left a comment

Choose a reason for hiding this comment

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

Did not fully test it, but diff looks good, thanks for the contribution! ✅

@th-ch th-ch merged commit 8114a28 into th-ch:master Oct 18, 2021
@cpiber cpiber deleted the discord branch October 18, 2021 22:22
@Araxeus
Copy link
Collaborator

Araxeus commented Oct 19, 2021

@cpiber Could you please explain the thinking process behind this export?

module.exports.info = Object.defineProperties({}, Object.keys(info).reduce((o, k) => ({ ...o, [k]: { enumerable: true, get: () => info[k] } }), {}));

why can't you just export something like

module.exports.isConnected = () => info.rpc !== null

@cpiber
Copy link
Contributor Author

cpiber commented Oct 19, 2021

why can't you just export something like

I had it differently before (used more from info), of course that's way cleaner now 😅

@Araxeus
Copy link
Collaborator

Araxeus commented Oct 19, 2021

Could you open a small pr to clean this up? if you're busy I can do it np

@cpiber
Copy link
Contributor Author

cpiber commented Oct 19, 2021

Will do ^^

th-ch added a commit that referenced this pull request Oct 24, 2021
Discord plugin: Clean Up Export (follow-up #380)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants