kinsi55 / BeatSaber_BetterSongList

Adds Various improvements to the Basegame song list like Filters, a persisted state and much more
MIT License
42 stars 12 forks source link

How about adding last played date sorter? #9

Closed nanikit closed 2 years ago

nanikit commented 3 years ago

I miss this feature of osu game's one. Just suggestion, feel free to close if you mind.

kinsi55 commented 3 years ago

I think it could be useful. I'll look into it at some point.

nanikit commented 2 years ago

I started working on it. If you have something to consider, please share to me.

kinsi55 commented 2 years ago

As for how I was going to build it:

As for issues I see with this sorter:

The order changes on every song play - If you scroll down and play a song from down there, when you go back to the menu, it would be at the very top. This is super weird UX and requires a song list reload, at which point it would become even worse because then BSL will scroll to the last selected song, so to the top.

kinsi55 commented 2 years ago

I think this is something that I'd like to be an optional / third party sorter. If you are not aware, I will be adding and API that allows others to add sorts and filters to BSL so everyone can add whatever they want, even if I disagree with it.

nanikit commented 2 years ago

Then I will try to make it as a separate plugin. All left is maybe plugin API, it looks like it could be a PR. If API is delayed then I'll try it.

kinsi55 commented 2 years ago

My main Issue with the API is that I have not yet figured out how to built it neatly because I want other plugins to have the ability to also disable their sorts / filters, depending on what Playlist / song etc you are currently in (So something like Hitbloq could for example add a sort by hitbloq difficulty, which is only available when you are actually in a Hitbloq playlist). I think I might have to do a minor refactor of how sorts / filters are implemented.

nanikit commented 2 years ago

I used the following for the development so far. What do you think about it?

namespace BetterSongList.Api {
    public interface ISortFilter {
        /// <summary>
        /// Sorter / filter name, appears on dropdown.
        /// </summary>
        string Name { get; }

        /// <summary>
        /// Whether show in dropdown or not
        /// </summary>
        ObservableVariable<bool> IsVisible { get; }

        /// <summary>
        /// Notify beatmap level changes.
        /// </summary>
        /// <param name="newLevels">All levels before sort or filter.</param>
        /// <param name="isSelected">Is the result levels should be changed because it is selected?</param>
        /// <returns>Processing task.</returns>
        Task NotifyChange(IEnumerable<IPreviewBeatmapLevel>? newLevels, bool isSelected = false, CancellationToken? token = null);

        /// <summary>
        /// Sort / filter result.
        /// </summary>
        ObservableVariable<IEnumerable<IPreviewBeatmapLevel>> ResultLevels { get; }
    }

    public interface ILegendProvider {
        ObservableVariable<IEnumerable<(string Label, int Index)>> Legend { get; }
    }
}
kinsi55 commented 2 years ago

What is the point of using ObservableVariable here? Havent seen anyone actually use that before.

What I had in mind was having a callback for external sorters / filters which passes them what it is thats currently selected (To streamline the process so they dont have to get this information themselves) which should be synchronous (return) so that the dropdowns update instantly on switch.

Additionally, what I considered is to make all the Interfaces that I have public so others can use them - What I would've done is make the method used for registering themselves require a special "plugin" interface (For example, something like your ISortFilter interface), but also the "normal" "internal" ISorter or IFilter (Which would then be public)

nanikit commented 2 years ago

What is the point of using ObservableVariable here? Havent seen anyone actually use that before.

I used that for plugin's arbitrary value update. For example pp sorter can request pp information to scoresaber, and update the result later and it can be multiple times (though it is contrived). Listening to you, ObservableVariable may have memory issue, so we can replace it with plain event. Would it be better?

a callback for external sorters / filters which passes them what it is thats currently selected

Does it mean to change Task NotifyChange() to IEnumerable<IPreviewBeatmapLevel> NotifyChange()? If it does I didn't choose it because the above. If we support that plugins update the result asynchronously (or multiple times), this signature increases complexity. I saw current implementation has Prepare() for asynchronous operation. I didn't think this is minimal so just unified these.

Additionally, what I considered is to make all the Interfaces that I have public so others can use them

You may do that. Maybe we'll need a method for plugin registration and so on. Above interfaces are just for minimal requirement of BetterSongList's plugin communication.

Feel free to comment if I misunderstood.

kinsi55 commented 2 years ago

I'll use FS as a shorthand for sorts / filters from here on because its kinda redundant mentioning it all the time

I used that for plugin's arbitrary value update. For example pp sorter can request pp information to scoresaber, and update the result later and it can be multiple times & following section

I dont like that idea, that would increase complexity / edge cases a lot and might degrade performance (Say you are scrolling through the list or are searching for a song and the FS decides to update, possibly resorting the list and / or resetting the search), so I would like to keep the current flow of having a prepare method which is called when the FS is used and forcing the actual data manipulation to be synchronous

nanikit commented 2 years ago

If we don't await NotifyChange, actual data manipulation can be synchronous.

More controversial point is maybe whether plugin can be able to update anytime or not. I initially thought when player finished playing, then I can update the list except the last played song. I didn't intend modifying the list very frequently. But if you think you should control update timing, then the ISortFilter should be changed.

Well, actually you can control update timing without modification.

nanikit commented 2 years ago
#nullable enable
using System;
using System.Collections.Generic;
using System.Threading;

namespace BetterSongList.Api {
    public interface ISortFilter {
        /// <summary>
        /// Sorter / filter name, appears on dropdown.
        /// </summary>
        string Name { get; }

        /// <summary>
        /// Notify beatmap level changes.
        /// </summary>
        /// <param name="newLevels">All levels before sort or filter. null means there is no change.</param>
        /// <param name="isSelected">Is the result levels should be changed because it is selected?</param>
        void NotifyChange(IEnumerable<IPreviewBeatmapLevel>? newLevels, bool isSelected = false, CancellationToken? token = null);

        /// <summary>
        /// null means sorter / filter is not usable.
        /// </summary>
        event Action<ISortFilterResult?> OnResultChanged;
    }

    public interface ISortFilterResult {
        /// <summary>
        /// Sort / filter result.
        /// </summary>
        IEnumerable<IPreviewBeatmapLevel> Levels { get; }

        IEnumerable<(string Label, int Index)>? Legend { get; }
    }
}

Reflected the discussions so far. Still request for comments.

kinsi55 commented 2 years ago

Just so I dont make any wrong assumptions, Is this intended to universally replace my current structure of FS or is this meant to only be used by plugin FS?

nanikit commented 2 years ago

I don't care. Currently I used universally replacement way crude implementation just for test, but if you provide API I'll use that.

kinsi55 commented 2 years ago

I think what I would do:

I would then have the plugin FS always be at the top of the dropdowns so they do not mess with muscle memory of builtin options depending on if they are hidden or not and additionally display a symbol i front of them, indicating they are a plugin FS (Maybe 🔌)

This should keep things very consistent between first- and thirdparty FS with just minor differences

nanikit commented 2 years ago

I'll wait for your work.

kinsi55 commented 2 years ago

I will try to come up with an implementation later

kinsi55 commented 2 years ago

Progress

devenv_2022-03-03_18-03-06

Beat_Saber_2022-03-03_18-01-59

nanikit commented 2 years ago

I have two questions.

I uploaded my testing plugin on here just for reference. I didn't implement all features.

I made BetterSongList support asynchronous (though my plugin doesn't use it).

kinsi55 commented 2 years ago

Could I change the sort result at any time?

No, the result intentionally is not supposed to change without user interaction for the previously mentioned reasons. I do not like the idea of thirdparty FS being able to just randomly resort / refilter the songs list.

levelCategory and playlist are necessary information to all plugins? How about making it as static property?

They are basegame types so I dont see the problem of having them in the function signature. I suppose these two things are what plugin FS would use to decide if they want to be visible or not

kinsi55 commented 2 years ago

I've pushed my proposed implementation now. Keep in mind the ContextSwitch callback is not implemented yet, but at least you can play around with it

nanikit commented 2 years ago

OK it works. image

nanikit commented 2 years ago

FYI BetterSongList does not remember last selected custom sorter. Could you add this feature?

kinsi55 commented 2 years ago

Yeah I'll fix that, this was just a basic implementation test

kinsi55 commented 2 years ago

I just pushed the changes necessary for 1.20, you can add the preprocessor flag PRE_1_20 to compile for 1.19 (Also need to fix the manifest then). I think I fixed it not saving the last option if its a custom one and also implemented the ContextChange method. If you could give this a try and see if it works fine that'd be great

nanikit commented 2 years ago

I checked last used plugin sort works well.

It seems not buildable with PRE_1_20 preprocessor. I'll make a PR.

kinsi55 commented 2 years ago

Cool. If you find any issues with it let me know, else this will be what I release once 1.20 mods roll out

nanikit commented 2 years ago

I close this because plugin feature is shipped. I may file a separate issue if there is a problem.

Thank you for your work.