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

feat: Add onlinemembers and player command #2412

Merged
merged 6 commits into from
Apr 18, 2024

Conversation

ShadowCat117
Copy link
Contributor

@ShadowCat117 ShadowCat117 commented Apr 17, 2024

/onlinemembers <guildName> shows the current online members sorted into guild ranks. It supports entering guild name or prefix, case insensitive though so if there are multiple matches it will return whatever is found first. (Players in /stream are shown as offline in the API but still show in guild total hence 12 online here but it says 13)
image

/player guild <username> shows the guild a player is in, their rank and how long they have been in the guild for.
image
image

/player lastseen <username> shows if a player is currently online and their current server, or when they were last online if they are currently offline.
image
image

The /player commands also have support for the multi selectors where there can be multiple players with the same name in the API. Clicking a UUID will run the respective command with the UUID to get the correct result. As guild names are unique this shouldn't be necessary for /onlinemembers
image

Wynntils/Static-Storage#108

Copy link
Collaborator

@kristofbolyai kristofbolyai left a comment

Choose a reason for hiding this comment

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

I think this looks good. I only wonder if there the command parsing should be moved from the command, to a Service (or a Model..? This is in a gray zone between the two). We've not done anything like this before, so we have not figured out where commands like this really fit into our overall design. Since these are going to be the only commands of this kind, Overall, I think we should hold off on refactoring this into a new higher-level abstraction of some kind, until this type of command appears a few more times.

@magicus
Copy link
Member

magicus commented Apr 17, 2024

I only wonder if there the command parsing should be moved from the command, to a Service (or a Model..?

I think this looks okay..? Maybe I skimmed the code a bit too quick, but I did not understand what you were reacting to. All our commands implement (or extend, I do not remember) a Command baseclass (or interface), and we have some general framework to hook into the vanilla command thingy. It is somewhat complex, and I have been thinking about making it easier, but it is difficult. Even though it is a bit heavy-handed, the vanilla system is rather well-designed and it is hard to make anything much better, at least as long as you want it to be Java-only.

@ShadowCat117
Copy link
Contributor Author

I only wonder if there the command parsing should be moved from the command, to a Service (or a Model..?

I think this looks okay..? Maybe I skimmed the code a bit too quick, but I did not understand what you were reacting to.

I may be wrong but I interpreted it as the parsing of the json into the response for the user to see should ideally be handled elsewhere, usually when we make calls to athena or download static data it's all handled inside a Model or Service, but this is done inside the command

@kristofbolyai
Copy link
Collaborator

I only wonder if there the command parsing should be moved from the command, to a Service (or a Model..?

I think this looks okay..? Maybe I skimmed the code a bit too quick, but I did not understand what you were reacting to.

I may be wrong but I interpreted it as the parsing of the json into the response for the user to see should ideally be handled elsewhere, usually when we make calls to athena or download static data it's all handled inside a Model or Service, but this is done inside the command

Yes, this is what I meant.

@kristofbolyai kristofbolyai merged commit d6e9e3f into Wynntils:fuygg_features Apr 18, 2024
1 check passed
@magicus
Copy link
Member

magicus commented Apr 18, 2024

Aha, I see. I checked too quickly before. Yes, I agree. It should most likely be a model, according to the idea that a model captures something "Wynncraft knows about", and what we get from their API is by definition something they know about.

@kristofbolyai
Copy link
Collaborator

Aha, I see. I checked too quickly before. Yes, I agree. It should most likely be a model, according to the idea that a model captures something "Wynncraft knows about", and what we get from their API is by definition something they know about.

Sure. If @JamieCallan117 wants to refactor this now, go ahead, if not, I think it is not a pressing issue until we need those API calls for other features.

@ShadowCat117
Copy link
Contributor Author

I won't do it now, want to get the filter screen done and it's giving me some trouble but after that I can

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

3 participants