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

WebHost: unify references to options #2037

Merged
merged 14 commits into from
Oct 24, 2023

Conversation

alwaysintreble
Copy link
Collaborator

What is this fixing or adding?

Unify references to options in the webhost as options, instead of using options and settings interchangeably.

How was this tested?

Running webhost and viewing the changes. Weighted options page is currently broken so putting in draft for now.

If this makes graphical changes, please attach screenshots.

Screenshot_49
Screenshot_51
Screenshot_52

@ThePhar ThePhar added is: documentation Improvements or additions to documentation. affects: webhost Issues/PRs that touch webhost and may need additional validation. is: refactor/cleanup Improvements to code/output readability or organizization. affects: core Issues/PRs that touch core and may need additional validation. labels Jul 26, 2023
@MrRedstone54
Copy link

On the third image it shows "download a options file." This should be, "download an options file". Since this draft is about language, might as well make sure the grammar is correct.

@alwaysintreble alwaysintreble marked this pull request as ready for review July 30, 2023 20:17
@alwaysintreble
Copy link
Collaborator Author

@LegendaryLinux

@LegendaryLinux
Copy link
Member

Semantically there is a difference between a setting and an option. A setting has both a name and a set of options.

I also don't want to change the title of the pages away from "Player Settings" and "Weighted Settings."

In your first image, the only inconsistency is the header "Game Options", which should be changed to "Game Settings." The rest is correct as written.

Copy link
Member

@LegendaryLinux LegendaryLinux left a comment

Choose a reason for hiding this comment

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

See my above comment.

@alwaysintreble
Copy link
Collaborator Author

Semantically there is a difference between a setting and an option. A setting has both a name and a set of options.

I also don't want to change the title of the pages away from "Player Settings" and "Weighted Settings."

In your first image, the only inconsistency is the header "Game Options", which should be changed to "Game Settings." The rest is correct as written.

So, semantically, I agree. I should've more clearly outlined the goal/reasoning in the original post so that's on me. Previously, in the API we've been kind of using settings and options interchangeably, but as of 827444f this is no longer the case. We now have a clear cut line in the actual Archipelago API on wh at an "option" and a "setting" are, used to define 2 separate interactions, where an option is a player submitted choice for their world, and a setting is the host configured choices for the multiworld, and the local settings used to configure their Archipelago installation (ROM paths etc). Primary goal of this PR is to unify these terms so that any references to options are called options, and references to settings are settings. I'm open to compromise if you still believe it, but context is important here.

Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

The change to use consistent language is very welcome from a my point of view. When people talk about "player options" it should be clear that the game's option api will be used. When people talk about "game settings" it should be clear that we talk about the game's settings api/host.yaml.

"Player settings" is unclear in that regard.

WebHostLib/misc.py Outdated Show resolved Hide resolved
Copy link
Member

@LegendaryLinux LegendaryLinux left a comment

Choose a reason for hiding this comment

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

Finally found time to go through this. Looks okay to me.

# Conflicts:
#	WebHostLib/static/styles/player-options.css
# Conflicts:
#	WebHostLib/misc.py
#	WebHostLib/static/assets/player-options.js
WebHostLib/misc.py Fixed Show fixed Hide fixed
WebHostLib/misc.py Outdated Show resolved Hide resolved
# Conflicts:
#	WebHostLib/static/assets/weighted-options.js
@black-sliver
Copy link
Member

Hm, we use " in AP, not '. Can you change the changed strings? I realize that my suggestion was wrong - have been working on a different code base at the time :S

@alwaysintreble
Copy link
Collaborator Author

I did ' not because of your suggestion, but because most of that file uses '. Not all of it does though so I suppose this could go either way.

@black-sliver
Copy link
Member

I think the decision we reached a while ago was to change all touched strings from ' to ", but not change strings that are not touched.

WebHostLib/misc.py Outdated Show resolved Hide resolved
WebHostLib/misc.py Outdated Show resolved Hide resolved
Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

Have not checked js and css changes, only py and checked the links and redirects

@black-sliver black-sliver merged commit 7641285 into ArchipelagoMW:main Oct 24, 2023
12 checks passed
nex3 added a commit to nex3/Archipelago that referenced this pull request Oct 31, 2023
The request for the JSON file that provides the setting data was missed during the rename in ArchipelagoMW#2037, so prior to this the weighted settings page wasn't rendering at all.
@nex3 nex3 mentioned this pull request Oct 31, 2023
LegendaryLinux pushed a commit that referenced this pull request Oct 31, 2023
The request for the JSON file that provides the setting data was missed during the rename in #2037, so prior to this the weighted settings page wasn't rendering at all.
LegendaryLinux pushed a commit that referenced this pull request Oct 31, 2023
* Factor out a common function for building lists

* Expose name groups through the weighted-settings UI

* Fix weighted-settings page

The request for the JSON file that provides the setting data was missed during the rename in #2037, so prior to this the weighted settings page wasn't rendering at all.
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
* WebHost: unify references to options

* it was just an extra s the whole time...

* grammar

* redirect from old pages

* redirect stuff correctly

* use url_for

* use " for modified strings

* remove redirect cache

* player_settings

* update site map
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
The request for the JSON file that provides the setting data was missed during the rename in ArchipelagoMW#2037, so prior to this the weighted settings page wasn't rendering at all.
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
…lagoMW#2327)

* Factor out a common function for building lists

* Expose name groups through the weighted-settings UI

* Fix weighted-settings page

The request for the JSON file that provides the setting data was missed during the rename in ArchipelagoMW#2037, so prior to this the weighted settings page wasn't rendering at all.
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
* WebHost: unify references to options

* it was just an extra s the whole time...

* grammar

* redirect from old pages

* redirect stuff correctly

* use url_for

* use " for modified strings

* remove redirect cache

* player_settings

* update site map
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
The request for the JSON file that provides the setting data was missed during the rename in ArchipelagoMW#2037, so prior to this the weighted settings page wasn't rendering at all.
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
…lagoMW#2327)

* Factor out a common function for building lists

* Expose name groups through the weighted-settings UI

* Fix weighted-settings page

The request for the JSON file that provides the setting data was missed during the rename in ArchipelagoMW#2037, so prior to this the weighted settings page wasn't rendering at all.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. affects: webhost Issues/PRs that touch webhost and may need additional validation. is: documentation Improvements or additions to documentation. is: refactor/cleanup Improvements to code/output readability or organizization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants