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.

Sign up for GitHub

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

Jump to bottom

add: ImageType.JPEG #1862

Draft
wants to merge 5 commits into
base: dev
Choose a base branch
from
Draft

add: ImageType.JPEG #1862

wants to merge 5 commits into from

Conversation

cssxsh
Copy link
Contributor

@cssxsh cssxsh commented Jan 30, 2022 •
edited

有些时候文件的后缀命名是 jpeg，还有就是 http header 中 jpg 图片 的 Content-Type 一般是 image/jpeg，
这里是希望，对于 jpeg 不用处理直接 可以作为 formatName 参数

@Him188
Copy link
Member

Him188 commented Jan 30, 2022

那应该为 JPG 添加 secondaryNames 比较好

@cssxsh
Copy link
Contributor Author

cssxsh commented Jan 30, 2022 •
edited

那应该为 JPG 添加 secondaryNames 比较好

这里我参考的是对 APNG 的处理

mirai/mirai-core-api/src/commonMain/kotlin/message/data/Image.kt

Line 380 in d8def8e

APNG("png"),

mirai/mirai-core/src/commonMain/kotlin/message/ImageDecoder.kt

Line 158 in d8def8e

ImageType.PNG, ImageType.APNG -> getPNGImageInfo()

@Him188
Copy link
Member

Him188 commented Feb 10, 2022 •
edited

可考虑弃用原 APNG 然后实现我上面提议的新方案 @Karlatemp 你觉得呢

@AdoptOSS
Copy link
Contributor

可考虑弃用原 APNG 然后实现我上面提议的新方案 @Karlatemp 你觉得呢

这不可行，APNG 并不是 PNG 的 alternative name，而是另一种基于PNG的扩展标准，在协议中也使用单独的format id

@AdoptOSS
Copy link
Contributor

这里我参考的是对 APNG 的处理

jpg 和 jpeg 似乎是纯粹的别名关系，png 和 apng 并不属于这种关系，两种情况不应按照相似方法处理
考虑到 mirai 的项目目的，对 ImageType 的细化程度应和TX的协议要求尽可能一致

mirai/mirai-core/src/commonMain/kotlin/message/imagesImpl.kt

Lines 216 to 227 in 2d26f94

internal fun getIdByImageType(imageType: ImageType): Int {
return when (imageType) {
ImageType.JPG -> 1000
ImageType.PNG -> 1001
//ImageType.WEBP -> 1002 //Unsupported by pc client
ImageType.BMP -> 1005
ImageType.GIF -> 2000
ImageType.APNG -> 2001
//default to jpg
else -> 1000
}
}

@Him188
Copy link
Member

Him188 commented Feb 19, 2022

@AdoptOSS 确实，以前写这个 enum 应该就是按这些 id 写的

@sandtechnology
Copy link
Collaborator

那应该为 JPG 添加 secondaryNames 比较好

这里我参考的是对 APNG 的处理

mirai/mirai-core-api/src/commonMain/kotlin/message/data/Image.kt

Line 380 in d8def8e

APNG("png"),

mirai/mirai-core/src/commonMain/kotlin/message/ImageDecoder.kt

Line 158 in d8def8e

ImageType.PNG, ImageType.APNG -> getPNGImageInfo()

有些时候文件的后缀命名是 jpeg，还有就是 http header 中 jpg 图片 的 Content-Type 一般是 image/jpeg， 这里是希望，对于 jpeg 不用处理直接 可以作为 formatName 参数

这里有个问题 对于这种别名 在处理时是不会管你后缀是啥的 mirai内部会直接进行识别 直接按识别结果存储jpg即可 所以为什么要添加JPEG呢（

@cssxsh
Copy link
Contributor Author

cssxsh commented Feb 19, 2022

那应该为 JPG 添加 secondaryNames 比较好

这里我参考的是对 APNG 的处理

mirai/mirai-core-api/src/commonMain/kotlin/message/data/Image.kt

Line 380 in d8def8e

APNG("png"),

mirai/mirai-core/src/commonMain/kotlin/message/ImageDecoder.kt

Line 158 in d8def8e

ImageType.PNG, ImageType.APNG -> getPNGImageInfo()

有些时候文件的后缀命名是 jpeg，还有就是 http header 中 jpg 图片 的 Content-Type 一般是 image/jpeg， 这里是希望，对于 jpeg 不用处理直接 可以作为 formatName 参数

这里有个问题 对于这种别名 在处理时是不会管你后缀是啥的 mirai内部会直接进行识别 直接按识别结果存储jpg即可 所以为什么要添加JPEG呢（

有时候已经知道 具体的类型，希望可以手动提交

AdoptOSS
AdoptOSS suggested changes Feb 19, 2022
View reviewed changes
mirai-core-api/src/commonMain/kotlin/message/data/Image.kt
@@ -391,7 +392,7 @@ public enum class ImageType(
@JvmStatic
public fun matchOrNull(str: String): ImageType? {
val input = str.uppercase()
return IMAGE_TYPE_ENUM_LIST.firstOrNull { it.name == input }
return IMAGE_TYPE_ENUM_LIST.firstOrNull { it.name == input || it.secondaryNames.contains(input) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

secondaryNames里全是小写，但input被uppercase处理了，会匹配不到任何信息

另外，建议在getByExt（大意）之类的地方改

@AdoptOSS
Copy link
Contributor

其实吧，我觉得应该另外增加一个类似fromMimeType或者fromExtensionName这样的API
然后弃用掉 matchOrNull 这种表意不明的API，至少不应该用在这种场合

但是问题在于，仅仅通过fromMimeType或者fromExtensionName是无法完全确认格式的（有APNG这种情况存在，必须通过byte stream判断），所以实际上还是依靠字节流自动判断比较靠谱

@AdoptOSS
Copy link
Contributor

有时候已经知道 具体的类型，希望可以手动提交

可以说明一下这样做的具体优势吗

@cssxsh
Copy link
Contributor Author

cssxsh commented Feb 20, 2022 •
edited

有时候已经知道 具体的类型，希望可以手动提交

可以说明一下这样做的具体优势吗

避免多余的判断（随机读取）？（实际上image还是会随机读取以获取高度和宽度）

最开始的想法是 既然 ExternalResource 的构造器提供了 formatName 这一参数，就应该有一些兼容性

@Him188 Him188 added the x:question 标签: 需要更多信息 label Feb 27, 2022
Him188
Him188 reviewed Apr 18, 2022
View reviewed changes
Copy link
Member

@Him188 Him188 left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这样的修改将会改变 matchOrNull 的行为（将在原本返回 null 的情况下不返回 null），考虑增加一个新方法。

关于方法命名，@AdoptOSS 的提议很有参考性，不过我觉得使用 fromString(OrNull) 就足够且恰当了。

mirai-core-api/src/commonMain/kotlin/message/data/Image.kt
) {
PNG("png"),
BMP("bmp"),
JPG("jpg"),
JPG("jpg", "JPEG", "JPE"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

建议这里用小写，可以修改匹配的代码来支持

@Him188 Him188 added N 优先级: 一般 t:problem 类型: 不容易归类为特性或 bug 的综合问题 s:core 子系统: mirai-core and removed x:question 标签: 需要更多信息 labels Apr 18, 2022
@Him188 Him188 added this to the Backlog milestone Apr 18, 2022
@Him188 Him188 marked this pull request as draft October 29, 2022 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Him188 Him188 Him188 left review comments

@AdoptOSS AdoptOSS AdoptOSS requested changes

Assignees
No one assigned
Labels
N 优先级: 一般 s:core 子系统: mirai-core t:problem 类型: 不容易归类为特性或 bug 的综合问题
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cssxsh @Him188 @AdoptOSS @sandtechnology