imagemagick branch

imagemagick branch

[joakim]

Hello,

I have done some minor improvements to the Imagemagick branch, and it
would be nice if it could receive some testing. It would be especially
interesting to see how it behaves on more plattforms. Basically Fedora
12 works well, but Ubuntu 8.04 doesnt seem to work well due to a too old
Imagemagick version.

There is a README in the branch with more information, comments on that
would be nice as well.
-- 
Joakim Verona

Re: imagemagick branch

[Eli Zaretskii]

> From: joakim@verona.se
> Date: Tue, 18 May 2010 10:22:57 +0200
> 
> I have done some minor improvements to the Imagemagick branch, and it
> would be nice if it could receive some testing.

Are there any reasons not to merge it with the trunk?  If someone
doesn't use Imagemagick, would they sense any difference or execute
any of the code you added?

Re: imagemagick branch

[joakim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: joakim@verona.se
>> Date: Tue, 18 May 2010 10:22:57 +0200
>> 
>> I have done some minor improvements to the Imagemagick branch, and it
>> would be nice if it could receive some testing.
>
> Are there any reasons not to merge it with the trunk?  If someone
> doesn't use Imagemagick, would they sense any difference or execute
> any of the code you added?

Yes, the code could probably safely be added to trunk. Even if one
activates imagemagick with "configure --with-imagemagick=yes"
imagemagick wont kick in unless you execute (imagemagick-register-types)

Another question then, how do I merge best to trunk? I'm reading:
http://www.emacswiki.org/emacs/BzrForEmacsDevs#MergeToUpstream
but it suggests removing my local branch after merging.

I would like to continue to add some more experimental functions to
the branch, and later merge these to trunk. Would that also be ok?
Or would it be better to create a new branch for this purpose?

What I would like to do is experiment with some new keybindings in
image-mode for zooming, switching the indexed file in a multifile bundle
and so on.


-- 
Joakim Verona

Re: imagemagick branch

[Eli Zaretskii]

> From: joakim@verona.se
> Cc: emacs-devel@gnu.org
> Date: Tue, 18 May 2010 12:41:38 +0200
> 
> Yes, the code could probably safely be added to trunk. Even if one
> activates imagemagick with "configure --with-imagemagick=yes"
> imagemagick wont kick in unless you execute (imagemagick-register-types)

Then I suggest to talk to Stefan and Yidong and ask for their
permission to merge.

> Another question then, how do I merge best to trunk? I'm reading:
> http://www.emacswiki.org/emacs/BzrForEmacsDevs#MergeToUpstream
> but it suggests removing my local branch after merging.
> 
> I would like to continue to add some more experimental functions to
> the branch, and later merge these to trunk. Would that also be ok?
> Or would it be better to create a new branch for this purpose?

You can keep the old branch, but it would be best to resync it to
trunk after you merge your current code with the trunk.  Something
like this:

  cd ../trunk
  bzr up
  bzr merge ../imagemagick
  # resolve conflicts, if any
  # build, fix any build errors
  # test, fix any bugs resulting from merge
  bzr ci

The last "bzr ci" could fail if someone committed to the master
repository in the meantime.  IN that case, you will need another
"bzr up" before "bzr ci".

At this point, trunk has the same code you have on your branch.  Now
resync the branch with the trunk:

  cd ../imagemagick
  bzr pull --overwrite

Now your branch is identical to the trunk, and you can continue
development in it.  When you are done, merge again with the trunk,
as shown above.

Note that "pull --overwrite" will overwrite any changes in your branch
that were not merged to the trunk.  I assume from your description
that there will be no such changes.  If you do have changes whcih you
want not to merge at this point, then just continue developing in the
imagemagick branch without the "pull --overwrite" step.  Then, when
you are done with another batch of changes, merge with trunk again, as
shown above, and commit to the master repository.

Re: imagemagick branch

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

>> Yes, the code could probably safely be added to trunk. Even if one
>> activates imagemagick with "configure --with-imagemagick=yes"
>> imagemagick wont kick in unless you execute (imagemagick-register-types)
>
> Then I suggest to talk to Stefan and Yidong and ask for their
> permission to merge.

I think you should go ahead and merge into the trunk.

Does the imagemagick support work on Windows and OS X?  It would be nice
to have a cross-platform solution that allows Emacs to render SVG.  This
would allow us to replace many of the bitmap images currently used in
Emacs (which are getting a little long in the tooth by modern GUI
standards).

Re: imagemagick branch

[Stefan Monnier]

> Yes, the code could probably safely be added to trunk. Even if one
> activates imagemagick with "configure --with-imagemagick=yes"
> imagemagick wont kick in unless you execute (imagemagick-register-types)

I just took a quick look at the code and I see the following nits to fix:
- obviously a merge will have to come with a good ChangeLog.
- also the merge will need to come with documentation.  Maybe not in the
  Texinfo form yet, but at least in the etc/NEWS with enough info that
  describes the `scale' and other such arguments that someone can start
  using them.
- the README talks about naming inconsistencies, I think these should be
  fixed before a first commit (should be straightforward).
- the "let" in image.el should not be followed by a line break and the while
  should be replaced by a dolist.
- the prototype of imagemagick_load_image has some odd indentation in
  its args, not sure what happened.
- a few lines in the C code break the 80columns limit.
- please use ANSI style function declarations rather than K&R for new code.
- you can get rid of the prototypes by reordering the code.
- the docstrings in DEFUN should not be indented (they'll display
  weirdly otherwise in C-h f).
- Some "{" are at the end of a for/if rather than on their own line.
- why use "*( imtypes + i)" rather than "imtypes[i]"?
- some "," lack a space after them.
- several "=" and "==" lack spaces around them.

> Another question then, how do I merge best to trunk? I'm reading:
> http://www.emacswiki.org/emacs/BzrForEmacsDevs#MergeToUpstream
> but it suggests removing my local branch after merging.

The branch's history is not really interesting it seems, so you might as
well do the first commit on trunk as a separate commit rather as a merge.

But if you prefer a merge, see Eli's answer.

> I would like to continue to add some more experimental functions to
> the branch, and later merge these to trunk. Would that also be ok?
> Or would it be better to create a new branch for this purpose?

No need for a new branch.


        Stefan

Re: imagemagick branch

[joakim]

Chong Yidong <cyd@stupidchicken.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Yes, the code could probably safely be added to trunk. Even if one
>>> activates imagemagick with "configure --with-imagemagick=yes"
>>> imagemagick wont kick in unless you execute (imagemagick-register-types)
>>
>> Then I suggest to talk to Stefan and Yidong and ask for their
>> permission to merge.
>
> I think you should go ahead and merge into the trunk.
>
> Does the imagemagick support work on Windows and OS X?  It would be nice
> to have a cross-platform solution that allows Emacs to render SVG.  This
> would allow us to replace many of the bitmap images currently used in
> Emacs (which are getting a little long in the tooth by modern GUI
> standards).

I would be interested to know if it works on Windows and OS X as well.
I dont have those development environments handy at the moment.

-- 
Joakim Verona

Re: imagemagick branch

[joakim]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Yes, the code could probably safely be added to trunk. Even if one
>> activates imagemagick with "configure --with-imagemagick=yes"
>> imagemagick wont kick in unless you execute (imagemagick-register-types)
>
> I just took a quick look at the code and I see the following nits to fix:
> - obviously a merge will have to come with a good ChangeLog.
> - also the merge will need to come with documentation.  Maybe not in the
>   Texinfo form yet, but at least in the etc/NEWS with enough info that
>   describes the `scale' and other such arguments that someone can start
>   using them.
> - the README talks about naming inconsistencies, I think these should be
>   fixed before a first commit (should be straightforward).
> - the "let" in image.el should not be followed by a line break and the while
>   should be replaced by a dolist.
> - the prototype of imagemagick_load_image has some odd indentation in
>   its args, not sure what happened.
> - a few lines in the C code break the 80columns limit.
> - please use ANSI style function declarations rather than K&R for new code.
> - you can get rid of the prototypes by reordering the code.
> - the docstrings in DEFUN should not be indented (they'll display
>   weirdly otherwise in C-h f).
> - Some "{" are at the end of a for/if rather than on their own line.
> - why use "*( imtypes + i)" rather than "imtypes[i]"?
> - some "," lack a space after them.
> - several "=" and "==" lack spaces around them.

I refreshed the branch and tried to address the issues you raised.
Could you have a look again to see if its merge-worthy?

>
>> Another question then, how do I merge best to trunk? I'm reading:
>> http://www.emacswiki.org/emacs/BzrForEmacsDevs#MergeToUpstream> but it suggests removing my local branch after merging.
>
> The branch's history is not really interesting it seems, so you might as
> well do the first commit on trunk as a separate commit rather as a merge.
>
> But if you prefer a merge, see Eli's answer.
>
>> I would like to continue to add some more experimental functions to
>> the branch, and later merge these to trunk. Would that also be ok?
>> Or would it be better to create a new branch for this purpose?
>
> No need for a new branch.
>
>
>         Stefan
-- 
Joakim Verona