Add theme support to mimetypeIcon through imagePath integration

[Egari]

Added theme support to mimetype icons by using the pre-existing imagePath function

[ghost]

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

[Egari]

This contribution may be used under the MIT license.

[karlitschek]

@jancborchardt @DeepDiver1975 What do you think?

[jancborchardt]

So this fixes #15320?

@krempelweb @FunkyM can you test this?

[jancborchardt]

@Egari also, is this similar to a fix for #6773 maybe? :)

[Egari]

@jancborchardt First of all, thank you for labeling 👍
Second, yes, it will indeed fix #15320.
Regarding #6773, in those cases the app tries to load images using the pure-javascript OC.imagePath and OC.filePath functions, which will not use any images and/or files that might be available in your theme. Since the pure-javascript OC.imagePath function does not actually use serverside code, it's more difficult to check for existing files. A possible change for this is to let apps do an ajax call to a PHP server-side url generator (or OC_Helper in this case).
So in short, unfortunately no.

[MorrisJobke]

@Egari Can I ask you to use tabs for intendation? See https://doc.owncloud.org/server/8.1/developer_manual/general/codingguidelines.html#coding

This also would make the diff more pretty.

Thanks

[Egari]

@MorrisJobke Of course, my apologies. Updated.

[nickvergessen]

@DeepDiver1975 8.1?

[DeepDiver1975]

@Egari can I ask you to squash the commits? THX

[DeepDiver1975]

@DeepDiver1975 8.1?

hmm .... better not -> 8.2

[jancborchardt]

I’d also say 8.2 since it has the potential to break stuff.

[Egari]

@DeepDiver1975 squashed into single commit, sorry for the delay.

[RobinMcCorkell]

This is effectively duplicated code from UrlGenerator (which is where self::imagePath() goes). Instead of doing all the file_exists() stuff here, just call self::imagePath(), then if there is a result it must exist (as per UrlGenerator::imagePath()). Note that imagePath() will throw a RuntimeException if the image can't be found, which provides a nice clean way of implementing this:

try {
    // Icon exists?
    self::$mimetypeIcons[$mimetype] = self::imagePath(...);
    return self::$mimetypeIcons[$mimetype];
} catch (\RuntimeException $e) {
}

try {
    // Try only the first part of the filetype
    self::$mimetypeIcons[$mimetype] = self::imagePath(...);
    return self::$mimetypeIcons[$mimetype];
} catch (\RuntimeException $e) {
}

self::$mimetypeIcons[$mimetype] = self::imagePath(... 'file.png');
return self::$mimetypeIcons[$mimetype];

[Egari]

If Exception-catching (something I'm personally not a fan of) is preferred over code duplication, I will update the pull request when I get the chance.

[RobinMcCorkell]

Code duplication in itself isn't necessarily a bad thing, but when code that has the identical function is split over two files, then it is a recipe for disaster and inconsistency. The exception plumbing is already there in the imagePath() function, and it'd be silly to just ignore it. 😄

[RobinMcCorkell]

👍

[nickvergessen]

intendation is off

[nickvergessen]

intendation is off

[nickvergessen]

okay actually you use spaces instead of tabs

[Egari]

@nickvergessen Darn, I'll .vimrc the thing... I have it set to spaces by default, and I mustve forgotten to noexpandtab... Fixed code.

[nickvergessen]

Thanks, sorry for the troubles

[jancborchardt]

@vgiannoul Please review this as it probably fixes your #16547

[RobinMcCorkell]

My 👍 still stands

[vgiannoul]

maybe I'm wrong or this is my fault but I get an error when applying this patch

Fatal error: Call to undefined method OC_Helper::userAvatarSet() in owncloud/lib/private/templatelayout.php on line 80

and I have to comment out this line in order to fix it. I have enabled userAvatars in my configuration

[Egari]

That function is defined not 20 lines below the end of the changed function;
https://github.com/Egari/core/blob/addThemeSupportToMimeIcon/lib/private/helper.php#L327

Did something go wrong applying the patch? Conflicting local changes?

[vgiannoul]

Sorry for late response,
i continue to get this error message
Fatal error: Class 'OC\Avatar' not found in /Users/vgiannoul/Sites/owncloud/lib/private/helper.php on line 328
after applying the patch on a clean install (version 8.0.3)

[DeepDiver1975]

This patch can only be applied against master.

[PVince81]

@rullzer what do you think ? (in regard to your other changes regarding mimetypes)

[PVince81]

@owncloud-bot ok to test

[rullzer]

I can't check but moving to SVG here by default might break some browsers..

[rullzer]

O and don't use self::imagePath!!! That function is deprecated.
Use \OC::$server->getURLGenerator()->imagePath($app, $image)

[rullzer]

This of course hold in the other cases as well

[Egari]

You're quite right, updated to remove the use of self::imagePath and revert the change to use svg by default.
Thanks for the feedback! 👍

[rullzer]

I would love to drop the PNGs... but unfortunatly we can't.... (yet)

[scrutinizer-notifier]

A new inspection was created.

[ghost]

🚀 Test PASSed.🚀

[rullzer]

Looking good and working properly for themes.
I still think this function needs to be migrated (#15543). But this theming stuff should be fixed!

👍

[MorrisJobke]

Tested and works 👍