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.

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

Fix some mp3/mp4 media files show as Raw #18458

Closed
wants to merge 17 commits into from
Closed

Fix some mp3/mp4 media files show as Raw #18458

wants to merge 17 commits into from

Conversation

marxangels
Copy link

from

image

to

image

@codecov-commenter
Copy link

Codecov Report

Merging #18458 (3479fe4) into main (246902c) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #18458      +/-   ##
==========================================
- Coverage   46.27%   46.27%   -0.01%     
==========================================
  Files         842      842              
  Lines      121178   121184       +6     
==========================================
- Hits        56075    56073       -2     
- Misses      58292    58304      +12     
+ Partials     6811     6807       -4     
Impacted Files Coverage Δ
modules/typesniffer/typesniffer.go 79.54% <0.00%> (-12.56%) ⬇️
routers/web/repo/view.go 39.52% <0.00%> (ø)
modules/util/remove.go 41.26% <0.00%> (-7.94%) ⬇️
modules/git/blame.go 69.31% <0.00%> (-2.28%) ⬇️
modules/queue/queue_bytefifo.go 52.02% <0.00%> (-1.48%) ⬇️
modules/queue/workerpool.go 56.74% <0.00%> (-1.13%) ⬇️
models/repo_list.go 75.66% <0.00%> (-0.49%) ⬇️
services/gitdiff/gitdiff.go 74.16% <0.00%> (+0.27%) ⬆️
services/repository/files/content.go 64.51% <0.00%> (+6.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 246902c...3479fe4. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 30, 2022
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 30, 2022
@lunny lunny added this to the 1.17.0 milestone Jan 30, 2022
@lunny lunny added type/bug and removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Jan 30, 2022
@Gusted
Copy link
Contributor

Gusted commented Jan 30, 2022

Should mp3/mp4 be viewed as normal file? IIRC Gitlab/Github still haves them under /raw

@marxangels
Copy link
Author

Should mp3/mp4 be viewed as normal file? IIRC Gitlab/Github still haves them under /raw

It depends on how you use it.
Maybe we can add a configuration for the end user to make a decision.

@lunny
Copy link
Member

lunny commented Jan 30, 2022

You can use display= like what svg file did.

@@ -21,6 +24,7 @@ func newMimeTypeMap() {
m := make(map[string]string, len(keys))
for _, key := range keys {
m[strings.ToLower(key.Name())] = key.Value()
_ = mime.AddExtensionType(key.Name(), key.Value())
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should ignore a error here.

func DetectContentTypeExtFirst(name string, readSeekerOrBytes interface{}) (SniffedType, error) {
// we can merge `setting.MimeTypeMap` by mime.AddExtensionType()
ct := mime.TypeByExtension(filepath.Ext(name))
if ct != "" && !strings.Contains(ct, "text/") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to change Contains -> HasPrefix?

if nil != err {
return SniffedType{}, err
}
_, err = r.Seek(0, io.SeekStart)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is Seek needed?

@@ -63,6 +63,7 @@ func ServeData(ctx *context.Context, name string, size int64, reader io.Reader)
// Google Chrome dislike commas in filenames, so let's change it to a space
name = strings.ReplaceAll(name, ",", " ")

// st, err := typesniffer.DetectContentTypeExtFirst(name, buf)
Copy link
Member

Choose a reason for hiding this comment

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

Commented code is confusing and we shouldn't have it. If you want to retain it for future reference, it's better to add a sentence or two about it.

@marxangels
Copy link
Author

Build is failing. I'll fix this after another pull request #18448 is finished.

// ServeBlob download a git.Blob
func setCommonHeaders(ctx *context.Context, name string, data interface{}) error {
// Google Chrome dislike commas in filenames, so let's change it to a space
name = strings.ReplaceAll(name, ",", " ")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed actually, from what I remember about this weird bug, if you quote the filename(as being done in this function) chromium won't error about it.

@stale
Copy link

stale bot commented Apr 19, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Apr 19, 2022
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Apr 19, 2022
@stale stale bot removed the issue/stale label Apr 19, 2022
@lunny
Copy link
Member

lunny commented Jun 3, 2022

Please resolve the conflicts.

@techknowlogick techknowlogick modified the milestones: 1.17.0, 1.18.0 Jun 8, 2022
@lunny lunny removed this from the 1.18.0 milestone Dec 20, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants