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

Develop - 3.1.52 #1521

Merged
merged 44 commits into from
Nov 4, 2024
Merged

Develop - 3.1.52 #1521

merged 44 commits into from
Nov 4, 2024

Conversation

ebkr
Copy link
Owner

@ebkr ebkr commented Nov 4, 2024

No description provided.

anttimaki and others added 30 commits October 18, 2024 12:20
Event listening is not needed and makes following the code
unnecessarily difficult. Instead, trigger the import process baased on
user interactions.
Thanks to ImmutableProfiles, we can defer changing the active profile
until the import process completes.
Both "IMPORT" and "UPDATE" are actually imports, so use CREATE instead.

Drop the empty null value. The implementation never checks if the value
is empty, so this is less misleading.
We already wholly depend on the components internal state, so passing
the profile name via wrappers makes little difference.
Remove the result check since it's done anyway in the
importProfileHandler().
- Make button ids unique
- Remove unnecessary if-checks
As the "create" and "update" paths are mutually exclusive, might as
well just have one variable for the profile name.
Originally user could close the profile import progress modal and start
importing another profile. Option to close the modal was removed when
it was detected that this lead to weird behaviour when active profile
was changed while an import was in progress. Some of the code related
to this feature was left behind in the hopes that it could later be
fixed and brought back.

ImmutableProfiles were introduced, and it in fact seems they fix the
immediate issues with changing the profile. However, I decided the
feature won't be coming back anytime soon based on the following:

- It seems unlikely to me that user would want to import multiple
  profiles at the same time. I've no data to back up this claim
- Users currently have no way of knowing when an import finishes once
  they close the modal (unlike regular downloads, which have a
  dedicated view for this), which may cause confusion
- Having multiple concurrent imports seems to degrade the performance
  of the app
- Things still hard break if the active game is changed while the
  import is in progress. This could probably be fixed by finishing
  rolling out the ImmutableProfile to all parts of the code base

Even if the feature is brought back some day, the component is now so
heavily refactored that it's easier to start the implementation from
the scratch than based on the old one.
Yet more cleanup for ImportProfileModal
ImmutableProfile objects will now be used with uninstallation
operations, as changing the target profile in the middle of the
operation would result in disaster.
…mulator-and-subterranauts

Add games Old Market Simulator and Subterranauts
Remove last uses of Profile from ProfileInstallerProvider
The old implementation had a comment mentioning BepInEx icon can be
read only from the cache, yet the code immediately tried to read it
from another location. Based on a sampling on 10 BepInEx games, none
actually contained the icon in the path that was used.

To simplify things, try to read the icon from the default location used
by BepInEx plugins, and fall back to reading the icon from cache. This
still isn't ideal, since cleaning the cache (by manually deleting the
contents) breaks all icons relying on the cache, but fixing this to
support all possible mod loaders is more than I'm willing to tackle
right now.
uninstallMod method was recently changed to accept ImmutableProfile
instead of a profile, but this file wasn't included in the PR since
I had stashed the file due to other simultaneous changes.
After the downloads are completed, the download modal hangs at 100% for
a moment while mods are copied to the profile folder and the mods.yml
file is updated.

The old implementation called ProfileModList.addMod() for each mod,
which meant that for each mod:

- The mods.yml was read six times, including checking if the file
  exists, reading it, parsing the yaml into ManifestV2[], and checking
  if *each* mod in the profile has icon file inside the profile folder
- The mods.yml was written twice, serializing the ManifestV2[] into
  yaml.
- For a modpack with 69 mods, all already in the cache, this took 25
  seconds

The new implementation tracks the contents of the ManifestV2[] in
memory. As a result mods.yml is read twice and saved once regardless
of the number of processed mods. For the aforementioned modpack this
takes four seconds.

It's worth noting that the implementations aren't completely identical
in other things. The new implementation doesn't necessarily define
which mod caused the error, whereas the old one did. The new
implementation doesn't give special treatment to bbepis-BepInExCack
anymore - it's now uninstalled like all the other files are.

Unfortunately this doesn't allow us to use the new implementation
with the static DownloadModModal.downloadSpecific(), nor clean up
the old implementation. That would require more refactoring, which
doesn't need to block these changes.
Both regular installation and importing a profile now use the same
helper function. Effectively this means that import now tracks
installed mods to profile's mods.yml file instead of copying the file
as-is from the exported zip. This restores compatibility with profiles
exported with Gale, and closes one potential attack vector.

Profile exporting no longer includes the mods.yml in the zip files.
Profile import ignores the file even if it exists to accommodate older
profiles.
When ThunderstoreCombos were created from data fetched from IndexedDB,
the objects were ordered in which ever order the database returned
them. Sort the combos based on the dependency string array argument
instead to guarantee consistent ordering.

At least for profile imports this is meaningful, as the mods will be
shown in this order when "custom" sort order is selected. Another place
where this might be an improvement is the buildDependencySet helper in
the download providers.
anttimaki and others added 14 commits October 30, 2024 09:29
Improve error handling when importing profiles via code
Change the icon paths used when reading a mod list from mods.yml
Improve performance of installing mods with multiple dependencies
Always track changes to mods.yml when installing mods to a profile
Return combos in the order based on an argument
Prevent submitting form with enter when invalid profile code is entered
- Across the Obelisk
- Aloft
- Core Keeper
- Dyson Sphere Program
- Green Hell VR
- Subnautica
- Timberborn
This had broken when ThunderstoreMod object dropped a reference to a
list of ThunderstoreVersions to reduce memory usage. ThunderstoreMod
defined a separate getter for package's total download count, but the
sorting method kept using the getter inherited by ThunderstoreMod from
ThunderstoreVersion, always returning 0. TypeScript didn't catch the
error, since due to inheritance, it was a valid getter to call.

Rather than rename the sorting method to use the correct getter, drop
the duplicate getter to ensure similar errors don't occur in the
future. Both ThunderstoreMod and ThunderstoreVersion now have the same
getter, while former just returns the combined download counts of all
the versions.
Fix sorting online mod list by download counts
Upgraded Electron to v24, replaced node-sass with sass
@ebkr ebkr merged commit 3238331 into master Nov 4, 2024
6 checks passed
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.

4 participants