rmagick / rmagick

Ruby bindings for ImageMagick
https://rmagick.github.io/
MIT License
696 stars 140 forks source link

Added missing -alpha remove flag #192

Closed ollie closed 9 years ago

ollie commented 9 years ago

Hello,

I tried to add the -alpha remove flag to close #63. Is it correct? Do I need to add tests or other things? I pretty much have no C experience but the change I did works.

Thanks.

vassilevsky commented 9 years ago

Looks like one cannot simply add a new identifier :(

https://travis-ci.org/gemhome/rmagick/jobs/59717747#L5096

ollie commented 9 years ago

Humm, what now though? I can't really do anything more. :disappointed:

vassilevsky commented 9 years ago

Thank you for making this PR! It's much better than nothing.

Maybe @u338steven knows what to do with it?

u338steven commented 9 years ago

@ollie Thanks for your PR!

ImageMagick 6.6.9-10 does not have RemoveAlphaChannel, so it should be as below.

rmmain.c:

#if defined(HAVE_ENUM_REMOVEALPHACHANNEL)
        ENUMERATOR(RemoveAlphaChannel)
#endif

about HAVEENUM*

On RMagick, extconf.rb generate a header extconf.h.

e.g. CopyAlphaChannel

extconf.rb:

      have_enum_values('AlphaChannelType', ['CopyAlphaChannel',                    # 6.4.3-7
                                            'BackgroundAlphaChannel'], headers)    # 6.5.2-5

If ImageMagick has CopyAlphaChannel, extconf.h will be like this:

#ifndef EXTCONF_H
#define EXTCONF_H
...
#define HAVE_ENUM_COPYALPHACHANNEL 1
...

HAVE_ENUM_COPYALPHACHANNEL is used in rmmain.c:

#if defined(HAVE_ENUM_COPYALPHACHANNEL)
        ENUMERATOR(CopyAlphaChannel)
        ENUMERATOR(ExtractAlphaChannel)
        ENUMERATOR(OpaqueAlphaChannel)
        ENUMERATOR(ShapeAlphaChannel)
        ENUMERATOR(TransparentAlphaChannel)
#endif

Therefore, there are no problems even if ImageMagick does not have CopyAlphaChannel.

vassilevsky commented 9 years ago

You are a SUPERHERO! :D

ollie commented 9 years ago

Thanks @u338steven for the info, it makes more sense now. :smile: I've added the check, let's see if it passes.

ollie commented 9 years ago

Funny thing, looks like this test is failing now:

# test/Magick.rb:54
def test_enumerators
  ary = nil
  assert_nothing_raised do
    ary = Magick::AlphaChannelType.enumerators
  end
  assert_instance_of(Array, ary)
  assert_equal(12, ary.length)
  # ...

How do I make it 11 for versions where remove is not supported and 12 where it is? :question:

vassilevsky commented 9 years ago

I suggest deleting that line. It's not helping.

ollie commented 9 years ago

Ok done. The tests now seem to segfault or fail with failed to allocate memory (same thing?). Is that normal? :D

vassilevsky commented 9 years ago

I heard that this is the expected behaviour for C programs :)

Nevertheless, let them finish.

vassilevsky commented 9 years ago

Do you know a hosted CI free for OSS that can do similar build matrix but faster?

ollie commented 9 years ago

Not really, but a quick search revealed this https://en.wikipedia.org/wiki/Comparison_of_continuous_integration_software and this https://www.quora.com/What-are-the-alternatives-to-Travis-CI. It's probably a matter of picking something and trying it out.

vassilevsky commented 9 years ago

Close enough!

vassilevsky commented 9 years ago

Thanks Oldřich! You are welcome to send in any other pull requests!

ollie commented 9 years ago

No problem, thanks for the help, too.