Show duration and filesize in results #1776

kermieisinthehouse · 2021-09-27T00:46:03Z

Resolves #1775

This PR has some issues. Off the top of my head:

the frontend stuff can be ignored. It's a mess and I'm sure I'm doing stuff wrong
the scenes query doesn't work if it requires a join (probably simple)
the images query is untested and doesn't work
For performance, Query () may need to be refactored into something along the lines of:

type SceneQueryStatsRequest struct {
    count bool
    durationSeconds bool
    filesizeBytes bool
}

type SceneQueryStats struct {
    count int
    durationSeconds float64
    filesizeBytes int
}

func Query(query queryBuilder ..., statsToReturn SceneQueryStatsRequested) (scenes ..., stats SceneQueryStats) {
...
}

So that it will conditionally run the queries.

pkg/models/image.go

WithoutPants · 2021-10-21T05:49:12Z

I have finally spent some time working on this. The backend is complete. It refactors the way we perform Scene and Image queries. They now accept a query options object which gives the filter criteria and the wanted return values. They return a query result object, which only contains the ids initially, and can be resolved into objects. All impacted calling methods are adjusted. At the api package-level, the aggregate values (count, filesize etc) are only queried for if the field was requested in the graphql.

The frontend has been adjusted slightly, but imo still needs a bit of work. I'm not yet convinced that - at least currently - having pagination at both the top and bottom is necessarily a good idea. At the top, it uses up quite a lot of vertical real estate.

I particularly don't think that its useful (or aesthetically pleasing) to have the pagination statistics at both the top and the bottom.

It looks pretty ordinary especially when there is a small amount of results:

I don't have any decent prior art to work with - or UX principles - but these would be my suggestions:

both paginations should not be shown if they will be on the same screen without scrolling. I'm not sure how to implement something like this though. The hacky solution is to not show it if there are fewer items than some constant. I suspect that we'll get complaints about inconsistent behaviour with this though.
Query statistics should be shown once - either at the bottom, or perhaps right aligned on the same row as the pagination bar, so as not to take up more vertical real estate than necessary.

kermieisinthehouse · 2021-10-21T06:38:07Z

This is freaking fantastic! Works great, I haven't noticed any performance issues even without the index I thought it needed. Makes it much easier to reason about the relative sizes of sections of a collection.

About the UI concerns, I think they are perfect as-is. I find myself scrolling very often to the bottom of a page to check how many results were returned for a filter, and having it at the top is extremely convenient. You get used to it very quickly. Even with few results, I don't think it's cluttered having it duplicated on the page.

SmallCoccinelle

This is all the things I can find from a quick glance.

I think it's pretty good.

graphql/schema/types/image.graphql

SmallCoccinelle · 2021-10-21T21:51:50Z

pkg/image/query.go

+func QueryWithCount(qb Queryer, imageFilter *models.ImageFilterType, findFilter *models.FindFilterType) ([]*models.Image, int, error) {
+	// this was moved from the queryBuilder code
+	// left here so that calling functions can reference this instead
+	result, err := qb.Query(QueryOptions(imageFilter, findFilter, true))


Nitpicking, but at this point, result is really a request.

Then you can have request.AddCount()

This is incorrect - Query returns an ImageQueryResult which in this case will contain the ids only. The result is turned into Image objects only with the Resolve function. So not really a request. We want the ability to not resolve the images if we don't need to.

In any case, this particular function is no longer necessary since I removed all reference to it and there are concise alternatives anyway.

SmallCoccinelle · 2021-10-21T21:53:25Z

pkg/image/query.go

+		return nil, 0, err
+	}
+
+	images, err := result.Resolve()


And resolved here into a result. An alternative implementation would return images inside a struct.

It's nitpicking though and I wouldn't let it hold back this patch.

I know the code doesn't do this however.

SmallCoccinelle · 2021-10-21T21:54:29Z

pkg/models/image.go

+
+	finder     ImageFinder
+	images     []*Image
+	resolveErr error


would probably just call this err. It's implicit it is for resolution.

I think renaming it to err could imply that its the error for the query operation and not the resolve operation.

pkg/models/image.go

kermieisinthehouse · 2021-10-23T02:30:04Z

I can't approve my own PR, but
<Tested-By: kermieisinthehouse>

Emilaia · 2021-10-25T17:03:26Z

Started getting the following issue since either yesterday or this morning.

So I'm going through the newest additions to see what queried an aggregate value, and I believe it should be this feature.

When I apply a filter to target a very limited subset of scenes/images, this error doesn't pop up, but when I leave it to my default filters (which are basically just a random that targets all my entries), this starts to happen. I don't know the concrete implementation to know if one unit of the size int should represent a byte, mb or something higher or smaller.

Can't tell what extra information I should provide other than my library stats, so here you go.

kermieisinthehouse · 2021-10-25T18:39:43Z

@Emilaia What platform are you running on? I'll try to reproduce this

Emilaia · 2021-10-25T18:44:30Z

Running on a raspberry pi 4B, via docker (so linux/arm), using the latest stashapp/stash:development image.

kermieisinthehouse · 2021-10-25T18:46:56Z

Can you post the results of docker version and uname -a?

Emilaia · 2021-10-25T18:52:16Z

SmallCoccinelle · 2021-10-25T19:06:34Z

Armv7l is 32bit, so int is 32bit on that platform. So the count doesn't fit when it is signed. It has to be an int64 to make sure stuff fits in it. The JSON will usually have a problem once get past 53bit integers, since that's the breakoff point for a float64: once you have a larger integer it is not float64-representable.

SmallCoccinelle · 2021-10-25T19:07:35Z

And sizes are often stored in uint64 or the like. They can't be negative, so it's worth signifying this.

kermieisinthehouse · 2021-10-25T19:16:12Z

Any number around 3 billion and change always tips me off to 32 bit problems. It's strange how the front page stats don't seem to have a problem. I'll put up a fix when I find the culprit

bnkai · 2021-10-25T19:53:55Z

Same issue was present in the main stats page
Relevant fix #994

kermieisinthehouse · 2021-10-25T19:56:25Z

Same issue was present in the main stats page Relevant fix #994

You just saved me a few hours!

kermieisinthehouse · 2021-10-26T00:12:21Z

@Emilaia I opened up a PR, #1895 with a proposed fix, but I don't have an RPI handy to test.

Can you test and see if the problem is resolved? I've created a build for armv7, available at https://kermie.isinthe.house/stash-linux-arm32v7-statsfix

Emilaia · 2021-10-26T00:25:36Z

Sorry, but I'm unaware as to how use this and docker/docker-compose, as I don't see a file extension attached to it, have only worked with tar files.

kermieisinthehouse · 2021-10-26T00:32:49Z

It's a plain linux executable. Let me see if I can put together a docker build

kermieisinthehouse · 2021-10-26T00:48:35Z

@Emilaia
I've created an arm32 docker image for that branch, available at https://kermie.isinthe.house/docker-stashapp-stash-stats-test.tar . You can use it by doing docker load < docker-stashapp-stash-stats-test.tar. Let me know if you need anything else to test.

Emilaia · 2021-10-26T00:50:09Z

It leads me to a 404

kermieisinthehouse · 2021-10-26T00:53:47Z

Sorry, I thought it would upload a lot faster - should be all set now.

Emilaia · 2021-10-26T01:04:28Z

Tested it on all the screens that I had the issues with before, no issues now.
Guess I'll keep this image until the official rollout comes, haha.

kermieisinthehouse · 2021-10-26T01:08:26Z

Fantastic! That PR will probably hit develop soon, and if you use the 'latest' branch, it will probably be sometime in November.

kermieisinthehouse added 2 commits September 25, 2021 04:37

Complete backend / graphql edit

3655008

more progress

4ccc89f

WithoutPants reviewed Sep 27, 2021

View reviewed changes

pkg/models/image.go Outdated Show resolved Hide resolved

WithoutPants added bounty This issue has a bounty on it in the OpenCollective improvement Something needed tweaking. labels Sep 27, 2021

kermieisinthehouse added this to the Version 0.11.0 milestone Oct 10, 2021

WithoutPants added 15 commits October 21, 2021 09:23

Merge remote-tracking branch 'upstream/develop' into prs/1776

b682658

Rename fields

bb18276

Revert Query changes

3017eee

Add new query interface

558f70d

Refactor query builder

810294d

Change Query interface

21430c3

Return duration and filesize in scene query

1d56213

Adjust UI for scene metadata

bfd2278

Introduce new image query interface

a6a2f65

Change image Query interface

67e51b4

Add megapixels and size to image query

dd895fb

Update image UI

5ac6505

Handle scene null values

fba8a71

Handle null values

c30811d

Styling fixes

8aca35d

kermieisinthehouse marked this pull request as ready for review October 21, 2021 06:38

SmallCoccinelle reviewed Oct 21, 2021

View reviewed changes

WithoutPants added 2 commits October 22, 2021 14:59

Typo

f958356

Defer Find refactor to another PR

54cb839

WithoutPants added 2 commits October 25, 2021 11:00

Merge remote-tracking branch 'upstream/develop' into prs/1776

4155c6d

Add changelog entry

7aecd20

WithoutPants approved these changes Oct 25, 2021

View reviewed changes

Fix compile error

f467cbb

WithoutPants merged commit 4dd56c3 into stashapp:develop Oct 25, 2021

kermieisinthehouse deleted the show-duration-and-filesize-in-results branch October 25, 2021 00:43

kermieisinthehouse mentioned this pull request Oct 26, 2021

Convert filesize to float64 for arm32 #1895

Merged

7dJx1qP mentioned this pull request Nov 7, 2021

[Feature] media metadata on performers/tags/movies #1968

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show duration and filesize in results #1776

Show duration and filesize in results #1776

kermieisinthehouse commented Sep 27, 2021 •

edited by WithoutPants

Loading

WithoutPants commented Oct 21, 2021

kermieisinthehouse commented Oct 21, 2021

SmallCoccinelle left a comment

SmallCoccinelle Oct 21, 2021

WithoutPants Oct 22, 2021

WithoutPants Oct 22, 2021

SmallCoccinelle Oct 21, 2021

SmallCoccinelle Oct 21, 2021

SmallCoccinelle Oct 21, 2021

WithoutPants Oct 21, 2021

kermieisinthehouse commented Oct 23, 2021

Emilaia commented Oct 25, 2021

kermieisinthehouse commented Oct 25, 2021

Emilaia commented Oct 25, 2021 •

edited

Loading

kermieisinthehouse commented Oct 25, 2021

Emilaia commented Oct 25, 2021

SmallCoccinelle commented Oct 25, 2021

SmallCoccinelle commented Oct 25, 2021

kermieisinthehouse commented Oct 25, 2021

bnkai commented Oct 25, 2021

kermieisinthehouse commented Oct 25, 2021

kermieisinthehouse commented Oct 26, 2021

Emilaia commented Oct 26, 2021

kermieisinthehouse commented Oct 26, 2021

kermieisinthehouse commented Oct 26, 2021

Emilaia commented Oct 26, 2021

kermieisinthehouse commented Oct 26, 2021

Emilaia commented Oct 26, 2021 •

edited

Loading

kermieisinthehouse commented Oct 26, 2021

Show duration and filesize in results #1776

Show duration and filesize in results #1776

Conversation

kermieisinthehouse commented Sep 27, 2021 • edited by WithoutPants Loading

WithoutPants commented Oct 21, 2021

kermieisinthehouse commented Oct 21, 2021

SmallCoccinelle left a comment

Choose a reason for hiding this comment

SmallCoccinelle Oct 21, 2021

Choose a reason for hiding this comment

WithoutPants Oct 22, 2021

Choose a reason for hiding this comment

WithoutPants Oct 22, 2021

Choose a reason for hiding this comment

SmallCoccinelle Oct 21, 2021

Choose a reason for hiding this comment

SmallCoccinelle Oct 21, 2021

Choose a reason for hiding this comment

SmallCoccinelle Oct 21, 2021

Choose a reason for hiding this comment

WithoutPants Oct 21, 2021

Choose a reason for hiding this comment

kermieisinthehouse commented Oct 23, 2021

Emilaia commented Oct 25, 2021

kermieisinthehouse commented Oct 25, 2021

Emilaia commented Oct 25, 2021 • edited Loading

kermieisinthehouse commented Oct 25, 2021

Emilaia commented Oct 25, 2021

SmallCoccinelle commented Oct 25, 2021

SmallCoccinelle commented Oct 25, 2021

kermieisinthehouse commented Oct 25, 2021

bnkai commented Oct 25, 2021

kermieisinthehouse commented Oct 25, 2021

kermieisinthehouse commented Oct 26, 2021

Emilaia commented Oct 26, 2021

kermieisinthehouse commented Oct 26, 2021

kermieisinthehouse commented Oct 26, 2021

Emilaia commented Oct 26, 2021

kermieisinthehouse commented Oct 26, 2021

Emilaia commented Oct 26, 2021 • edited Loading

kermieisinthehouse commented Oct 26, 2021

kermieisinthehouse commented Sep 27, 2021 •

edited by WithoutPants

Loading

Emilaia commented Oct 25, 2021 •

edited

Loading

Emilaia commented Oct 26, 2021 •

edited

Loading