Improve avatar uploading / resizing / compressing, remove Fomantic card module

[wxiaoguang]

Fixes: #8972
Fixes: #24263

And I think it also (partially) fix #24263 (no need to convert) , because users could upload any supported image format if it isn't larger than AVATAR_MAX_ORIGIN_SIZE

The main idea:

• if the uploaded file size is not larger than AVATAR_MAX_ORIGIN_SIZE, use the origin
• if the resized size is larger than the origin, use the origin

Screenshots:

JPG:

APNG:

WebP (animated)

The only exception: if a WebP image is larger than MaxOriginSize and it is animated, then current webp package can't decode it, so only in this case it isn't supported. IMO no need to support such case: why a user would upload a 1MB animated webp as avatar? crazy .....

[zeripath]

Looking at image.DecodeConfig(...) the registered function that x/image/webp provides will return an imgCfg with dimensions for webp and a format string to identify that the image is a webp. Which types of webp does detectAcceptableWebp provide config for that that function does not?

It might be helpful to add a line to explain why that's there - and if it's needed perhaps it should just move it out of avatar.go into say webp.go?

---

If I'm correct that DecodeConfig would just work for us, then from a code-flow PoV I suspect that we would be better off moving the dimensions check out of resizeAvatar.

Instead of having TryToResizeAvatar we would have the function CheckOrReencodeAvatar (or maybe CheckOrReencodeImage as it is already in the avatar package) e.g.:

// CheckOrReencodeImage checks the provided image data conforms with maximal dimensions and sizes
// and if the provided Image is too big it is resized and reencoded, otherwise it is returned unchanged
func CheckOrReencodeImage(data []byte, maxOriginSize int64) ([]byte, error) { ... }

(I'm not immediately certain of the use the maxOriginSize in that signature as it appears to be always just setting.Avatar.MaxOriginSize and modules/avatar is already dependent on modules/setting - I know you're trying to make this testable - an option may be to make an Options type - see below)

CheckOrReencodeImage would first then check the dimensions and the format of the image, if it is a webp of the correct dimensions it could be left alone otherwise it could be reencoded as per resizeAvatar. That way the code flow could be a bit clearer.

---

Should we make an Avatar Options type?

From an architecture PoV I've always thought that we should invert the dependency hierarchy of modules/setting, but this would not be easy.

One of the major problems with the current setting architecture is that makes making unit tests difficult, if not impossible as there is such a reliance on global state.

However, we could actually begin avoiding some of the problem with global state for settings at least within modules by doing a trick...

1. Within modules/setting/picture.go make the Avatar variable implement a type:

// AvatarOptions is a struct type for Avatar setttings
type AvatarOptions {
		Storage

		MaxWidth           int
		MaxHeight          int
		MaxFileSize        int64
		RenderedSizeFactor int
}

var (
	Avatar = AvatarOptions{
		MaxWidth:           4096,
		MaxHeight:          3072,
		MaxFileSize:        1048576,
		RenderedSizeFactor: 3,
	}

...

2. Then within modules/avatar/avatar.go create a type alias for that setting.AvatarOptions.

type Options = setting.AvatarOptions

3. Change every function within modules/avatar that would depend on setting.Avatar to use *Options which is passed in, with setting.Avatar passed in for that code.

This of course won't remove the dependency on modules/setting but ... it creates a way that you can more easily test avatar code away from the global state and could lead to a way in which we could invert the hierarchy in future.

[wxiaoguang]

Which types of webp does detectAcceptableWebp provide config for that that function does not? ... It might be helpful to add a line to explain why that's there ...

Added in 73340d0844b212219016fef6b3a29b27a8cfed2f

the maxOriginSize, the Avatar Options type

It would change more lines of code, and I don't see enough benefit at the moment.

Since this is not a refactoring PR (actually, this is more like a bug-fix PR), I think keeping maxOriginSize is good enough, leave more "setting dependency" problems to the future refactoring PRs.

Refactor!

---

Update: I think we can reuse the result image.DecodeConfig , WIP done.

[wxiaoguang]

To avoid unnecessary "setting.xxx" access, I think it could be like this f402234e82c800d13e71fe79e4f11b281af06c04 at the moment.

[wxiaoguang]

Refactoring done in 8afdb7e5f5eb43fb3229c7ea51459269369f12bb and ce749df22a231fc52cf802fe1a14bd3ce1e33e89 , major changes:

• Introduce new ProcessAvatarImage, and add more comments
• Some old test asserts are remove, they are covered by new Test_ProcessAvatarImage
• Only accept supported image formats explicitly (png/jpeg/gif/webp) , no implicit support if any other image package is registered anywhere else.

I think this PR is ready for review.

[silverwind]

Moved card styles to modules/card.css and rebased.

[delvh]

util.NewInvalidArgumentErrorf?
Also, shouldn't we print the unsupported image type for debugging reasons?