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

Dragged mp3 file gets renamed as "foo.mp3_.mp3" #5

Open
timbl opened this issue Oct 10, 2019 · 10 comments
Open

Dragged mp3 file gets renamed as "foo.mp3_.mp3" #5

timbl opened this issue Oct 10, 2019 · 10 comments
Assignees
Labels
bug Something isn't working

Comments

@timbl
Copy link
Contributor

timbl commented Oct 10, 2019

Eg https://timbl.com/timbl/Public/Test/Music/Les%20Hayden/Proverbs/Les_Hayden_-_01_-_Gift_Horse.mp3_.mp3

The algorithm should notice that the mime type of the file is fine as it .. and not add another extension.

@timbl timbl added the bug Something isn't working label Oct 10, 2019
@timbl
Copy link
Contributor Author

timbl commented Oct 10, 2019

That file was made by dragging a file from the desktop onto the green plus of the folder.

(Free music for testing from the Free Music Archive https://freemusicarchive.org/music/Les_Hayden/Proverbs_1720 )

@timbl
Copy link
Contributor Author

timbl commented Oct 10, 2019

hmm

> mime.lookup('foo.mp3')
'audio/mpeg'
> mime.extension('audio/mp3')
'mp3'
> mime.extension('audio/mpeg')
'mpga'
> mime.lookup('foo.mpga')
'audio/mpeg'
> 

@bourgeoa
Copy link
Contributor

I don't know what is your NSS version.

I tried to drag a .mp3 file to a pod with NSSv5.2.0 and the file on the filesystem appears to be :
file.mp3_.mp3$.mp3
I think it indicates that the solution can only be a combination of solid-ui that adds _.mp3 and NSS that on top adds $.mp3.

  • I think that the check if contentType is correct for the file extension should not be done in 2 different places
    If done in NSS it should be deleted in solid-ui
  • the check in NSSv5.2.0 will display in solid-ui file.mp3 and on the filesystem file.mp3$.mp3
    that is not satisfactory. I think that in such cases the file in the filesystem should be file.mp3 and in general 'file.ext$.ext' should always be 'file.ext'

(I don't know if there is an other case than mp3 in mime-types)

@RubenVerborgh
Copy link

RubenVerborgh commented Oct 23, 2019

NSS issue is separate (but same cause) and fixed in nodeSolidServer/node-solid-server#1328

@megoth megoth added this to To do in Data Browser via automation Oct 23, 2019
@bourgeoa
Copy link
Contributor

bourgeoa commented Nov 7, 2019

https://github.com/solid/solid-ui/pull/147 solves the point suppressing double check in solid-ui and NSS

@megoth
Copy link
Contributor

megoth commented Nov 8, 2019

@bourgeoa I've tested the latest version of dev on node-solid-server (which has nodeSolidServer/node-solid-server#1336 merged into it), and it seems to me that we don't need a fix for solid-ui. (Your fix being https://github.com/solid/solid-ui/pull/147.)

@timbl I suspect this to be the case for your PR as well (https://github.com/solid/solid-ui/pull/144).

With the fix in NSS, when I drag and drop a mp3 file onto the green plus icon, the resulting filename for file_example_MP3_700KB.mp3 is still file_example_MP3_700KB.mp3 (not file_example_MP3_700KB.mp3_.mp3 which happened earlier).

Am I missing something, or could we close these two PRs?

@megoth megoth moved this from To do to On hold in Data Browser Nov 8, 2019
@timbl
Copy link
Contributor Author

timbl commented Nov 9, 2019

I think you are missing something. I am not sure why yours works without #144. The problem is for any mime type extension where mapping is not symmetrical, the old code will mistakenly think that the extension is wrong when in fact it is right. If your case works, then do you still have at the console these results?

> mime.lookup('foo.mp3')
'audio/mpeg'
> mime.extension('audio/mp3')
'mp3'
> mime.extension('audio/mpeg')
'mpga'
> mime.lookup('foo.mpga')
'audio/mpeg'
> 

The extension must be allowed if EITHER it is the right extsnion for the mime type OR th emime type is the right one for the extension. Maybe when there > 1 extsnsions, then the module gives randomly each one... so it will fail randomly? Not sure why yours worked., Not a function of NSS at all.

@RubenVerborgh
Copy link

Not a function of NSS at all.

@timbl It depends.

In NSS 4.x, a file that was POST/PUT to the server would need to have the correct extension, or NSS would think it was Turtle.

In NSS 5.x, only the content type matters.

So why is Mashlib adding this extension?

@bourgeoa
Copy link
Contributor

https://github.com/solid/solid-ui/pull/147 solves the point suppressing double check in solid-ui and NSS

@bourgeoa
Copy link
Contributor

@timbl

  • Not a function of NSS at all.

In NSS 5.x the content is reflected in the filesystem extension :
. foo.txt --> text/plain
. foo.txt$.ttl --> text/turtle
in each case mashlib and any app see foo.txt and real contentType (the $ extension if any is dropped)

  1. mashlib actually for foo.mp3 (audio/mp3) mashlib returns foo.mp3_.mp3 and NSS 5.x doing the same check used to return foo.mp3_.mp3$.mp3 (in the filesystem) --> after NSS fix NSS return foo.mp3_.mp3
  2. mashlib with your fix foo.mp3 is kept --> NSS used to return foo.mp3$mp3 --> after NSS fix NSS returns foo.mp3
  3. with https://github.com/solid/solid-ui/pull/147 and without your fix mashlib keeps foo.mp3 and NSS used to return foo.mp3$mp3 --> after NSS fix returns foo.mp3 on the filesystem

@megoth megoth added this to the Data Browser Release 5 milestone Dec 13, 2019
@megoth megoth moved this from On hold to To do in Data Browser Dec 13, 2019
@megoth megoth self-assigned this Dec 13, 2019
@timea-solid timea-solid removed this from To do in Data Browser Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants