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

Feature/5101 ck editor resize images on upload #5128

Conversation

skamphuis
Copy link
Contributor

This PR is a redo of #5123 because of merge/rebase issues. As @valadas advised, I cherry picked the relevant commits, and gave it a quick test based on a fresh 9.11.0 install.

Closes #5101
This PR is built on PR #5122 which was submitted for #5100, but is not necessarily related.

Summary

This PR adds settings for:

  • Image Resize Width On Upload
  • Image Resize Height On Upload
    And will resize any uploaded image to fit within these dimension on upload.

Since pictures are becoming larger and larger, it can be important on many portals, to have this automatic resize option, to allow users uncapable of resizing images themselves, to upload reasonably sized images.
image

@valadas
Copy link
Contributor

valadas commented May 30, 2022

I have seen the failed test fail for no reason so re-firing the build to see...

@valadas
Copy link
Contributor

valadas commented May 30, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@skamphuis
Copy link
Contributor Author

I have seen the failed test fail for no reason so re-firing the build to see...

seems to have worked now

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Cool, cool, so looks good to me then

@valadas valadas added this to the 9.11.0 milestone May 30, 2022
Copy link
Contributor

@bdukes bdukes left a comment

Choose a reason for hiding this comment

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

I'd like a couple of the manual Dispose calls converted to using statements (to avoid things not getting closed if there are errors), otherwise it looks good to me.

Comment on lines 2510 to 2512
var fileStream = FileManager.Instance.GetFileContent(uploadedFile);
var uplImage = Image.FromStream(fileStream);
fileStream.Close();
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to be disposed, right?

Suggested change
var fileStream = FileManager.Instance.GetFileContent(uploadedFile);
var uplImage = Image.FromStream(fileStream);
fileStream.Close();
using (var fileStream = FileManager.Instance.GetFileContent(uploadedFile))
using (var uplImage = Image.FromStream(fileStream))
{

using (Image newImage = uplImage.GetThumbnailImage(newWidth, newHeight, null, IntPtr.Zero))
{
var imageFormat = uplImage.RawFormat;
uplImage.Dispose();
Copy link
Contributor

Choose a reason for hiding this comment

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

Switching to using from Dipose()

Suggested change
uplImage.Dispose();

@skamphuis
Copy link
Contributor Author

@bdukes thanks for your comments, Do I need to commit your changes? Or did you already do the changes? (I'm a bit scared to mess it up again :-) )

@bdukes
Copy link
Contributor

bdukes commented Jun 2, 2022

I have not applied the changes, you can go ahead and do that. Thanks!

@skamphuis
Copy link
Contributor Author

I have not applied the changes, you can go ahead and do that. Thanks!

Did that just now, tnx

@valadas
Copy link
Contributor

valadas commented Jun 2, 2022

Awesome, thanks @skamphuis

@valadas valadas merged commit 7e8f6fa into dnnsoftware:release/9.11.0 Jun 2, 2022
@skamphuis skamphuis deleted the feature/5101-CKEditor-resize-images-on-upload branch November 23, 2022 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants