marking this as enhancement request.

i have made the necessary changes and will add a patch as soon as i resolve this
situation with the palette-editor not setting the index appropriately.

Created attachment 30744 [details] [review]
testing patch

this includes actions to go to the next and prev palette entry, and actions to 

find the appropriate index given the current color. also makes the palette
editor set the foreground/background index when you click a color.

the changes made to the foreground/background index by the palette editor and
index-finder are ignored. i suspect that there is something more to be done
regarding adding the FOREGROUND_INDEX and BACKGROUND_INDEX to the GimpContext
properties.
does anyone know what that is?

That is a lot more changes than are necessary to implement the functionality
that this bug report asks for. I don't think that adding color indices to the
GimpContext makes any sense and would rather not see this happening.

index must be tracked because a palette can contain the same color multiple
times and therefore a search by color doesn't work. 
where do you suggest it be tracked then?

In the palette editor. We are talking about actions to modify the state of the
palette editor here, aren't we?

yes.
how would i make it accessible globally, though? (it needs to be accessible from
the image)

How could you not make it accessible? There's a single UI manager in GIMP 2.1.
All actions are accessible from everywhere. I don't think it makes sense to
assign keyboard shortcuts for this by default, but we should make it possible to
do that.

i had those actions added to the palette editor menu before, and i could not
assign shortcuts to them. this appears to be true for palette-editor,
gradient-editor.. those actions do not show up in the shortcut editor at all.

or is this because there is no 'paletteeditor' group? that doesn't seem right,
because it shows up in menurc (but changing its shortcut by editing menurc and
uncommenting the relevant menurc line has no effect)

to be precise, these actions don't appear in the shortcut editor:
colormap_editor/*
documents/* (unfortunate because i want to use ctrl+number for other things than
recent document loading.)
gradient-editor/*
gradients/*
palette-editor/*

(some others don't (eg gimpdata displays such as brush), but these are the ones
it might be useful to be able to assign shortcuts to.)

anyway i'll add it to the palette editor, then `how to make it shortcutable` can
be tackled.

i verified that these actions are inaccessible and treated as if they don't
exist when i assign them to a controller.
a beta-quality patch implementing fg/bg index changing actions for palette
editor is attached.
how do i make the actions visible (and why aren't they already?).
i can add them to the palette editor menu and thus verify their workingness, but
not use them elsewhere.

Created attachment 30811 [details] [review]
testing patch #2

Moving on the target milestone 2.2 so that the patch is at least considered for
inclusion.

i'll make another patch (still some stuff needs cleaning up).
after i set the accel to NULL rather than "", it still won't let me
set an accel on those items. i am reminded of a changelog entry awhile ago
regarding disabling accelerators belonging to dockables (for example, CTRL-S in
the gradient editor doesn't work despite being shown as the accel for two(?) of
the menu entries).

so, is a way to make selected dockable accels globally accessible what is needed
for that?

wait-- what was i thinking!?!
of course it SHOULDN'T be actions to alter the state of the palette editor --
the palette belongs to the context, and of course last FG/BGcolor from palette
should be remembered separately for each device, for exactly the same reason as
FG/BGcolor is.
so what would be required is:
A) add the properties to the Context structure
B) make a palette viewing widget which shows the current palette and marks
current fg/bg colors, to be used in displaying the device status. Allow this to
be clicked on so Finally it would be possible to select colors without using an
'editor'. 

so i'll do that.

secondarily there is the issue of making the palette editor more sane (eg
display should use square cells, some basic editing facilities such as blend
range and swap range). obviously this belongs in a separate bug though. i need
to dig up my old palette editor ond see what conventions were in use there, and
do some research on eg. Deluxe Paint IIe, PCPaint, see if they have any useful
conventions in addition to mine. 

(A) I don't think that palette indices belong to the context. A palette is a way
to select a color. All that the context needs to know about is the selected
color. It shouldn't know anything about a color index.

(B) What's the problem about using the palette editor to select colors? Yet
another palette widget will certainly not improve usability.

I vote for closing as WONTFIX.

A) Yes, okay. 

B) You have to have the palette editor up to select colors from the palette (and
the amount of screen space is limited -- i would prefer something with 4x4 cells
as a selection widget). anyway i'm just trying to justify this immensely useful
feature of next/prev color. but see below.

you have not explained how the action would be accessible if it belonged to the
palette editor, since currently no dockable action that is not explicitly
associated with the image can be used  via shortcut(either in the dockable
itself or on the image.) What you've already said does not render these accessible. 

do you suggest i finish off a patch that implements this in the palette editor,
and privately use something else that can be accessed while waiting for GTK to
be fixed so dockables' actions can be used elsewhere than the dockable?
that seems the only thing that will satisfy both of our ideas.

This shouldn't block the 2.2 release.

Comment on attachment 30811 [details] [review]
testing patch #2


From Sven's comments, obviously needs work before it will be accepted.

Mitch made it clear that you cannot assign a shortcut to a menu item belonging
to a dockable unless you explicitly add that accel group.
I am baffled how to implement this in a way you will deem acceptable.
here is the problem:

a) I want to be able to access these actions at any time. If i had to choose
between cut and paste and this, i would choose this, that is how useful it is --
the palette editor is laughable by comparison.
b) Placing these actions in the <palette-editor> menu group would make sense, BUT:
c) the actions cannot be accessed unless they are explicitly added to the images
accels. In the source I've only noticed adding entire accel groups. Can this be
done on an individual basis just for these two items?

so. Is c) possible and suitable as a solution? 

Okay, I have implemented most of this in the following way:

* Functions in widgets/gimppaletteeditor.[ch] for going to the next or previous  color or an arbitrary palette index, and for getting the currently selected color.
* Commands in actions/context-commands.[ch] for setting FGcolor to next or previous color in palette
* Actions in actions/context-actions.c using them
* A utility function in gimppalette.c for finding which index in a palette (if any) holds a given color.

Design-wise does that sound OK? If so I will proceed with the following list to finish it off.

What I haven't done yet:
* Enabled the context-(next|prev)-skip actions
* Added BG versions of the actions
* Finalized the action names (they're long currently - 'context-colors-prev-fg-in-palette')
* Coded a similar interface for indexed colormaps (which might be even more useful than this!)
* Coded color-matching (so if the current FG matches a color in the palette, but the currently selected palette entry is not that color, interpret the next/prev command as relative to the matching color in the palette)
* Coded a PDB interface to gimp_palette_color_index()
* Assigned a proper stock ID or help ID (What's involved in adding a new help ID or icon?)

A patch follows, if you want to try it in its current state (useful but not yet optimal). I'm aware it needs some cleanup (coding style and action description consistency).

Created attachment 60097 [details] [review]
all-new patch (WIP but quite wonderfully usable)

Possibly this should be marked 'needs-work' -- I'm not sure if that is needed when the patch author acknowledges it when submitting.

I would like to make this autodetect when the colormap editor is active (ie. an indexed image is being edited), and use the colormap instead of palette for these next/prev changes. That would be natural and helpful for indexed drawing.
I think I can manage to do that, too. Anything I should consider here?

This is fantastic! I am really excited about this feature. keyboard navigation by palette index will vastly increase the gimp's utility for pixelation art.

Created attachment 60141 [details]
20k tar.gz of latest version 

I only have anoncvs access so I was unable to add files (thus can't make diffs that add files).
I am aware that one part of this implementation is slightly hackish (#including a ".ch" file) -- i figure to refactor this when I'm done.

This implements:  (not actual action names, just mnemonic abbrevs.)
fg-next-in-pal
fg-prev-in-pal
fg-next-in-pal-skip (skips forward by a constant, currently == 4 entries)
fg-prev-in-pal-skip
fg-min-in-pal  (goto first palette entry)
fg-max-in-pal  (goto last palette entry)

(bg-* are not implemented yet)

the actual action description of fg-(min|max)-in-pal could be better;
I considered (initial,final), (first, last), (start, end) (0, max).
0, max is the only one I consider a serious alternative; the rest are vague or easily confused with something else. Any other suggestions?

The behaviour I mentioned in comment #24 is implemented, and works quite well in conjunction with the ability to automatically find the appropriate color in the palette or colormap when you eyedrop. After eyedropping, when you use one of the 
actions I've implemented, it automatically interprets the action as relative to that color in the palette. (it's hard to verbally describe, and very helpful.)

What I haven't done yet:
 * BG-related actions
 * Put the palette actions in their own group (group? -- what I mean is like the way all the brush actions are grouped.)
 * Define a stock id and help id.
 * PDB func to look up a color index from a source color.
 * Refactoring
 * Separate those files that can be patched and those that are completely new into two files, a diff and a tar.gz

Hm, that .tgz contains the complete source of the changed
files, not the patch. That makes it hard to review. Why don't
you just attach the patch and the new files separately?

Created attachment 60155 [details] [review]
diff (changed files)

Okay.

Created attachment 60156 [details]
tar.gz (new files)

In regards to comment #26, make a second copy of the source you get from anoncvs then use diff -uN when making your patch so it includes both changed and new files. If you are still unsure about this, ask on the mailing list or #gimp (IRC).

Created attachment 60186 [details] [review]
single diff

Like this? (applyable with 'patch -p1')
-- this is still missing the new icon from the .tgz.

Also, the code doesn't need to be reviewed yet; more the behaviour of the user-visible actions. The actual backend changed a lot(again), and I'm just cleaning up a lot of stuff; It's getting quite neat now, so the next patch
(to come later today) will probably be pretty much ready to commit.

Created attachment 60294 [details] [review]
Cleaned up patch

Ignore the earlier tar.gz; no new files are needed.
This diff should be ready to commit.

Created attachment 60369 [details] [review]
patch with obscure bug fixed.

In case it wasn't clear, both this and the previous patch are cleaned up, use no tabs, conform to the style guidelines outlined in HACKING. and are much more streamlined than the previous patches.


Summary of changes vs CVS HEAD:

 * Add a function to change the currently selected entry via an offset relative to the current selected entry, to gimpcolormapeditor.[ch] and gimppaletteeditor.[ch]. They optionally begin movement relative to the position of the passed color and store the color in the entry that they end up at.

 * Add a function to gimppaletteeditor.[ch] and gimpcolormapeditor.[ch] to get the number of entries in the palette/colormap.

 * Add two action callbacks to context-commands.[ch] and 14 actions to context-actions.c: 'context-(fore|back)ground-palette-color-(set|minimum|maximum|increase|decrease|increase-skip|decrease-skip)'
   the description string looks like
  "Foreground Palette color Skip Back" and "Foreground Palette color Previous"

 * In userland: 
  * The actions are effective when there is a palette editor up (if you want to move through the colormap of an indexed image, you'll need a colormap editor up as well/instead.)
  * Next/Prev color moves to the next or previous color in palette (or colormap, if the active image has one). It tries to match the current FG or BG color to one in the palette editor/colormap editor, so that eyedropping works naturally.
    If there is no such color, it defaults to moving relative to color #0.
  * Skip forward/back work like next/prev, except they move by steps of 4 colors instead of 1 at a time.
  * First/last do what you'd expect.
  * Set : I don't know whether this works right, as I have no suitable controller to test it with. I gather that it should set the active entry from an relative analog input, so that's what I coded it to do.

This updated patch adds a fix for an obscure bug (crashing when there is a palette editor up but the palette is empty), and simplifies slightly.

Some comments:

- the actions are named "context-foreground-palette-foo", which looks
  as if they were functions of the foreground color. However, they
  are rather functions of the palette, so "context-palette-foreground-foo"
  would be appropriate.
- they are also called -minimum, -maximum, -increase etc, however they
  operate in discrete steps, just as previous/next brush, so i think
  they should rather use -set, -first, -last, -previous and -next
- i don't think that we need the skip actions, since these are meant to
  mimick the behavior of an adjustment, which doesn't apply here for the
  reason above.
- i think it's questionable to work on both the colormap and palette
  editors, however both should be controllable. i'm not sure what's
  the best solution here.
- comparing colors should use "gimp_rgb_distance (a, b) < epsilon",
  since comparing floating point values directly is not reliable.
- making the utility functions inline is imho overkill, no need to optimize
  code that is executed once for each invoked action.
- there are still many wrong spaces around (), gimp coding style is
  foo_bar (gint blah), not foo_bar( gint blah ) or foo_bar(gint blah)

Thanks for taking the time to review this.

1. Fixed
2. Fixed
3. Yes, skip actions are VERY useful -- more than first or last, which are the real imitations (haha). I put skip-* in because I have used such functions in another program and know they are super useful; for example, lay down rough shading using the skip fwd/back actions, then detail them and smooth them using the next/prev actions. 

4. I did it this way because separating them is, pragmatically, broken: having to assign different shortcuts to two actions that do essentially the same thing. When you have an indexed image loaded it tends to be unuseful to select colors not actually in the image -- your main operation is likely to involve colors that are in the image; hence it gets preference. When it's not active, palette is the next best. I optimized for minimum mental context-switching ('okay, this is indexed, so i have to use these keys instead of those to navigate the colors' == bad)

5. Fixed. That originally stemmed from it being slow.. I discovered that the 'scales' color selector was causing it to slow down MAJORLY (performance is fine with any other color selector). That may warrant investigation (it's just doing a few calculations and a linear interpolation per scale, right?).

6. Fixed. HACKING could do with a more visual example like yours, instead of "   * There's a single space between the function name and the opening                               bracket."


Updated patch follows.


One remaining question: Should first/last in palette be kept? I say yes, for symmetry.


For the future:

Can colormap editor and palette editor be unified? 
For a separate bug, obviously -- could be implemented after this is committed, cause it's not trivial to do. Essentially it would look like the palette editor, except:
    * Include an index spinner, like ColormapEditor. Maybe it could replace the color name field when editing a colormap.
    * When an indexed image is active, set the 'palette name' to '[FILENAME] colormap', desensitize it, and desensitize the color name.
    * Adding colors at anywhere not the end would cause remapping of the indices in the image using a 256-byte LUT.
    * Removing colors would require actual color-matching.. not sure how that would be done. 
    * How would load/save colormap work? Hmm.
    * Quantize could be added (and work the same way for both colormap and palette editing -- colormap editing would just additionally remap the image colors.)

Created attachment 60643 [details] [review]
Latest (final?) patch

* All style issues fixed
* Compare colors using gimp_rgb_distance(a,b) < EPSILON
* Actions are renamed appropriately
* Gratuitous inlining removed.

The closest i can see to a solution that will satisfy both Mitch and myself, is to make three sets of actions, one operating only on palette editor, one only on colormap editor, and one on both (trying colormap editor first, and palette editor if that fails.)

Is that an acceptable solution, Mitch?

Raising priority to Urgent because this would be nice to have in 2.4 if the solution is acceptable.  If the solution still needs work, this should be bumped to Future and the priority lowered.

I can do the following:

 * Write a Python-Fu plugin that copies the image colormap into a predetermined palette and makes that palette active. (assists working with indexed images, see below)
 * Disable the indexed colormap support so that the actions only operate on the current palette.

Then when we work out a good scheme for implementing full support for indexed colormaps, colormap support can be reenabled and tweaked.

Does that sound good? I'll do it ASAP.

Created attachment 65986 [details] [review]
Latest patch against CVS HEAD, with colormap support disabled (not removed)

I've also written the plugin (which eg. from an image 'meia.gif' makes or remakes a palette named 'meia.gif')
. It needs some tweaks and then I'll attach it here also.

Created attachment 65987 [details]
implements plug_in_palette_from_colormap

The help string reads as follows:
"
Convert the colormap of the image to a palette named after the image. 
If such a palette exists, its contents are overwritten.
"

The plugin is registered at '<Image>/Colors/Copy colormap to palette', which is the best place I could find for it.

Sorry for not coming back earlier... :)

Actually, I like your idea from comment #38, because it offers the
greatest flexibility (and the additional actions don't hurt, because
they don't clutter any gui).

Some comments:

- The word "color" in action and function names is redundant
  (just like it's gimp_context_set_foreground(), and not
   gimp_context_set_foreground_color())
- The searching of a matching color in the palette is IMHO
  more surprising than useful.
- Instead of adding the colormap-to-palette script, we should
  export the functions from app/core/gimppalette-import to the PDB.
- Functions without arguments are declared (void), not just ().

Okay. Then we need to find a suitable name encompassing colormaps and palettes both('swatches'?)

1. Ok, I'll fix that.
2. Searching of a matching color in the palette is *very* important: it lets you eyedrop correctly regardless of whether the image is RGB or INDEXED. Try disabling it* and you will see, without it it gets very confusing when you eyedrop. pick up a light blue, go 'next color' and get an orange because that's where the cursor was sitting last.
3,4. Ok

* gimp_palette_editor_go() : if you comment out the first scope belonging to 'if (color)', that should do it.

Created attachment 66322 [details] [review]
newest patch with colormap, palette, and swatch support

I will be working on the gimppalette-import PDB interface separately, the plugin was just to work around the absence of colormap manipulation.
Since this new patch supports them all, there is no need for the plugin now.

for the autodetecting between colormap/palette actions I used the name 'swatch' as in context-swatch-foreground-last

Created attachment 66324 [details] [review]
with shortcut fix

The previous version had used 9/0 for previous/next of swatch, palette, and colormap. this version only has swatch-previous and swatch-next shortcutted.

At this point, the enhancements discussed in this bug are stable.

I'm considering coding optimized color matching for palettes, so that if the same color is in the currently edited palette when you (change FG/BG color in some way), it will be selected. (this might address your objection to the searching behaviour Mitch, and should make it work perfectly naturally)
That feature could be ready before the next release, it depends on when the next release is likely to be.

In any case, I feel that this patch is ready for committing. The color matching should probably be made a separate bug since it warrants discussion. (I'll add that bug/enhancement report once this one is resolved -- it would depend on some of the supporting functionality of this patch.)

Fixed in CVS. Modified the patch to get rid of the voodoo go() functions,
and made the action callback code look more like the one of similar
actions. I could have written an lengthy comment asking for such changes,
but after all these iterations i finally wanted in in CVS, i hope that's ok.

2006-05-28  Michael Natterer  <mitch@gimp.org>

	Applied patch from David Gowers which adds actions to select
	palette and colormap colors with actions. Modified the patch quite
	a bit. Fixes bug #130123.

	* app/widgets/gimpcolormapeditor.[ch]
	* app/widgets/gimppaletteeditor.[ch]: add functions get_index()
	which gets the currently selected color's index (optionally the
	index of a passed color), set_index() which sets the selected
	color by index, and max_index() which returns the maximum possible
	color index.

	* app/dialogs/dialogs-constructors.c: changed accordingly.

	* app/actions/context-actions.c
	* app/actions/context-commands.[ch]: actions and callbacks which
	use the new functions.

That's good. I used go() instead of 3 separate functions mainly because of speed considerations. It seems to get an acceptable speed anyway.