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

Rewrite presence a little bit #1853

Merged
merged 13 commits into from
Sep 2, 2017
Merged

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Aug 30, 2017

Please describe the changes this PR makes and why it should be merged:
Adds support for the richest of presence, switches to new presence caching

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@devsnek devsnek changed the title such presence many good Rewrite presence a little bit Aug 30, 2017
} else {
this.set(data.user.id, new Presence(this.client, data));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

You also have to return the presence here (all the other stores do)

src/index.js Outdated
@@ -33,7 +33,7 @@ module.exports = {
Collector: require('./structures/interfaces/Collector'),
DMChannel: require('./structures/DMChannel'),
Emoji: require('./structures/Emoji'),
Game: require('./structures/Presence').Game,
Activity: require('./structures/Presence').Activity,
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be moved up.

* Assets for rich presence
* @type {?RichPresenceAssets}
*/
this.assets = data.assets ? new RichPresenceAssets(data.assets) : null;
Copy link
Member

Choose a reason for hiding this comment

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

This should be: new RichPresenceAssets(this, data.assets)

/**
* Assets for a rich presence
*/
class RichPresenceAssets {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this class be exported?

}

async setClientPresence({ status, since, afk, activity }) {
const applicationID = activity.application ? activity.application.id || activity.application : null;
Copy link
Member

Choose a reason for hiding this comment

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

Omitting optional parameter activity throws an error here, for examply by using ClientUser#setAFK.

async setClientPresence({ status, since, afk, activity }) {
const applicationID = activity.application ? activity.application.id || activity.application : null;
let assets = new Collection();
if (activity.assets && applicationID) {
Copy link
Member

Choose a reason for hiding this comment

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

Same error with activity being undefined here.

since: since != null ? since : null, // eslint-disable-line eqeqeq
status: status || this.clientPresence.status,
game: activity ? {
type: Constants.ActivityTypes.indexOf(activity.type || 'PLAYING') || activity.type || 0,
Copy link
Member

@SpaceEEC SpaceEEC Aug 30, 2017

Choose a reason for hiding this comment

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

You can no longer pass a numeric type here (except 0). because indexOf returns -1 in that case which is not falsy.

* @param {?string} game Game being played
* @param {Object} [options] Options for setting the game
* Sets the activity the client user is playing.
* @param {?string} name activity being played
Copy link
Member

Choose a reason for hiding this comment

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

Activity

Copy link
Member Author

Choose a reason for hiding this comment

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

?

Copy link
Member

@SpaceEEC SpaceEEC Aug 30, 2017

Choose a reason for hiding this comment

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

Uppercase A instead of a lowercase one.

);
}
}

/**
* Represents a game that is part of a user's presence.
* Represents a activity that is part of a user's presence.
Copy link
Member

Choose a reason for hiding this comment

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

an activity

*/
smallImageURL({ format, size } = {}) {
if (!this.smallImage) return null;
return this.client.rest.cdn
Copy link
Member

Choose a reason for hiding this comment

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

This class does not have a client property.

*/
largeImageURL({ format, size } = {}) {
if (!this.largeImage) return null;
return this.client.rest.cdn
Copy link
Member

Choose a reason for hiding this comment

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

Same here, no client property.

@@ -92,6 +92,21 @@ class Activity {
this.applicationID = data.application_id || null;

/**
* Timestamps for the activity
* @type {Object}
Copy link
Member

@SpaceEEC SpaceEEC Aug 30, 2017

Choose a reason for hiding this comment

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

This is nullable.
And maybe add @property here to describe what it actually is?


/**
* Party of the activity
* @type {Object}
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

@@ -40,6 +40,8 @@ class ClientPresenceStore extends PresenceStore {
large_image: assets.get(activity.assets.largeImage) || activity.assets.largeImage,
small_image: assets.get(activity.assets.smallImage) || activity.assets.smallImage,
} : undefined,
timestamps: activity.timestamps || undefined,
Copy link
Member

Choose a reason for hiding this comment

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

This should convert the Date object back to a unix timestamp.

Copy link
Member Author

Choose a reason for hiding this comment

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

not needed

type: Constants.ActivityTypes.indexOf(activity.type || 'PLAYING') || activity.type || 0,
name: activity.name,
url: activity.url,
description: activity.description,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure myself but I tested it a bit and I think it needs to be 'details' and not 'description'

@amishshah amishshah merged commit c4df250 into discordjs:master Sep 2, 2017
@devsnek devsnek deleted the good-presence branch September 2, 2017 22:57
iCrawl pushed a commit that referenced this pull request Sep 3, 2017
… into their date stores (#1869)

* refactor/fix(DataStores): move instantiating of store related classes into the relevant create methods and fixed FetchMemberOptions

* Renamed ChannelStore's constructor parameter for claritiy

* define client property before creating items from passed iterable to avoid the client property being undefined

* Fix fetching members with a limit

* add static Channel#create back and moved channel instantiating there

* make ChannelStore's constructor accept an iterable

* make iterable an optional parameter, allow to just pass client and options

* fixed a little typo in docs of <User>.displayAvatarURL() (#1872)

* Rewrite presence a little bit (#1853)

* such presence many good

* Update PresenceStore.js

* Update index.js

* Update ClientPresenceStore.js

* Update Presence.js

* Update ClientPresenceStore.js

* Update ClientUser.js

* Update Presence.js

* add timestamps and party

* Update Presence.js

* Update PresenceStore.js

* Update ClientPresenceStore.js

* Update ClientPresenceStore.js

* fix: made options.type optional in ClientUser#setActivity (#1875)

* fix: made options.type optional in ClientUser#setActivity

* some changes, smol fix

due to too many types being possible, it should just be defaulted to 0. this means streamers will have to set the type manually, though.
also mistake with activity.name check

* docs(Collection): Adjust exists documentation (#1876)

Since the return value of Collection#exists is basically just a boolean of Collection#find's result, the same documentation and arguments can and should be applied.

* refactor(Emoji): remove Guild#deleteEmoji in favour of Emoji#delete (#1877)

* refactor/fix(DataStores): move instantiating of store related classes into the relevant create methods and fixed FetchMemberOptions

* Renamed ChannelStore's constructor parameter for claritiy

* define client property before creating items from passed iterable to avoid the client property being undefined

* Fix fetching members with a limit

* add static Channel#create back and moved channel instantiating there

* make ChannelStore's constructor accept an iterable

* make iterable an optional parameter, allow to just pass client and options

* rebase, this time with saving the file

* address issue with falsy activity throwing errors when setting client's presence
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.

None yet

5 participants