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: Refactor weighted-settings.js #2318

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Oct 16, 2023

What is this fixing or adding?

This moves most of the infrastructure into two classes:

  • WeightedSettings covers the settings page as a whole. It tracks the
    user's current settings in local storage as well as the game data
    from the server so they don't need to be manually passed around from
    function to function.

  • GameSettings covers the settings for a single game, and provides a
    view of the current settings and the game data just for that game.

How was this tested?

Manually tested adjusting settings, reloading from local storage, exporting to YAML, etc.

@nex3 nex3 force-pushed the class-refactor branch 2 times, most recently from badf79f to ca4810d Compare October 16, 2023 05:01
This moves most of the infrastructure into two classes:

* WeightedSettings covers the settings page as a whole. It tracks the
  user's current settings in local storage as well as the game data
  from the server so they don't need to be manually passed around from
  function to function.

* GameSettings covers the settings for a single game, and provides a
  view of the current settings and the game data just for that game.
@LegendaryLinux LegendaryLinux self-assigned this Oct 16, 2023
@ThePhar ThePhar changed the title Refactor weighted-settings.js WebHost: Refactor weighted-settings.js Oct 17, 2023
@ThePhar ThePhar added is: refactor/cleanup Improvements to code/output readability or organizization. affects: webhost Issues/PRs that touch webhost and may need additional validation. labels Oct 17, 2023
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.

Aside from moving everything into classes, what benefit do we gain from this? This doesn't necessarily improve readability, and there is no new functionality present.

@nex3
Copy link
Contributor Author

nex3 commented Oct 17, 2023

In addition to being slightly more efficient thanks to loading from localStorage less frequently, it makes it a lot easier to access data and settings for the current game when building each block of settings for each game. For example, instead of having to pass settingData.games[game].gameLocations into buildWeightedSettingsDiv(), buildLocationsDiv(), and buildHintsDiv(), each of these functions can now access them through this.data.gameLocations.

This is particularly helpful when adding new data that needs to be passed through these functions. For example, I have a work-in-progress commit built on this that makes location_name_groups and item_name_groups available in location and item lists through the web UI (they're already available to YAML):

image

and another that adds the ability for worlds to provide descriptions for individual items and locations (the tooltip shows up on hover of course):

image

Both of these are much easier if you can just directly access the data from this.data rather than having to pipe it through the various functions. I'll send them out later, but I wanted to make sure this refactoring approach was good first.

@LegendaryLinux
Copy link
Member

LegendaryLinux commented Oct 17, 2023

I was trying to prevent change for the sake of change. I'm down if you have work in the pipeline which depends on these features. I realized as I was typing this I forgot to test whether changing a single setting and reloading the page preserves the setting change. So I'll do that later tonight, and merge if all is good.

@LegendaryLinux
Copy link
Member

During some testing, I discovered a bug. The item pool quantities are not preserved upon a reload, and will always revert to 1. When exporting a .yaml file, the quantities are always listed as 1, even if the input contains a different quantity. The games used for testing are:

  • The Legend of Zelda: A Link to the Past
  • Factorio

Link to the Past is visible in the screenshots below.

As seen on the weighted-settings page:
image

As seen in the exported .yaml file:
image

@nex3
Copy link
Contributor Author

nex3 commented Oct 17, 2023

Oops, fixed! I'd missed one call to updateItemSetting() in a callback. I did a search to make sure there weren't any other update...() calls that I hadn't touched.

@nex3 nex3 requested a review from LegendaryLinux October 18, 2023 21:19
@LegendaryLinux LegendaryLinux merged commit 38c9ee1 into ArchipelagoMW:main Oct 18, 2023
10 checks passed
@nex3 nex3 deleted the class-refactor branch October 18, 2023 23:58
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
* Refactor weighted-settings.js

This moves most of the infrastructure into two classes:

* WeightedSettings covers the settings page as a whole. It tracks the
  user's current settings in local storage as well as the game data
  from the server so they don't need to be manually passed around from
  function to function.

* GameSettings covers the settings for a single game, and provides a
  view of the current settings and the game data just for that game.

* Fix item count updating
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
* Refactor weighted-settings.js

This moves most of the infrastructure into two classes:

* WeightedSettings covers the settings page as a whole. It tracks the
  user's current settings in local storage as well as the game data
  from the server so they don't need to be manually passed around from
  function to function.

* GameSettings covers the settings for a single game, and provides a
  view of the current settings and the game data just for that game.

* Fix item count updating
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: webhost Issues/PRs that touch webhost and may need additional validation. is: refactor/cleanup Improvements to code/output readability or organizization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants