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

U4-7312-2 #937

Merged
merged 5 commits into from
Jan 12, 2016
Merged

U4-7312-2 #937

merged 5 commits into from
Jan 12, 2016

Conversation

davidwincent
Copy link
Contributor

…and fileupload.controller.js

Changed ImageController.GetBigThumbnail to redirect
Changed thumbnail paths to include rnd equal to UpdateDate
/// </remarks>
public HttpResponseMessage GetResized(string imagePath, int width)
{
var media = Services.MediaService.GetMediaByPath( imagePath );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Shazwazza - I think there is a problem here.

The Services.MediaService.GetMediaByPath method expects the media path to be stored in a property type with alias 'umbracoFile'.
However the Umbraco.UploadField property editor uses the alias 'fileUpload'

Could this method by changed to support other aliases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, I just realized that the GetBigThumbnail method needs to work for non-media files. Will provide a fix for that.

@davidwincent
Copy link
Contributor Author

I have tested to following:

  • The media library, media thumbnails can now be cached be the browser
  • The media picker, media thumbnails can now be cached be the browser
  • The standard image upload property editor, seems to be working fine
  • The grid, seems to be working fine
  • the image cropper, seems to be working fine
  • any other place where a media image is being rendered, should be fine since the input format of GetBigThumbnail has not changed

@davidwincent
Copy link
Contributor Author

This commit conflicts witch changes in ImagesController:
1bf2a3b

This pull request delegates thumbnail generation to Image Processor and therefore any logic based on file extension should not be needed here?

…U4-7312-2

Conflicts:
	src/Umbraco.Web/Editors/ImagesController.cs - Removed changes from upstream
@Shazwazza
Copy link
Contributor

@davidwincent yup i'll review this PR, the other commit, the impact it has on 7.4 final


return result;
var response = Request.CreateResponse( HttpStatusCode.Found );
response.Headers.Location = new Uri( string.Format( "{0}?rnd={1}&width={2}", imagePath, string.Format( "{0:yyyyMMddHHmmss}", System.IO.File.GetLastWriteTime( fullOrgPath ) ), width ), UriKind.Relative );
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work unfortunately. The purpose of IFileSystem (i.e. the mediaFileSystem) is to abstract away the physical file system so that media can be stored anywhere (i.e. Azure blob storage). We cannot mix IFileSystem with the physical file system (i.e. System.IO.File), you need to use the mediaFileSystem to get whatever information you need about the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you can push a fix for this, I can test and try to get this in to 7.4.0 which is due for release very early Jan.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will look into a fix soon.

@davidwincent
Copy link
Contributor Author

Removed dependency to System.IO.File

@nul800sebastiaan nul800sebastiaan merged commit 96b24d7 into umbraco:dev-v7 Jan 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants