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

Default Filter for Links & Details #4036

Closed
wants to merge 24 commits into from

Conversation

yoshnopa
Copy link
Contributor

This Commit is aiming at the fact that the saved default filter is not taken into consideration outside of the main page its associated with. This is specifically relevant for two types of scenarios, and both are adressed in this commit:

The details page of Performer, Tag or Studio
Links provided from Cards of all kinds in all places, as well as clickable tags

Both right now will give you the view stash delivers as default, combined with the associated filter. This commit reads the default filter of the according type and applies all view-related settings (this means further filters from default as well as searchterms are ignored).
This seems to me much more intuitive, and it has been requested in several ways multiple times from what I can tell:
#4010 #3079 #2573 #1938

However, it was far more complicated than anticipated, if someone more adapt with react sees a way to streamline this more I would be open for that, although I think this solution is not really bad, its just a bit widespread in the code...

I have tested several scenarios and they all work, however we are talking about dozends of scenarios so anyone willing to test it further is welcomed.

Lastly, this commit changes the behavior of stash in a not insignificant way. This raises the question whether it should be made optional. I personally don't think this is necessary, as I cannot think of a user that would not expect the sorting and viewtype (grid/wall) to be identical in details pages, the default main page and when clicking on a tag or other link to a filtered view. But maybe I'm just lacking imagination here.

@yoshnopa
Copy link
Contributor Author

yoshnopa commented Aug 16, 2023

Well, it looks like it slows down the main page for me pretty hefty when opened first. Everything else is performant, I think it has something to do with cache getting confused? All the long taking parts are from index.js... I really don't know how to debug this, its javascript and not a rendering issue, any ideas?

EDIT: The only Information I am pretty sure about is that it depends on the amount of elements on the start page, they seem to sequentially start something and take about two seconds on my not too shabby laptop. For 8 Elements thats 16 seconds of freezing after everything is rendered.

@yoshnopa yoshnopa marked this pull request as draft August 16, 2023 17:14
@yoshnopa
Copy link
Contributor Author

As adressed in Discord, I will put this on hold until the saved_filter refactor is done either by me or WithoutPants, as I'm pretty sure that this problem will not persist if the according filters are not catched from graphql and distributed by apollo, but simply fetched and saved at app start together with the rest of the settings. From there I can fetch the contexts without useeffect, useref or similar asynchronous stuff...

@WithoutPants
Copy link
Collaborator

I think the approach described here is probably not the correct one.

For sub-page views, the default filtering and view options should be persisted in the UI options - they aren't relevant elsewhere, and utilising the default filter is the incorrect approach. This should address all of the related issues.

The clickable tags is a separate issue and should be addressed in separate PR. However, I don't think that we should use the default filter for these either. For the clickable tags, I think the better approach is to modify the current filter to add whatever was clicked. For example, you are viewing scenes with a filter set to only show from studio foo. You click on the tag bar, and the filter will be modified to filter on that tag, in addition to the studio filtering, rather than filtering only on the tag.

@yoshnopa
Copy link
Contributor Author

@WithoutPants Ok, I can do that, however for both scenarios I still have a question/issue before I can do that.

Sub-Pages View: Should this then be made an option in the options->Interface Panel? If so, how would it look like? Thing is, while you could give a drop-down of existing filters there, again I can't see the advantage over taking simply the default filter, beside being customizable (which again, I can't really think of an scenario where you wouldn't expect your default view to be applied). Or do you want it to be a button in the filter drop-down, like the "set default"? Maybe there is something I'm missing?

Clickable Tags: Sounds interesting, but is not really easy implement that way and by itself very incomplete. For example, no (additional) filter, and therefore no view, would be set this way when coming from the start page, when clicking in the scene details page on galleries associated with performer etc.
There are simply multiple places these links can be located at and not all of them simply link to the same page with a different filter. So we need the basis of adaptable link view options before we can think about dynamically adjusting the preexisting filter. I think that THAT part would be a separate PR as well.
This approach would basically ignore the main reason I thought of this as valuable, being brought into views that do not resemble how the site normally presents itself every time a link is clicked and on every subpage.

Sorry if I got something wrong, but while I'm willing to adjust the strategy, I'm just not sure that these proposals are going in the right direction.

@WithoutPants
Copy link
Collaborator

Sub-Pages View: Should this then be made an option in the options->Interface Panel? If so, how would it look like? Thing is, while you could give a drop-down of existing filters there, again I can't see the advantage over taking simply the default filter, beside being customizable (which again, I can't really think of an scenario where you wouldn't expect your default view to be applied). Or do you want it to be a button in the filter drop-down, like the "set default"? Maybe there is something I'm missing?

As mentioned in #2573 - the Default Filter in its current incarnation determines only what is displayed initially when the user clicks on the applicable top-level link - Scenes, Movies etc. I don't think it's expected or desired that whatever that initial view is is applied to sub-page views. For example, my current default filter for scenes is for rating > x, and sorted randomly. This would not be a desirable filter to apply to sub-page views.

In the past, the sub-page view options would be persisted at least in the browser, so that at least the views would be consistent when navigating to different performer pages, for example. I notice that this is not currently the case - except for galleries which appear to at least persist the view mode.

If there's a desire for default view options and filtering applied to sub-pages, then I think it needs to be developed in concert with, or followed by the changes described here: #3880 (comment). Otherwise, we'd need to undo the filter hook adjustments when saving the filter.

In terms of how I'd see this working: I think something similar to how it works in the top-level views is probably fine. Instead of saving the default filter, it would save a default filter for the applicable sub-page view in the UI settings. I do think this will be non-trivial to accomplish with the way that the item lists are structured currently, and there probably needs to be a refactor of the item list code first.

@yoshnopa
Copy link
Contributor Author

It is not intended to use the default filter filters at all. Only the view options (grid/wall/list, Sorting Order, zoom) are applied, because I agree that an additional, default filter would render many subpage views then useless by default. However, for example, if you approach your scenes randomly by default (even if you use it together with another filter per default), you'd probably appreciate being presented with an everchanging order in the subpage view as well (however, the gallery page is taken out precisely because of this, here a separate structure according to the gallery would be expected).

To be honest, I think this part is the more debatable, while I would say that this is an expected behavior, I find the view mode (wall/grid) to be more distracting than the sorting if not taken from the default, so we could also restrict it to that...

The Problem with this way of doing it is that I simply can't think of a way to present this option in a way that is intuitively understandable. I think this problem is on its own something that can be felt as wrong (as the different issues seem to highlight), which is why I think we should at the very least have a better default behavior, with maybe the option to adjust it on your own, so that whats happening is what is expected intuitively. It just feels like bad design to adjust your views to a default so the data is presented in a pleasing way, clicking on a link and having all of this ignored, the same for subpages.
If you have an idea how this default behavior can be enhanced otherwise I'm all for it. To be more precise, the Issues I'm trying to solve are exactly described in #3079 (links) and #1938 (subpages), the other two "feel" that something is off, but I think their proposed solutions are not universally applicable (as you rightly cited from #2573).
This PR also already worked, so you can test the artifacts of previous builds here if my ramblings are confusing. The only thing left was not a feature missing, but a performance issue.

@yoshnopa
Copy link
Contributor Author

#4137 is another candidate to be closed with that.

@yoshnopa
Copy link
Contributor Author

yoshnopa commented Sep 27, 2023

@WithoutPants Alright, lets try again.
I Separated the default view for links and subpages from the default view for basic pages, as you wished. The Button for setting the former is placed where the other one already resides:
Bildschirmfoto vom 2023-09-27 09-52-17
This Button will then save the current filter in the ui options, from where it is retrieved when creating a link (for badges or cards) to a filtered page and when a view includes some subviews of other media (like tags having scenes, markers etc).
While I see that you and me are not fond of making it possible to support some default filters that way and instead would limit this to UI options only, I think the amount of tickets wishing for this speak for at least a possibility to add this, so I made a compromise:
When you try to save a default link setting that would include a filter (like rating, is missing etc) or a searchterm, a scary notice will pop up:
Bildschirmfoto vom 2023-09-27 09-52-44
With a red button for confirmation, and a UI only option (the way I would say it should be handled by default) being blue, where the filter will be sanitized from searchterm and filters before saving.

I have fixed the performance issues and bugs I encountered along the way, so from my point of view this is ready for review again.

@yoshnopa yoshnopa marked this pull request as ready for review September 27, 2023 08:03
@yoshnopa
Copy link
Contributor Author

Closes #3079
Closes #4010
Closes #4137

@sleetx
Copy link

sleetx commented Sep 28, 2023

@WithoutPants Alright, lets try again. I Separated the default view for links and subpages from the default view for basic pages, as you wished. The Button for setting the former is placed where the other one already resides: Bildschirmfoto vom 2023-09-27 09-52-17

I don't think most people would understand the difference between "default for link" and "default for site". I'm not sure this is the best approach.

When you try to save a default link setting that would include a filter (like rating, is missing etc) or a searchterm, a scary notice will pop up: Bildschirmfoto vom 2023-09-27 09-52-44 With a red button for confirmation, and a UI only option (the way I would say it should be handled by default) being blue, where the filter will be sanitized from searchterm and filters before saving.

Again, this is a piece of UI that needs an extensive explanation that I'm not sure people would understand. If this route is chosen there needs to be an easy way to see what content is negatively impacted by the filter, so the user can revert the change if they want to.

What about setting these details-level options in the Interface settings, and providing similar context (with long explanations, if necessary) there?

@yoshnopa
Copy link
Contributor Author

yoshnopa commented Sep 28, 2023

@sleetx I agree that a better explanation would be a good addition. Not quite sure how to explain it though, probably because I'm too deep in the technicalities... I'd appreciate feedback here greatly.
So a better naming for discerning those two separate default filters and a warning that's better understandable are absolutely something I can get behind. I also don't think that a lenghty explanation in the pop-up would be bad, so if this is what's necessary to explain this functionality that that could be the way to do it. When this pop-up is not coming up, the changes aren't that big anyway, so not really bad for someone just "playing around" (changing the default sorting on a subview will make it confusing under certain circumstances, but thats far from breaking or unintuitive).
While its not perfectly explainable, the fact that until now at least 5 issues were opened targeted at this idea means that there should probably be an option for it.

It is not placed in the settings because realizing it there would be even more convoluted: You'd need a way to apply filters, search terms, sorting and view options in the settings view, once for every view, and you wouldn't get any feedback on what this view might include and how it looks like. So I don't think that this would be helpful, as the settings of default always had the advantage that you could anticipate what you could expect. Although a further filter will be added in subview/link, this is, generally speaking, still the case.
Reverting it is quite easy, click on set default link again, in case the pop-up comes up, use ui settings only, done. Only the media vanishes if wrong settings are chosen, the UI elements stay, so this is not prone to locking someone out, and the filters and search terms are still visibly shown and can be tampered with. It's, like the name says, only the default thats changed, customization can be done after.

@WeedLordVegeta420
Copy link

The two "Set default" buttons is pretty confusing, and I'm not clear on why exactly they would be needed.

It seems like the simpler method would be to just go with the more specific option (the one that will be applied in the most limited/restricted scenario). This means the user might have to apply the default multiple times in several places, but how often would people be changing the default? It's a bit more work when you do want to change it everywhere, but it avoids the confusion of having two options and would avoid the frustration of accidentally applying a filter somewhere you don't actually want it.

@yoshnopa
Copy link
Contributor Author

@WeedLordVegeta420 I started out with one button doing both, but that was not how @WithoutPants liked it.
The problem is that there is not really a specific situation where one of it is shown, at least not like that. In a sense, its already done that way, but there are two situations where the new set default is applied: When you click on a link, and when you look through the media of an organization unit (performer, studio, tag). The latter already has only the new set default, but as the link leads to a normal page with a filter, here you can't discern between a default site or default link situation entirely, therefore you cannot simply provide only one set default link at all times.

@WithoutPants
Copy link
Collaborator

I've hacked together a proof of concept - modified from your branch - of my preferred approach for the default filter for sub-views functionality. Note that the link behaviour stuff is not included here; as stated, that behaviour should be separated into another PR. I believe that, instead of adding the default filter to every single link, a better approach would be to add a parameter to the link to load and append the default filter, and this can be handled in the URL parsing code.

https://github.com/WithoutPants/stash/tree/prs/4036

It adds default filters for each view in the system. We don't want to store these in the saved_filters table, as that will require some further back-end changes, potentially schema changes, and it should be a UI-facing preference anyway.

There are a few things that need to be addressed elsewhere before this can go through however. It is becoming increasingly apparent that the UI settings are becoming ungainly, and working with viper is a tremendous pain due to the way it mangles object keys. I think we need to change where the UI settings are saved. A possibility is a separate file, but I'm leaning more towards a database table. This could eventually hold user preferences.

The other major hang up is the way that the sub-views add criteria to the filter. In the PoC, it's not stripping these out, so they get saved in the default filter. Obviously needs to be addressed, but it might need refactoring of the item list code to not end up doing it in a hacky way.

@yoshnopa
Copy link
Contributor Author

yoshnopa commented Oct 26, 2023

EDIT:
Nevermind, it was much code so I've overseen that there is a separate value for every subview, and the button only applies to the subview mentioned. Please ignore the stuff below...

@WithoutPants Thanks for your effort!
While I can work off that and I also have a basic idea how to solve for the subview filter, you also subtracted the buttons and instead put it into the setdefaultfilter button/unified both. While this aligns with the original idea, it would basically mean that the options asked for by at least some other tickets (for example #4010) will become unsolvable at this point by design, or as long as we keep the current design at least. This approach is also a problem if you want a more specific default view (for example a rating/is favorite) when you open the default media view (for example scenes), but you would basically destroy your default behavior if this would be taken as default for subviews (just is Favorite for every subview of scenes would make many subviews of studios/performers/tags probably empty or very sparsly populated). It would also in large portions modify default behavior in the app, leading to confusion (people right now do not expect their default filter being applied everywhere, even if they have a setting that keeps the subviews usable). Same for links being different (although I agree that doing this with an url option is much more elegant, I will do that and put it in a different PR).

I can think of three solutions here:

  1. Of course the one I already implemented, separate the options entirely. While the way its formulated right now may be a bit raw, it could also be made one button which opens a multiple choice popup on where the default is to be applied for example, if its the two buttons bothering you. It could be combined with the UI only alert for that. It would also be possible to split off the default for links as well, which may make the explanation a bit more intuitive.
  2. Use UI options only across the subviews and links, and allow everything else (object filters, search query) in default media view only. In my current opinion this would be too incoherent, but the changes would not bother existing use cases as much. This was my first approach.
  3. Make a general option out of this: Basically have an option in UI that allows for UI of saved filter to be applied to subviews and links, and another one for applying the rest as well. I don't think this takes care of the fact that you may want very different objectfilters in a general media view and in a sub view by default, but it would mitigate some of the problems of just enforcing views where you may not want any.

Of course I tend to the first one, especially as the problems there seem to be the only ones that can be ironed out completely, while the others lock us in a certain design. If you have another solution or an addition we can of course also go another route.

@yoshnopa
Copy link
Contributor Author

yoshnopa commented Oct 26, 2023

@WithoutPants Alright, sorry for the WoT, now the filter from subviews is removed by default. I used the filterhooks that add the according subviewfilter to determine and remove all view-set subviewfilters. It is written to be broadly compatible with more complex filters to be excluded as well.
I don't really know where to start on the viper problems, but I will leave this commit open so you can take it into consideration right now. There is only one line for communicating with the backend in this PR (in filter.ts for loading, the actual saving of the default filter is done the same way it was done before), so adding this before refactoring is not really a big deal, except if you expect the config file size to impact performance right now or the migration to be more complex due to the new value(s).

Thank you again for your help, I will look into the link setting in a different PR.

@yoshnopa
Copy link
Contributor Author

There hasn't been any movement here in the last 8 months. It still resolves a much requested problem. Could this request therefore be reevaluated for implementation?
I know there are unresolved conflicts, but before I tackle them without knowing if I have to wait for another year, I would like to get some update.

@WithoutPants
Copy link
Collaborator

I have a branch of work that adapts the work in this branch. I plan to get it into 0.27 and will close this PR when I submit my own.

@yoshnopa
Copy link
Contributor Author

Awesome, thanks for the heads up!

@WithoutPants
Copy link
Collaborator

Closing in favour of #4962

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