Closed Bug 302083 Opened 16 years ago Closed 16 years ago

Have bugzilla automatically convert uncompressed bmp attachments to png or some other compressed format

Categories

(Bugzilla :: Attachments & Requests, enhancement)

2.21
All
Linux
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.22

People

(Reporter: gregaryh, Assigned: gregaryh)

Details

Attachments

(1 file, 6 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: 

We have been running into this problem with our developers since they use MS
paint to capture their screenshots which they then upload to bugzilla. These
files often start at 4 MB and go up from there and it would be just as good for
a screenshot if it were to use png or gif. 



Reproducible: Always
Attached patch Patch v1 (tip) (obsolete) — Splinter Review
This patch uses Image::Magick to convert any and all bmp files to png as they
are uploaded. This is assuming that you do not care that all bmp images are
converted. It might be a nice thing for the future to add a checkbox on the
attachment UI to say whether you really want it converted but in our case we
saw no need.
Attachment #190472 - Flags: review?(mkanat)
Comment on attachment 190472 [details] [diff] [review]
Patch v1 (tip)


>+    {   
>+        name => 'Image::Magick',
>+        version => '0'
>+    }

I haven't ImageMagick installed and I certainly don't want to install it. If we
want this module, it should be optional. My r- because of that.
Attachment #190472 - Flags: review-
i like this :)

> It might be a nice thing for the future to add a checkbox on
> the attachment UI to say whether you really want it converted

i think that would be confusing.  perhaps it would be better to make it a global
parameter?


on windows, Image::Magick presents a problem -- the latest imagemagick installer
doesn't recognise the latest version of activestate perl, so the ppm isn't
installed, and you can't install it seperately via ppm.  not your problem, but
for now i can't test it on windows.
Assignee: attach-and-request → ghendricks
Status: UNCONFIRMED → NEW
Ever confirmed: true
Yeah, agreed with glob and LpSolit -- the module should be optional, and it
should be controllable through a global parameter. (No checkbox for the user --
it's user error that you're trying to prevent. :-) )

Remember to use underscores in the global parameter name. So it would be
named_like_this instead of namedlikethis.

Also, the end-user will need a message that their attachment has been converted
to PNG format, on the process_bug screen.
Hardware: PC → All
Version: unspecified → 2.21
Attachment #190472 - Flags: review?(mkanat) → review-
Attached patch Patch v2 (tip) (obsolete) — Splinter Review
Made the image magick module optional in checksetup and added a parameter to
turn it on or off. Also added a note to let the user know what happened.
Attachment #190472 - Attachment is obsolete: true
Attachment #190574 - Flags: review?(LpSolit)
Attachment #190574 - Flags: review?(mkanat)
Attachment #190574 - Flags: review?(LpSolit)
Attachment #190574 - Flags: review?
Attachment #190574 - Flags: review?
Attached patch Patch v3 (tip) (obsolete) — Splinter Review
Forgot to make the use Image::magick optional in attachment.cgi
Attachment #190574 - Attachment is obsolete: true
Attachment #190576 - Flags: review?(mkanat)
Attached patch Patch v4 (tip) (obsolete) — Splinter Review
This PERL ignoramus does not know the difference between use and require....
Or at least didn't ;)
Attachment #190576 - Attachment is obsolete: true
Attachment #190580 - Flags: review?(mkanat)
Attachment #190574 - Flags: review?(mkanat)
Attachment #190576 - Flags: review?(mkanat)
Comment on attachment 190580 [details] [diff] [review]
Patch v4 (tip)

You'll want to name the param something like convert_image_attachments or
convert_uncompressed_images in case we want to expand what it converts from/to
in the future.

Other than that, it *looks* OK to me.

Oh, except I prefer "new Image::Magick(magick => 'bmp')" instead of the
Image::Magick->new.

Beyond that, you'll want to get one of the attachment.cgi reviewers to look at
it.

The Reviewer List is on bugzilla.org under the developer resources section.
Attachment #190580 - Flags: review?(mkanat) → review-
Attached patch Patch v5 (tip) (obsolete) — Splinter Review
OK, renamed the parameter as per mkanat's suggestion. 
I am afraid I am not as familiar with the other way of instantiating an object,
and this way works so I am leaving that the way it is. 

Hope this is alright.
Attachment #190580 - Attachment is obsolete: true
Attachment #190895 - Flags: review?(myk)
I think it'd be a good idea to have defparams.pl check for Image::Magick when a
tweakparams person tries to switch convert_uncompressed_images on, just like the
user_verify_class checker check_user_verify_class checks for Net::LDAP.
Attached patch Patch v6 (tip) (obsolete) — Splinter Review
Added a checker as per comment 10
Attachment #190895 - Attachment is obsolete: true
Attachment #190962 - Flags: review?(myk)
Comment on attachment 190962 [details] [diff] [review]
Patch v6 (tip)

>+    # Windows screenshots are usually uncompressed BMP files which
>+    # makes for a quick way to eat up disk space. Let's compress them 
>+    # we do this before we check the size since the uncompressed version
>+    # could easily be greater than maxattachmentsize

Nits:
  Let's compress them -> Let's compress them.
  we do this -> We do this
  maxattachmentsize -> maxattachmentsize.


>+    if (Param('convert_uncompressed_images') == 1){
>+        require Image::Magick; 
>+        if ($cgi->param('contenttype') =~ 'image/bmp'){

Nit: since convert_uncompressed_images is really a boolean value (even if we
represent those as "1" right now):
  Param('convert_uncompressed_images') == 1 ->
Param('convert_uncompressed_images')

Also, to save Perl from interpreting Image::Magick when it isn't necessary,
this should be:

if (Param('convert_uncompressed_images')
    && $cgi->param('contenttype') =~ 'image/bmp')
{
    require Image::Magick; 

Finally, are there multiple content types like "image/bmp", or is it really
just "image/bmp"?  If the latter, the operator would make more sense as "eq"
than "=~".  If the former, we might want the right-hand operand to be a real
regular expression that matches specifically those content types.


>+if (!$imagemagick && !$silent) {
>+    print "If you want to convert bmp attachments to png to conserve disk\n",
>+          "space, you will need to install the Image::Magick module\n ",
>+          "by running (as $::root):\n\n",
>+    "   " . install_command("Image::Magick") . "\n\n";
>+}

Nits:
  bmp -> BMP
  png -> PNG
  attachments -> image attachments

Also, perhaps it would make sense to mention here that you need both the
application and the Perl module.  Perhaps something like this would work:

If you want to convert BMP attachments to PNG to conserve disk space, you will
need to install the ImageMagick application, available from
http://www.imagemagick.org/, and the Image::Magick Perl module, available by
running (as $::root):


>+   name => 'convert_uncompressed_images',
>+   desc => 'If this option is on, attachments with mime type image/bmp ' .
>+           'will be converted to png and compressed before uploading to'.
>+           'the database to conserve disk space.',

Nits:
  mime type -> content type
  png -> PNG
  uploading to -> being stored in


>+      [% IF convertedbmp %]
>+        <p>
>+          <b>Note:</b> [% terms.Bugzilla %] automatically converted your bmp file to a 
>+          compressed png format.
>+        </p>
>+      [% END %]

Nits:
  bmp -> BMP
  png -> PNG

Nit: it'd make sense to display this message before the message that says what
content type autodetect detected.  Otherwise you read that your file was
autodetected as PNG before you find out it was converted to PNG, which could
make you think you uploaded the wrong file or that it got mis-autodetected.

Otherwise this looks great and is a welcome addition to attachments usability.
Attachment #190962 - Flags: review?(myk) → review-
Attached patch Patch v7 (tip)Splinter Review
Addressed the nits
Attachment #190962 - Attachment is obsolete: true
Attachment #190977 - Flags: review?(myk)
Comment on attachment 190977 [details] [diff] [review]
Patch v7 (tip)

>+if (!$imagemagick && !$silent) {
>+    print "If you want to convert BMP image attachments to PNG to conserve\n",
>+          "disk space, you will need to install the ImageMagick application\n ",
>+          "Available from http://www.imagemagick.org, and the Image::Magick",

Nit:
ImageMagick application Available from http://www.imagemagick.org,
->
ImageMagick application, available from http://www.imagemagick.org,

Otherwise looks good. r=myk
Attachment #190977 - Flags: review?(myk) → review+
Flags: approval+
Target Milestone: --- → Bugzilla 2.22
Checking in on Greg's behalf.  Thanks for the patch, Greg!

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.90; previous revision: 1.89
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.419; previous revision: 1.418
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.162; previous revision: 1.161
done
Checking in template/en/default/attachment/created.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/created.html.tmpl,v
 <--  created.html.tmpl
new revision: 1.11; previous revision: 1.10
done
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Keywords: relnote
Attachment #190895 - Flags: review?(myk)
Added to the Bugzilla 2.22 Release Notes in bug 322960.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.