-
-
Notifications
You must be signed in to change notification settings - Fork 826
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
Caption support #2462
Caption support #2462
Changes from 51 commits
5598f91
040a677
8ee6fed
5b6a133
1e793a5
25b64f9
590693a
bc636d6
cf6cebc
fb43459
feeeb1c
9e3424f
2d8c5c5
6152119
861a1b7
09182b1
81d031b
188e141
0c8f5dc
7ddf865
e503bbd
41bf0b3
657b2ae
6074053
b177119
a30ff42
dfcbdee
4fc85e7
2777509
9298311
c3e9593
ffeb5f0
322b3b8
482b967
fdaa1cf
fe702bd
3afdd80
83ba144
85a982e
08e733a
af85da3
2fdcf9b
917c4ce
422a214
69075b3
f7f0772
38dbce4
5b2f4d8
0ec3e78
d1dedca
7be9076
75e8bfb
10d29c6
7d0eb25
ea6d415
8da00f2
852183f
a79cf2d
fdea166
a3bce53
92dbf25
013031b
2a4f8e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,8 @@ input ScanMetadataInput { | |
scanGeneratePhashes: Boolean | ||
"""Generate image thumbnails during scan""" | ||
scanGenerateThumbnails: Boolean | ||
"""Detect scene captions""" | ||
scanDetectCaptions: Boolean | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the benefit of making this optional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this was this intentional. I can update it to be required. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, what I mean is - is there any reason not to have the scan task always detect captions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The detect captions toggle was introduced when the code was still looking for captions by trying various name combinations to see if we could get a match. That approach was expensive, which led to this toggle being added so users without captions wouldn't be impacted by that search when it didn't benefit them. The toggle could probably be removed now that the search code is more intelligent. |
||
|
||
"Filter options for the scan" | ||
filter: ScanMetaDataFilterInput | ||
|
@@ -105,6 +107,8 @@ type ScanMetadataOptions { | |
scanGeneratePhashes: Boolean! | ||
"""Generate image thumbnails during scan""" | ||
scanGenerateThumbnails: Boolean! | ||
"""Detect scene captions""" | ||
scanDetectCaptions: Boolean! | ||
} | ||
|
||
input CleanMetadataInput { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the values are only
true
orfalse
, then it should be a boolean. I think however, that the filtering should probably be aStringCriterionInput
or something so that one can filter on caption language etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a StringCriterionInput initially similar to the studio alias filter. However, we noticed that approach inherited some issues the studio alias filter has. The current implementation was copied from the
has markers
filter, which also uses strings to define a Boolean, but I can look into updating this to use a Boolean.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having had a quick look, I think the
StringCriterionInput
is probably better. I'd prefer to use that and fix the issues arising out of that than have to change the graphql schema later when we figure it out.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll revert the change to continue using the
StringCriterionInput
.