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

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

Merged
merged 6 commits into from
Jul 10, 2015
Merged

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

merged 6 commits into from
Jul 10, 2015

Conversation

rullzer
Copy link
Contributor

@rullzer rullzer commented Jul 8, 2015

Since we now have the fancy OC.MimeType class (#16724) we should actively use it!

This moves the file app away from the getMimeIcon function that did a request for every mimetype to the server

Testing is easy!

  1. Add some files for which we have mimetypes
  2. Reload files app and observe the console.. for each mimetype you should see at least 1 request to the server (ajax/mimeicon.php)
  3. Now checkout this awesome branch
  4. Reload files app
  5. Observe no calls to the server for each mimetype (but you still see them)

TODO:

  • Remove ajax/mimeicon.php (and its routes)
  • Migrate calls in core app to getMimeIcon to OC.MimeType.getIconUrl

@rullzer rullzer added this to the 8.2-current milestone Jul 8, 2015
@rullzer rullzer mentioned this pull request Jul 8, 2015
Closed
@RobinMcCorkell RobinMcCorkell changed the title Move away from OCA.files.getMimeIcon and use OC.MimeType.getIconUrl [WIP] Move away from OCA.files.getMimeIcon and use OC.MimeType.getIconUrl Jul 8, 2015
@rullzer
Copy link
Contributor Author

rullzer commented Jul 8, 2015

Would it be safe to remove ajax/mimeicon.php and its routes? Could other apps depend on that? I guess they can. Probably no real way to mark this deprecated then... :S

@PVince81
Copy link
Contributor

PVince81 commented Jul 9, 2015

Yes, please kill the ajax stuff with 🔥

@rullzer rullzer changed the title [WIP] Move away from OCA.files.getMimeIcon and use OC.MimeType.getIconUrl Move away from OCA.files.getMimeIcon and use OC.MimeType.getIconUrl Jul 9, 2015
@rullzer
Copy link
Contributor Author

rullzer commented Jul 9, 2015

Review time!

CC: @PVince81 @Xenopathic @MorrisJobke

@rullzer
Copy link
Contributor Author

rullzer commented Jul 9, 2015

@owncloud-bot retest this please

@PVince81
Copy link
Contributor

PVince81 commented Jul 9, 2015

  • TEST: regular file list icons
  • TEST: public file list icons
  • TEST: file picker
  • TEST: conflict dialog

@PVince81
Copy link
Contributor

PVince81 commented Jul 9, 2015

  • BUG: public page does not show mime icons

@rullzer please have a look

@rullzer
Copy link
Contributor Author

rullzer commented Jul 9, 2015

  • BUG: public page does not show mime icons

Seems to work here.
Which mime icons are not displayed?

@PVince81
Copy link
Contributor

PVince81 commented Jul 9, 2015

All of them 😉
public-nomime

@PVince81
Copy link
Contributor

PVince81 commented Jul 9, 2015

This is the file list from a public link.

Regular file list works fine.

@PVince81
Copy link
Contributor

PVince81 commented Jul 9, 2015

Rebase onto master, it's a regression caused by https://github.com/owncloud/core/pull/16902/files#diff-dafefd9519924bec4efa9f5af4af8e68R765

@MorrisJobke
Copy link
Contributor

I tested this and it works just fine: (chrome, firefox, IE8)

  • TEST: regular file list icons
  • TEST: public file list icons
  • TEST: file picker
  • TEST: conflict dialog

@MorrisJobke
Copy link
Contributor

👍

@rullzer
Copy link
Contributor Author

rullzer commented Jul 9, 2015

@MorrisJobke I did not rebase on master yet. But all should be fixed now.
@PVince81 please give it another go.

I fixed #17527 here as well.

@RobinMcCorkell
Copy link
Member

@rullzer @PVince81 Wouldn't an easier way to fix the 'httpd/unix-directory' mismatch be to add it to the aliases list? Then it works in PHP too, which is nice

@rullzer
Copy link
Contributor Author

rullzer commented Jul 9, 2015

@Xenopathic could also be done indeed.. but I can imagine people wanting to map stuff to `httpd/unix-directory'....

But fine with me both ways

@RobinMcCorkell
Copy link
Member

AFAIK we use the mimetype 'dir' all over the place, rather than 'httpd/unix-directory'.

@rullzer
MimeTypes now also supports httpd/unix-directory
eea56dc 
Since we both use dir and httpd/unix-directory in OC they should map to
the same icon.

Fixes #17527
@rullzer
Copy link
Contributor Author

rullzer commented Jul 9, 2015

@Xenopathic agreed. And fixed.

@rullzer
Copy link
Contributor Author

rullzer commented Jul 9, 2015

Ah need to fix the js unit tests... let me take care of that

@scrutinizer-notifier
Copy link

A new inspection was created.

@ghost
Copy link

ghost commented Jul 9, 2015

🚀 Test PASSed.🚀
chuck

@rullzer
Copy link
Contributor Author

rullzer commented Jul 9, 2015

Unit tests updated... all is fine now... I hope...
Review time!

@PVince81
Copy link
Contributor

All work 👍

PVince81 pushed a commit that referenced this pull request Jul 10, 2015
Merge pull request #17483 from rullzer/migrate-mimetype-js
87f3500 
Move away from OCA.files.getMimeIcon and use OC.MimeType.getIconUrl
@PVince81 PVince81 merged commit 87f3500 into owncloud:master Jul 10, 2015
@rullzer rullzer deleted the migrate-mimetype-js branch July 18, 2015 20:02
@jospoortvliet
Copy link

just curious - does this make loading folders faster, too? As you mention it doesn't have to do a request for mimetype for each file anymore...

@rullzer
Copy link
Contributor Author

rullzer commented Jul 27, 2015

@jospoortvliet should be a little snappier. But I did not run any timing benchmarks.

@jospoortvliet
Copy link

Just wanted to make sure I wrote no nonsense ;-) thanks!

@PVince81 PVince81 mentioned this pull request Aug 5, 2015
Closed
@lock lock bot locked as resolved and limited conversation to collaborators Aug 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
3 - To Review enhancement
Projects
None yet
Milestone
8.2
Development

Successfully merging this pull request may close these issues.
6 participants
@rullzer @PVince81 @MorrisJobke @RobinMcCorkell @scrutinizer-notifier @jospoortvliet