[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#22453: [PATCH] Support for switching to hexl-mode from image mode
From: |
Alexander Kuleshov |
Subject: |
bug#22453: [PATCH] Support for switching to hexl-mode from image mode |
Date: |
Sun, 24 Jan 2016 20:59:41 +0600 |
User-agent: |
Mutt/1.6.0-rc0 ((null)) |
Hello, first of all thanks for your feedback.
On 01-24-16, Eli Zaretskii wrote:
> > From: Alexander Kuleshov <kuleshovmail@gmail.com>
> > Date: Sun, 24 Jan 2016 12:30:36 +0600
> > Cc: Alexander Kuleshov <kuleshovmail@gmail.com>
> >
> > We can easily switch to text mode from the image mode by the
> > pressing of C-c C-c. But sometimes, it is more useful to open
> > an image in hex format. This patch provides new keybinding
> > for the image mode - C-c C-x which works like C-c C-c, but
> > executes switch to the hexl-mode. Like switching to text mode,
> > switching to hex mode supports switching back to the image-mode.
> >
> > The patch contains following changes:
> > ...
> > ...
> > ...
> >
> > Patch tested on top of emacs-25 branch on fedora 23 with all
> > Linux related configured options.
>
> Thanks. This generally looks good. A few comments:
>
> . This should go to master, not emacs-25, as it's a new feature.
Ah, ok, didn't know will move it into master in next revision.
> . Please provide a ChangeLog-style commit log message for the
> changes (some details are in CONTRIBUTE).
Ok, will do, just one question. where do I need add it? As you may
see I added some description to the commit message. Do I need to
do something additional? But anyway, I will look into CONTRIBUTE.
> . Please provide an update for the user manual, which describes
> "C-c C-c", to describe "C-x C-x" as well.
Will do.
> Some specific comments below.
>
> > +(defun image-mode-as-hex ()
> > + "Set a non-image mode as major mode in combination with image minor mode.
> > +A non-mage major mode found from `auto-mode-alist' or fundamental mode
> > +displays an image file as hex. `image-minor-mode' provides the key
> > +\\<image-mode-map>\\[image-toggle-hex-display] to switch back to
> > `image-mode'
> > +to display an image file as the actual image.
> > +
> > +You can use `image-mode-as-text' in `auto-mode-alist' when you wanto
> ^^^^^
> A typo.
>
> > +to display an image file as text initially.
>
> Why does this sentence talk about text, when the mode is about hex
> display? Copy/paste mistake?
Yes, anyway it is very similar. Will fix it.
> > +(defun image-toggle-hex-display ()
> > + "Toggle between image and text display.
> > +If the current buffer is displaying an image file as an image,
> > +call `image-mode-as-hex' to switch to text. Otherwise, display
> > +the image by calling `image-mode'."
>
> It is best to avoid referring to another command in a doc string, if
> you can avoid that. In this case, the reference doesn't add any
> useful information, so I think it should be deleted.
Are you about `image-mode-as-hex`? I've copied it from the image-toggle-display
doc string. Do we need to delete it there too?
> Last, but not least, I think this contribution is too large to accept
> without legal paperwork. I don't see your copyright assignment on
> file; would you like me to send you the form so you could start the
> paperwork rolling?
Yes, sure.
Thank you.