The BMP -> PNG conversion tool for new attachments should be an extension

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
Attachments & Requests
--
enhancement
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Frédéric Buclin, Assigned: Frédéric Buclin)

Tracking

3.3.3
Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

9 years ago
I have no idea how much this feature is used, but I think it should be a plugin. Image Magick seems to be a pain to install on some installations, and install-module.pl should not try to install it by default, even when called with -all.
(Assignee)

Comment 1

9 years ago
For the record, the code about Image::Magick is located in Bugzilla/Attachment.pm in _validate_data() around line 502.
(Assignee)

Comment 2

8 years ago
Created attachment 392999 [details] [diff] [review]
patch, v1

I put the BMP -> PNG conversion into the 'example' extension. The extension is fully functional if you uncomment the last two lines. The parameter is now obsolete as if someone installs the extension, this means he wants it. I also handle the case where Image::Magick is not installed. Nice cleanup! :)
Assignee: attach-and-request → LpSolit
Status: NEW → ASSIGNED
Attachment #392999 - Flags: review?(mkanat)
(Assignee)

Updated

8 years ago
Target Milestone: --- → Bugzilla 3.6
(Assignee)

Comment 3

8 years ago
Created attachment 393000 [details] [diff] [review]
patch, v1.1

Forgot to add |use Bugzilla::Hook| in Attachment.pm.
Attachment #392999 - Attachment is obsolete: true
Attachment #393000 - Flags: review?(mkanat)
Attachment #392999 - Flags: review?(mkanat)
(Assignee)

Updated

8 years ago
Blocks: 357661
(Assignee)

Updated

8 years ago
Blocks: 340525
Comment on attachment 393000 [details] [diff] [review]
patch, v1.1

>Index: extensions/example/code/attachment-process_data.pl
>+# This extension automatically converts BMP images into PNG.
>+return unless $args->{attributes}->{mimetype} eq 'image/bmp';
>+eval { require Image::Magick; };
>+return if $@;
>+
>+my $data = $args->{attach_data};
>+my $img = Image::Magick->new(magick=>'bmp');
>+$img->BlobToImage($$data);
>+$img->set(magick=>'png');
>+my $imgdata = $img->ImageToBlob();
>+# Uncomment these two lines to enable the BMP -> PNG conversion.
>+# $$data = $imgdata;
>+# $args->{attributes}->{mimetype} = 'image/png';

If I leave the last two lines commented (i.e. I don't want conversion): does this mean that if Image::Magick is installed, then the conversion *always* takes place? It just is discarded afterwards?
(Assignee)

Comment 5

8 years ago
(In reply to comment #4)
> If I leave the last two lines commented (i.e. I don't want conversion): does
> this mean that if Image::Magick is installed, then the conversion *always*
> takes place?

If you removed extensions/example/disabled, yes, else this extension is never used.
(Assignee)

Comment 6

8 years ago
(In reply to comment #4)
> If I leave the last two lines commented (i.e. I don't want conversion): does
> this mean that if Image::Magick is installed, then the conversion *always*
> takes place?

Oh, and note that even with extensions/example/disabled removed, you trigger this code only if the attachment is a BMP image. Else we return earlier. So that's fine IMO.

Comment 7

8 years ago
Comment on attachment 393000 [details] [diff] [review]
patch, v1.1

I haven't looked at the patch, but this should be in its own extension, not in the example/ extension.
(Assignee)

Comment 8

8 years ago
(In reply to comment #7)
> I haven't looked at the patch, but this should be in its own extension, not in
> the example/ extension.

Why not? It's an concrete example to show how to do this. People are then free to copy this example in their own extension.

Comment 9

8 years ago
It's a functional plugin, it should be separate. You don't want people to enable the example plugin on a production machine, ever. It does things like insert the word "EXAMPLE" in random places in the UI.
(Assignee)

Comment 10

8 years ago
(In reply to comment #9)
> It's a functional plugin, it should be separate. You don't want people to
> enable the example plugin on a production machine, ever. It does things like
> insert the word "EXAMPLE" in random places in the UI.

It's a functional plugin, yes. It's also a feature we are removing from Bugzilla, and so we should make it easily accessible. I never said people should enable the example plugin, I said they should *copy* my code into their own extension.
I think it should be a function plugin as well, not a example. To me the statement "The extension is fully functional if you uncomment the last two lines" kinda clinches the issue of it being an example or not.

Semi off subject but thought of due to this thread... Do we have a concept for bundled extensions/plug-ins? Doesn't seem like it. How about a new plugins control page. Extensions like this one, could be bundled or detected and be configurable as enabled/disabled. It would be nice to allow for bundled plugins that could be enabled by Admins. And would prompt the usage of bugzilla as a versatile platform. The additional of extensions within bugzilla was a good first step but it still not truly intuitive for a ... hmm.. newbie to use or administer.

The model I'm thinking of can be see in most open source blogging software and wikis such as http://twiki.net.
re comment 11 prompt should be promote

Comment 13

8 years ago
"Copy my code into their extension" is not a good configuration plan for admins.

Just make a new extension that is shipped with Bugzilla itself, disabled by default. We can call it bmpcompress.

The attachment hook (which should go into a separate bug) should also have its own example code.
(Assignee)

Comment 14

8 years ago
Created attachment 393169 [details] [diff] [review]
patch, v2

This is now a separate fully functional plugin.
Attachment #393000 - Attachment is obsolete: true
Attachment #393169 - Flags: review?(mkanat)
Attachment #393000 - Flags: review?(mkanat)
(In reply to comment #5)
> If you removed extensions/example/disabled, yes, else this extension is never
> used.

Ok, I wasn't aware of extensions/example/disabled. Great, thanks!
(Assignee)

Updated

8 years ago
Depends on: 509027
(Assignee)

Comment 16

8 years ago
Created attachment 394209 [details] [diff] [review]
patch, v3
Attachment #393169 - Attachment is obsolete: true
Attachment #394209 - Flags: review?(mkanat)
Attachment #393169 - Flags: review?(mkanat)

Updated

8 years ago
Attachment #394209 - Flags: review?(mkanat) → review-

Comment 17

8 years ago
Comment on attachment 394209 [details] [diff] [review]
patch, v3

>+{ name        => 'BMP to PNG converter',
>+  version     => '1.0',
>+  description => 'Automatically converts BMP images to the PNG format',
>+  x_author    => 'Greg Hendricks, Frédéric Buclin',

  Let's not use name, description, and author yet. We can add them in a separate bug, where we document them specifically.

>Index: extensions/bmp_convert/code/attachment-process_data.pl
>+my $args = Bugzilla->hook_args;
>+return unless $args->{attributes}->{mimetype} eq 'image/bmp';

  I don't think it's safe to call return here. Does this throw a warning?

  Probably a better thing to do would be to create a module in lib/ and call a subroutine from it. (This is what I did for all the hooks in Traceparser, which is an extension that I should be releasing sometime shortly.)

>+my $data = ${$args->{data}};

  You're making an in-memory copy of what could be a huge file, here.

>+# We need the attachment content as we are editing it.
>+if (ref $data) {
>+    local $/;
>+    $data = <$data>;

  Same there. I would have to imagine that Image::Magick has functionality to work with filehandles and references, no?
(Assignee)

Comment 18

8 years ago
(In reply to comment #17)
>   Let's not use name, description, and author yet. We can add them in a
> separate bug, where we document them specifically.

I filed bug 510181. Meanwhile, I will prepend x_ to both field names.


>   I don't think it's safe to call return here. Does this throw a warning?

It's safe, and no warning is thrown, see |perldoc -f return|. It explicitly mentions calling return from within a do {} block. No reason to have a separate subroutine for this.
Summary: The BMP -> PNG conversion tool for new attachments should be a plugin → The BMP -> PNG conversion tool for new attachments should be an extension
(Assignee)

Comment 19

8 years ago
Created attachment 394255 [details] [diff] [review]
patch, v4

I no longer load the content of the filehandle in memory if we get one.
Attachment #394209 - Attachment is obsolete: true
Attachment #394255 - Flags: review?(mkanat)

Comment 20

8 years ago
(In reply to comment #18)
> It's safe, and no warning is thrown, see |perldoc -f return|. It explicitly
> mentions calling return from within a do {} block. No reason to have a separate
> subroutine for this.

  Okay.

Comment 21

8 years ago
Comment on attachment 394255 [details] [diff] [review]
patch, v4

>+my $data = ${$args->{data}};

  Hmm, unfortunately, I don't see any way to get around this in the ImageMagick docs. (Unfortunately, this creates an in-memory copy of the data if you're using blobs.)
Attachment #394255 - Flags: review?(mkanat) → review+

Updated

8 years ago
Flags: approval+
(Assignee)

Comment 22

8 years ago
Checking in Bugzilla/Attachment.pm;                                    
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.65; previous revision: 1.64                                    
done                                                                           
Checking in Bugzilla/Config/Attachment.pm;                                     
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Attachment.pm,v  <--  Attachment.pm
new revision: 1.8; previous revision: 1.7                                             
done                                                                                  
Checking in Bugzilla/Config/Common.pm;                                                
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v  <--  Common.pm        
new revision: 1.26; previous revision: 1.25                                           
done                                                                                  
Checking in Bugzilla/Install/Requirements.pm;                                         
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm                                                                                         
new revision: 1.67; previous revision: 1.66                                               
done                                                                                      
Checking in docs/en/xml/installation.xml;                                                 
/cvsroot/mozilla/webtools/bugzilla/docs/en/xml/installation.xml,v  <--  installation.xml  
new revision: 1.169; previous revision: 1.168
done
Checking in docs/en/xml/modules.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/en/xml/modules.xml,v  <--  modules.xml
new revision: 1.14; previous revision: 1.13
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/extensions/bmp_convert/disabled,v
done
Checking in extensions/bmp_convert/disabled;
/cvsroot/mozilla/webtools/bugzilla/extensions/bmp_convert/disabled,v  <--  disabled
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/extensions/bmp_convert/info.pl,v
done
Checking in extensions/bmp_convert/info.pl;
/cvsroot/mozilla/webtools/bugzilla/extensions/bmp_convert/info.pl,v  <--  info.pl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/extensions/bmp_convert/code/attachment-process_data.pl,v
done
Checking in extensions/bmp_convert/code/attachment-process_data.pl;
/cvsroot/mozilla/webtools/bugzilla/extensions/bmp_convert/code/attachment-process_data.pl,v  <--  attachment-process_data.pl
initial revision: 1.1
done
Checking in template/en/default/admin/params/attachment.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/attachment.html.tmpl,v  <--  attachment.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/setup/strings.txt.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/setup/strings.txt.pl,v  <--  strings.txt.pl
new revision: 1.14; previous revision: 1.13
done
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: relnote
Resolution: --- → FIXED

Updated

8 years ago
Blocks: 372974

Comment 23

8 years ago
Added to the release notes in bug 547466.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.