Javascript to fetch mimetype icons #16724

rullzer · 2015-06-03T13:18:45Z

As stated in #16531 we do not want to have the icon in the return of the OCS Share API.

This PR adds a mimetype.js file that is basically and implentation of the mimetype stuff from OC_Helper in javascript. It can probably be made more efficient later on.

The second commit removes the icon from the OCS Share API.

Todo:

Tests!

@MorrisJobke something like this is what you had in mind as well I assume?
@PVince81 could you also have a look
@oparoz Probably also relevant for #14431

MorrisJobke · 2015-06-03T13:23:47Z

core/js/mimetype.js

+	mimeTypeIcons: {},
+
+	init: function() {
+		OC.MimeType.mimeTypeAlias = {


Shouldn't this be generated from

core/lib/private/helper.php

Line 57 in 246000f

private static $mimeTypeAlias = array(

or vice versa or both use a common JSON file?

Is this then even needed in the PHP code part?

I'm not sure. if It is needed then on the php side... we'd have to check.

And yes they should be generated from one source if both are needed.

Probably we want something that makes a lot of the get requets unneeded by including which files are available as mimetype and do almost everything just in javascript.

But probably such improvements are for a later PR

The mapping is needed, otherwise we don't know which media types belongs to which icon and you end up with a bitmap icon instead of a vector icon for an eps file per example.

oparoz · 2015-06-03T13:33:45Z

If both the JS and PHP files need the same mappings, maybe it would be wise to implement #15384 at the same time and cache those files?

PVince81 · 2015-06-03T13:34:03Z

Not sure if related, but have you seen this oldie https://github.com/owncloud/core/blob/master/apps/files/js/files.js#L152

oparoz · 2015-06-03T13:35:31Z

@PVince81 - The problem with that method is that it makes an extra request to the server.

EDIT: I see now in the code that checkExists() does the same thing... :(

PVince81 · 2015-06-03T13:37:53Z

core/js/mimetype.js

+			return OC.MimeType.mimeTypeIcons[mimeType];
+		}
+
+		if (mimeType == 'dir') {


PVince81 · 2015-06-03T13:39:14Z

I don't think it's a good idea to duplicate the list we have in PHP and have it again in JS.
But what could be done is have a JSON file that contains the mime types mapping, and load that JSON file from both PHP and JS.

PVince81 · 2015-06-03T13:40:08Z

core/js/mimetype.js

+	},
+
+	mimetypeIcon: function(mimeType) {
+		if (mimeType === undefined) {


use _.isUndefined(mimeType) which is more accurate. (sometimes equalities in JS are weird)

rullzer · 2015-06-03T13:44:28Z

Ok, so my quick hack obviously has even more problems than I quickly noticed.

I'll try to script a file together that generates a json file which should make any access to the server unneeded.

rullzer · 2015-06-03T14:34:24Z

Ok now it just loads 1 big json file. Which makes the calls for each mimetype to the server obsolete.

rullzer · 2015-06-03T14:36:57Z

@PVince81 I do not assume we can count on the init of the function being completed before the first call to mimetypeIcon?

Any nice javascript magic (that does not make my eyes bleed) that can help?

rullzer · 2015-06-03T14:38:29Z

O and where should this mimetypes.json file be stored? right now it is in /core

PVince81 · 2015-06-03T14:44:10Z

@rullzer $.getJSON() is asynchronous, which means that when init() ends, the call isn't done yet.
There is no good way to make ajax-loaded stuff available in the main thread.

Who would be calling init() usually ?

oparoz · 2015-06-03T14:44:32Z

What you could do is add the callback as an argument to the mimetype method and call that once you've retrieved the file. Caching it would make sure you only make the call the first time.

EDIT: Scratch that, mimetype is to get the path. It's best to have another call to init first.

rullzer · 2015-06-03T14:50:57Z

@PVince81 yeah that was I ws afraid of. It is now just called when the document is ready.

@oparoz I taught about that as well. However then you do run the risk that multiple instances simultaneously try to obtain the json file. Although that chance is probably pretty small

oparoz · 2015-06-03T14:52:03Z

As for the location, maybe move it to config, but that would mean renaming the file to mimetype.aliases.js (or map), so that it doesn't conflict with that move #15384

The reason to not keep it in core would be that as soon as people start adding their own media types, they will want to change the mapping as well.

PVince81 · 2015-06-18T09:08:07Z

If we don't want to load mimetypes asynchronously, which brings some issues, another idea is to compile a Javascript file with all mimetypes in it. Then just load that file with addScript().

OC.Mimetypes = [
   // list of mimetypes
]

This is how it's done currently with L10N files to avoid extra async ajax load.

Anyway, I think this is too big/risky a change for 8.1, moving to 8.2

MorrisJobke · 2015-06-18T09:55:54Z

As stated in #16531 we do not want to have the icon in the return of the OCS Share API.

@PVince81 That was the goal for 8.1, because it was introduced in 8.1.

PVince81 · 2015-06-18T09:57:40Z

Is there a way to fix this without introducing new APIs ?

MorrisJobke · 2015-06-18T10:02:00Z

Is there a way to fix this without introducing new APIs ?

I introduced the icon into OCS ... this PR would like to remove it again to not clutter OCS even more with useless code. :(

rullzer · 2015-06-22T12:00:56Z

@PVince81 yeah that suggestions sounds like the best solution. Probably a simple script to generate this file is easiest.

I guess the change is indeed a bit to big for 8.1... :(

rullzer · 2015-07-06T14:10:27Z

@owncloud-bot retest this please

PVince81 · 2015-07-06T14:17:48Z

config/mimetypealiases.json

@@ -0,0 +1,68 @@
+{
+	"_comment" : "When this file is changed be sure to run",
+	"_comment2": ".occ maintenance:mimetypesjs",


.occ => ./occ

RobinMcCorkell · 2015-07-06T14:28:33Z

Now, how do we trigger the regeneration on every oC update? There may be new files installed with the update, so I guess some sort of always-on migration step will suffice?

rullzer · 2015-07-06T14:30:26Z

Now, how do we trigger the regeneration on every oC update? There may be new files installed with the update, so I guess some sort of always-on migration step will suffice?

@Xenopathic lets look at that for another PR and just get this in for now :)

PVince81 · 2015-07-06T14:30:45Z

A "repair step" could be written that calls the same API as the command, yes.

Maybe that command's code should be exposed as a core public API instead to be callable from other places ?

Does this also support that apps provide their own mime types ? If yes, then it means apps would also need an API to be able to regenerate the file.

rullzer · 2015-07-06T14:31:35Z

@PVince81 no this is not yet supported. Lets also look at that for another PR

This makes it possible to retrieve the icon for mimetypes in javascript. It makes no additional queries to the server to retrieve the mimetype. * config/mimetypealiases.json added * mimetype.js: this is where the logic resides to convert from mimetype to icon url * mimetypelist.js: generated file with a list of mimetype mapping (aliases) and the list of icon files * ./occ maintenance:mimetypesjs : new command for occ to gernerate mimetypes.js * unit tests updated and still work * javascript tests added * theming support * folder of the theme is now present in javascript (OC.theme.folder)

Switch to new javascript mimetype resolver

scrutinizer-notifier · 2015-07-06T14:32:29Z

A new inspection was created.

rullzer · 2015-07-06T14:32:31Z

One more rebase because I can't type.

ghost · 2015-07-06T14:34:41Z

💣 Test FAILed. 💣

rullzer · 2015-07-06T14:35:14Z

@owncloud-bot retest this please

PVince81 · 2015-07-06T15:19:46Z

core/js/tests/specs/mimeTypeSpec.js

+				expect(res).toEqual(OC.webroot + '/core/img/filetypes/file.svg');
+			});
+
+			it('test if the cache works correctly', function() {


of course it tests because it's a test 😉
For next time (leave it now), this would be "caches the mimetype icons" (this is the behavior)

PVince81 · 2015-07-06T15:20:56Z

👍

ghost · 2015-07-06T15:31:49Z

🚀 Test PASSed.🚀

ghost · 2015-07-06T15:54:39Z

🚀 Test PASSed.🚀

RobinMcCorkell · 2015-07-06T18:33:24Z

👍 (I'm too scared to merge this beast though 🙈 )

Javascript to fetch mimetype icons

rullzer · 2015-07-06T19:01:29Z

@Xenopathic I'm not ;)
Luckily it is not yet used in a lot of places :)

LukasReschke · 2015-07-06T20:41:53Z

OMG IT BROKE EVERYTHING! WE SHOULD NOT HAVE MERGED THIS!!!!

Just kidding. Cool stuff 🚀

rullzer added enhancement 2 - Developing feature:sharing labels Jun 3, 2015

rullzer added this to the 8.1-current milestone Jun 3, 2015

MorrisJobke reviewed Jun 3, 2015
View reviewed changes

PVince81 reviewed Jun 3, 2015
View reviewed changes

core/js/mimetype.js

return OC.MimeType.mimeTypeIcons[mimeType];

}

if (mimeType == 'dir') {

Copy link

Contributor

PVince81 Jun 3, 2015

Choose a reason for hiding this comment

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

Use ===

PVince81 reviewed Jun 3, 2015
View reviewed changes

oparoz mentioned this pull request Jun 12, 2015

Send the mime icon if we can't generate a preview #12546

Merged

PVince81 modified the milestones: 8.2-next, 8.1-current Jun 18, 2015

rullzer force-pushed the mimetype-js branch from 0bf84e7 to ecbca27 Compare June 25, 2015 09:03

PVince81 reviewed Jul 6, 2015
View reviewed changes

rullzer added 2 commits July 6, 2015 16:32

Share app no longer use the OCS API icon

868bc42

Switch to new javascript mimetype resolver

rullzer force-pushed the mimetype-js branch from 8093b98 to 868bc42 Compare July 6, 2015 14:32

PVince81 reviewed Jul 6, 2015
View reviewed changes

rullzer pushed a commit that referenced this pull request Jul 6, 2015

Merge pull request #16724 from owncloud/mimetype-js

b4f782b

Javascript to fetch mimetype icons

rullzer merged commit b4f782b into master Jul 6, 2015

rullzer deleted the mimetype-js branch July 6, 2015 19:00

rullzer mentioned this pull request Jul 7, 2015

Move icon from OCS Share API to javascript #16531

Closed

MorrisJobke mentioned this pull request Jul 7, 2015

Core Javascript method to retrieve the media type icon is missing #14431

Closed

rullzer mentioned this pull request Jul 8, 2015

Move away from OCA.files.getMimeIcon and use OC.MimeType.getIconUrl #17483

Merged

2 tasks

oparoz mentioned this pull request Aug 13, 2015

Search can't find media type icons #14432

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 11, 2019

Javascript to fetch mimetype icons #16724

Javascript to fetch mimetype icons #16724

Conversation

rullzer commented Jun 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oparoz commented Jun 3, 2015

PVince81 commented Jun 3, 2015

oparoz commented Jun 3, 2015

Choose a reason for hiding this comment

PVince81 commented Jun 3, 2015

Choose a reason for hiding this comment

rullzer commented Jun 3, 2015

rullzer commented Jun 3, 2015

rullzer commented Jun 3, 2015

rullzer commented Jun 3, 2015

PVince81 commented Jun 3, 2015

oparoz commented Jun 3, 2015

rullzer commented Jun 3, 2015

oparoz commented Jun 3, 2015

PVince81 commented Jun 18, 2015

MorrisJobke commented Jun 18, 2015

PVince81 commented Jun 18, 2015

MorrisJobke commented Jun 18, 2015

rullzer commented Jun 22, 2015

rullzer commented Jul 6, 2015

Choose a reason for hiding this comment

RobinMcCorkell commented Jul 6, 2015

rullzer commented Jul 6, 2015

PVince81 commented Jul 6, 2015

rullzer commented Jul 6, 2015

scrutinizer-notifier commented Jul 6, 2015

rullzer commented Jul 6, 2015

ghost commented Jul 6, 2015

rullzer commented Jul 6, 2015

Choose a reason for hiding this comment

PVince81 commented Jul 6, 2015

ghost commented Jul 6, 2015

ghost commented Jul 6, 2015

RobinMcCorkell commented Jul 6, 2015

rullzer commented Jul 6, 2015

LukasReschke commented Jul 6, 2015