#95 First shot for implementing Focal Point Cropping #96
Conversation
There are still tests missing and the way it has been implemented required changing all other filters to inject the basic Skipper FilterContext. This should most probably be refactored.
|
Please modify this filter as well to support this new feature. The free transform should support all of the new features we add. After doing this, you might decide not to create a separate filter anymore. This is also because as of now, the skrop routes don't contain dynamic things (like the x and y in your case). We created this new filter exactly to support dynamic parameters. |
|
Yeah thought about this as well. The downside I see with this filter is that it required to use a query param instead of path variables. |
filters/cropbyfocalpoint.go
Outdated
| return CropByFocalPointName | ||
| } | ||
|
|
||
| func (f *cropByFocalPoint) CreateOptions(imageContext *ImageFilterContext, filterContext filters.FilterContext) (*bimg.Options, error) { |
There was a problem hiding this comment.
The filter context should not be passed here. Instead please extend this method to add the required information from the FilterContext to the ImageFilterContext
There was a problem hiding this comment.
@danpersa I haven't found an easy way to expose all PathParams without actually knowing which are present.
But yes I agree about refactoring this.
Codecov Report
@@ Coverage Diff @@
## master #96 +/- ##
=========================================
- Coverage 76.33% 75.7% -0.64%
=========================================
Files 18 19 +1
Lines 786 856 +70
=========================================
+ Hits 600 648 +48
- Misses 137 154 +17
- Partials 49 54 +5
Continue to review full report at Codecov.
|
filters/imagefilter.go
Outdated
| focalPointX := ctx.PathParam("focalPointX"); | ||
| focalPointY := ctx.PathParam("focalPointY"); | ||
|
|
||
| if len(focalPointX) > 0 && len(focalPointY) > 0 { |
There was a problem hiding this comment.
I would not add the PathParams to the parameters, I would rather add all of the path params to the ImageFilterContext, something like this:
&ImageFilterContext{
Image: image,
.....
PathParams: pathParams
}
The reason is that with your approach, every time a new filter adds a new parameter we need to modify this method, which breaks the Open Closed Principle. As soon as the path parameters are part of the ImageFilterContext, each of the filters can access them.
There was a problem hiding this comment.
@danpersa the problem is that Skipper currently does not allow to get all PathParams. You need to access them by name.
I can move them out of the parameters but unless there is a change in Skipper I don't see a way to add all PathParams without knowing which exist.
|
Please don't forget to add tests (the build will fail otherwise) and update the README (there is a section there where we mention all of the filters) |
#95 Wrap the filterContext into the ImageFilterContext
|
@danpersa can you have another look. I added both tests for the new filter as well as an entry to the README file. |
filters/cropbyfocalpoint.go
Outdated
| "github.com/zalando/skipper/filters" | ||
| "gopkg.in/h2non/bimg.v1" | ||
| "strconv" | ||
| ) |
There was a problem hiding this comment.
nitpicking: I am not sure if gofmt is working correctly here. The ) should be at start of the line
filters/cropbyfocalpoint.go
Outdated
| } | ||
|
|
||
| func (f *cropByFocalPoint) CreateOptions(imageContext *ImageFilterContext) (*bimg.Options, error) { | ||
| log.Debug("Create options for crop by focal point ", f) |
There was a problem hiding this comment.
Please remove the debug line before merge.
| } | ||
|
|
||
| func TestCropByFocalPoint_CreateOptions(t *testing.T) { | ||
| c := cropByFocalPoint{targetX: 0.5, targetY: 0.5, aspectRatio: 1.5} |
There was a problem hiding this comment.
Probably these repeating tests could be moved to a TableDriven tests?
There was a problem hiding this comment.
Given my very limited Go experience I wouldn't disagree if you suggest that.
I'll have a look on how to do this.
There was a problem hiding this comment.
Great. I am new too, but would love to be of help on this :)
|
@duergner I have added minor comments. Please check if they make sense |
|
@addityasingh I did the two small fixes. Will have a look into the restructure to TableDriven tests. |
| return &bimg.Options{}, nil | ||
| } | ||
|
|
||
| // TODO do focal point crop |
There was a problem hiding this comment.
@duergner can you please open a ticket for this part, so that we don't forget about it?
|
👍 |
There are still tests missing and the way it has been implemented required changing all other filters to inject the basic Skipper FilterContext. This should most probably be refactored.
refs #95