Refactor LocalModList search/sort #1202
Conversation
There was a problem hiding this comment.
Calling the new VueX store module the ProfileModule is a bit misleading IMO given that it primarily is concerned with the local mod list search & sort functionality.
I'm also assuming there's a reason you didn't move the settings updating code to the vuex store yet (perhaps due to the current game/settings not being as easily accessible there) so I don't mind if this version of the code gets merged as is; those concerns can be addressed later / aren't blockers.
So overall this looks good to me 👍
| emitChange() { | ||
| this.$emit('changed', this.internalValue); | ||
| this.$emit('change', this.internalValue); | ||
| } |
There was a problem hiding this comment.
Is this actually needed anymore now that the model binding is used?
There was a problem hiding this comment.
Yes, it's still needed to convey the changes to the internalValue to the external value.
| this.$store.commit('profile/setOrder', value); | ||
| this.settings.setInstalledSortBy(value); |
There was a problem hiding this comment.
I feel like the setting value change should be handled by the store rather than this component?
| this.$store.commit('profile/setDirection', value); | ||
| this.settings.setInstalledSortDirection(value); |
There was a problem hiding this comment.
Likewise here, shouldn't the store be the primary "API" for making these changes, and as such take care of syncing the state to where it should get synced?
Alternatively, the settings could subscribe itself to track the profile store I suppose.
There was a problem hiding this comment.
As per Discord discussions, to make this change properly we'd need to also have the active game and profile available in Vuex. That and these changes are now on my todo list but meanwhile I won't block these changes.
| this.$store.commit('profile/setDisabledPosition', value); | ||
| this.settings.setInstalledDisablePosition(value); |
This is foundational work for other changes that aim to improve performance by continuing to split LocalModList to smaller components.
The original implementation didn't support passing initial values to the input. This is required i.e. when the value should match value stored in VueX
Extract the search bar functionality from LocalModList to standalone component. Unfortunately this yielded no performance benefits.
Previously modifiableModList was used. It was derived from localModList by sorting it. However, one of the sort options allows hiding disabled mods, meaning that the list might be missing an arbitrary number of mods. Instead use localModList which is guaranteed to include all the dependencies/dependants. This will change the functionality a bit by possibly changing the order of mods returned by methods that return lists of dependencies and dependants. Given how these results are used, I *assume* it won't matter that much for the rest of the functionality (since the order could already be quite arbitrary) or the user.
Now that a separate orderedModList is no longer needed, we might as well combine the ordering and filtering into the same method, and do the fiiltering first to speed up ordering when searchQuery is used.
anttimaki
force-pushed
the
mod-list-sort
branch
from
93a2f02
to
21bce97
Compare
| } | ||
|
|
||
| async destroyed() { | ||
| this.$store.commit('profile/setSearchQuery', ''); |
There was a problem hiding this comment.
Added this so the search query gets cleared when the component is unmounted, to match the existing functionality.
No description provided.