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

Reintroduce legacy map pool command aliases #1090

Merged
merged 1 commit into from
Nov 3, 2022
Merged

Conversation

alexsosnovsky
Copy link
Contributor

Removing the "rotation", "rot" aliases for the map pool command was a counterintuitive change. A rotation is still a supported type of organized map collection, and not all PGM servers may utilize map pools. There are also a lot of people who are familiar with using the old aliases.
Having the rotation versions of the commands available to use doesn't negatively impact the plugin in any way, either.

@Pablete1234
Copy link
Member

Pablete1234 commented Nov 2, 2022

A rotation is still a supported type of organized map collection, and not all PGM servers may utilize map pools.

Rotation is still a type of pool but not all pools are rots. Using /rots should only show rotation pools instead of other pools?

I'd be fine if reintroducing it like that if you're willing to make the behaviour changes so that the output is consistent with the naming.

Keep in mind #1080 will conflict with this

@alexsosnovsky
Copy link
Contributor Author

Rotation is still a type of pool but not all pools are rots. Using /rots should only show rotation pools instead of other pools?

I'd be fine if reintroducing it like that if you're willing to make the behaviour changes so that the output is consistent with the naming.

This is a case where propagating the internal code structure to the end user interface doesn't necessarily result in the best user experience. Yes, certainly rotations in PGM were refactored several years ago to be a subtype of a map pool. But mapmakers and people hosting and/or administrating PGM servers don't necessarily know that. From the end users' perspective, voted pools, shuffled rotations, and ordered rotations are simply options for map organization, with no inheritance implied. Many people using these commands in the PGM community at large today have been in the community since well before map pools were introduced at all, and we are accustomed to using /rot, /rots, and /setrot. Removing these commands forces us to readjust to less familiar ones, going against years of experience. This is in spite of us not perceiving any dominance of map pools versus rotations, unless the specific user has explored that specific section of the PGM codebase.

I see the re-addition of the rotation-based aliases as a user experience issue. Just look at how many people submitted positive reactions to this pull request both on GitHub and in the PGM discord. We want to be able to use the commands we are accustomed to, even if it no longer aligns 100% with how the objects in PGM are structured. Nothing is gained by removing these aliases; it does not hurt PGM or its maintainers at all to keep them. Modifying the commands to not return voted pools could introduce the potential for significant confusion, given that it would be the same commands but different functionality from their prior implementations. If such a filtered command is desired, it could likely be better added as a flag of the regular /pools & /rots command.

Keep in mind #1080 will conflict with this

I assume the cloud framework supports aliases? If that PR will be merged soon, then this can certainly wait and I would update it for the new command definitions.

Copy link
Member

@Pablete1234 Pablete1234 left a comment

Choose a reason for hiding this comment

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

I believe keeping the commands in does nothing but fuel precisely the wrong perception you're describing, of users not knowing pools and rots are different things.

If they are to be reintroduced they should not be just aliases, but should instead either warn the user of deprecation, announcing it's for removal and that they should migrate to using pool commands, or be restricted to interact just with rotations, as the name implies.

Both could also be done where the command is restricted go interact with rots, and the user is informed about pool commands to interact with the rest of the pools.

@alexsosnovsky
Copy link
Contributor Author

I believe keeping the commands in does nothing but fuel precisely the wrong perception you're describing, of users not knowing pools and rots are different things.

That wasn't really the point of me pointing out the users' perception. Users I imagine typically understand the purpose of voted pools, shuffled rotations, and ordered rotations, and are successfully able to select one that suits their needs. My point was that to perform this task successfully, it is not pertinent for the user to be acutely aware that a rotation is a type of pool, because this knowledge would not affect their ability to use the three types of pools effectively.

If they are to be reintroduced they should not be just aliases, but should instead either warn the user of deprecation, announcing it's for removal and that they should migrate to using pool commands, or be restricted to interact just with rotations, as the name implies.
Both could also be done where the command is restricted go interact with rots, and the user is informed about pool commands to interact with the rest of the pools.

The first suggestion, to warn the user of deprecation and removal, does not maximize the long term user experience. Rather than being re-added several months after removal just to be removed again later, which seems awkward and confusing at best, my intention is for the commands to return permanently. If you would like to steer new users away from utilizing them, I would support removing these aliases from /help. However, as I have taken great care to explain, the command aliases remaining usable does not negatively affect users in the long term, especially those actually using these commands on a regular basis.

The second suggestion, I explained in my previous comment why it would be confusing for users. But to reiterate, while it would technically be better than what we currently have, there will now be users utilizing the reintroduced command and not understanding why certain "pools" aren't showing up if they have a config with diverse types of pools. This would introduce more confusion than simply re-adding the original aliases without modifying their functionality. If there is a desire for a command that only shows non-voted pools, I maintain that the cleanest implementation would be a flag applied to the existing command.

@Pablete1234
Copy link
Member

Pablete1234 commented Nov 3, 2022

I would support removing these aliases from /help

There's no simple way to do that, as the command library is what will handle help generation, which makes it part of the issue.

I like the idea of a flag (-t or --type for type of pool), where rot will take a default value for it.

Ie: /pools -> all pools
/pools -t voted -> only voted pools are shown
/rots -> equivalent to /pools -t rotation

Make the header component clearly state it if there's a type filter applied, ie: Showing 5 pools (filter: rotation) or similar.

Copy link
Member

@Electroid Electroid left a comment

Choose a reason for hiding this comment

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

I didn’t realize these were removed.

This is a case where propagating the internal code structure to the end user interface doesn't necessarily result in the best user experience.

I strongly agree. I understand the reasons to remove them, but I think that is outweighed by what end users want.

Originally this was removed, but reinstated to wait until a new release. But I think the feedback has been overwhelming to keep it the same.

@Electroid Electroid merged commit 50bcb06 into PGMDev:dev Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants