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

WIP: Model sort selection. #237

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Conversation

Symbiomatrix
Copy link
Contributor

@Symbiomatrix Symbiomatrix commented Sep 12, 2023

As outlined in #236 , js side inter category sorting remains to be handled, till then this does little (but kinda works if shouldSort is disabled).

@DominikDoom
Copy link
Owner

Alright, thanks. I'll see if I can integrate it.

@DominikDoom
Copy link
Owner

I was able to add most of the Javascript functionality today.
Also, since we always need to re-sort in the JS anyway, I simplified the python sort function. It actually doesn't sort at all anymore, but just calculates the correct value we want to sort by and writes them to the temp files.

You can try out what's there so far on the https://github.com/DominikDoom/a1111-sd-webui-tagcomplete/tree/feature-sorting branch. I didn't test it much yet, so please tell me if you notice something not working or sorting differently than you expected.
The sorting itself is mostly ready, but before releasing, I want to add more granular options on what gets sorted which way. (E.g. wildcards alphabetically but Loras by date). Options for ascending/descending order and a few other sorting keys would be nice too, which might require a few small javascript adjustments to make it more generic. But the basics are there now.

The sort by frequent use I talked about is still very WIP, but should be able to use the same js logic, so this can be added without needing to wait.

@DominikDoom DominikDoom linked an issue Sep 13, 2023 that may be closed by this pull request
@DominikDoom DominikDoom added the enhancement New feature or request label Sep 13, 2023
@Symbiomatrix
Copy link
Contributor Author

Also, since we always need to re-sort in the JS anyway, I simplified the python sort function. It actually doesn't sort at all anymore, but just calculates the correct value we want to sort by and writes them to the temp files.

Yep, I figured having a key would obviate the need for that patchwork sort. The main intention was to standardise the sort section for all model types, you seem to have built up that basic design well.

You can try out what's there so far on the https://github.com/DominikDoom/a1111-sd-webui-tagcomplete/tree/feature-sorting branch. I didn't test it much yet, so please tell me if you notice something not working or sorting differently than you expected.
The sorting itself is mostly ready, but before releasing, I want to add more granular options on what gets sorted which way. (E.g. wildcards alphabetically but Loras by date). Options for ascending/descending order and a few other sorting keys would be nice too, which might require a few small javascript adjustments to make it more generic. But the basics are there now.

Looking good so far, well done. Hum, what other adjustments might be needed on the js side, unless it's a dynamic sort like the planned frequency count?

@DominikDoom
Copy link
Owner

Hum, what other adjustments might be needed on the js side, unless it's a dynamic sort like the planned frequency count?

Not much, mostly just cleanup to make it less redundant and ugly if additional options get added.
Right now it is a basic switch case that returns a separate sort function depending on the selected option. But it's not really the prettiest thing to do that for each sorting type that might be relevant, especially if a choice between ascending/descending comes into play. So I want to create good templates for numeric and string sort and just decide which one to use in the switch case. Maybe even move that declaration back to the python dict to make it easier to manage.

Frequency count will require more changes since it is also relevant for normal tags, but like I said that will be a different update.

@Symbiomatrix
Copy link
Contributor Author

Symbiomatrix commented Sep 13, 2023

Ah, I see. In that case, I doubt you'd err even if inferring the type from the data (assume numeric unless text encountered); I've only seen a handful of models published with a numerical name, let alone all of them. I'd be more concerned with asynchronous updates to a numeric / string setting messing up the load.

Edit: Or a more lowbrow working solution: store two sort columns, numeric and string, and only fill one of them on the py side. Very scant in boilerplate, quick to detect, foolproof.

@DominikDoom
Copy link
Owner

Well, not much different at that point than just specifying in the JS switch case directly. Somewhere the decision must be made, don't really think there is much difference between doing it in python vs JS. The CSV loader snippet will treat everything as a string anyway.

The difference in implementation is literally just doing parseFloat and b - a or instead a.localeCompare(b) for strings.
Where it gets trickier is just if types are mixed, e.g. in the < menu where both items with real files (and thus a modification date) and single-file entries like Chants are returned. But I already have that covered in the current implementation, just as I said not as its own extracted function for reusability.

@DominikDoom
Copy link
Owner

I just settled on this in js for now, should be simple enough:

switch (criterion) {
    case "Date Modified (newest first)":
        return numericSort(a, b, true);
    case "Date Modified (oldest first)":
        return numericSort(a, b, false);
    default:
        return textSort(a, b);
}

with the last parameter specifying if it should be reversed or not.

I decided against a separate setting for Ascending/Descending and just additional list options, mainly because ascending & descending can get confusing with dates in my opinion. Newest and oldest (or a-Z/z-A for a reversed name sort) are much clearer.

Plugging in frequency will just be another case with a numeric sort, except that the values will come from an API call to get the statistics from a local db, so pretty compatible.

I'd be more concerned with asynchronous updates to a numeric / string setting messing up the load.

Regarding this, the js part awaits the refresh after the setting changes, so on that side it shouldn't cause issues. At most requiring a second popup close/open if you're too fast. The only thing I'm not sure about is if changing the setting too fast while the previous refresh is still running in python will lead to problems with writing the temp files, since the api call is async but calls a sync function. Probably need to add some safeguards for that and either block the setting from being changed during the load or making sure the API calls return in order so that the latest change really comes last.

@DominikDoom DominikDoom merged commit 3953260 into DominikDoom:main Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Add optional sort key to models.
2 participants