GNOME Bugzilla – Bug 150030
Reordering colour palettes
Last modified: 2005-01-03 16:29:53 UTC
I'd like to propose a small new small new feature for the GIMP. I sometimes need a way to reorder the colours in a palettized image. The GIMP does not seem to offer a way to do that. In older verions (1.2) you could somewhat work around this because the dominant colours seemed to be sorted first (convert to RGB, enlarge the canvas, add some paint, convert to indexed, clip canvas) but this no longer works with 2.x. The only way right now is to create a new palette and drag all colours into it in the desired order. This can get pretty tedious pretty quickly. So I'd like to see a way to manually shuffle colours around in a palette, preferably just by dragging them around. Any chance to get something like this added? Thanks.
As usual a patch is what will increase the likelihood of such an enhancement.
Created attachment 32680 [details] [review] Allows colours to be inserted into a palette by dragging This patch allows colours to be inserted (via dragging) at a particular place in the palette. The bulk of the patch extends the gimp drag-and-drop system to pass the position of the drop down to the drop destination. At the moment, only color-drop callbacks are passed the x,y values, but there's no reason the callbacks for other data types could not also be passed that data. The rest of the patch: - adds an insert function to gimppalette - alters the palette editor to work out which slot the colour is being dropped on (and inserts it there). TODO: This patch doesn't make any distinction between colours dragged within the palette editor and colours dragged in from elsewhere. For the former case, the source colour should be deleted as the new one is inserted. But I don't know yet how to distinguish the source of the drag... ideas welcome. This is my first attempt at hacking on the Gimp, so comments and advice welcome!
Very nicely done. The patch doesn't follow the coding style however. After it is cleaned up the patch should IMHO be applied after 2.2.
Cool! OK - I see a few tabs snuck into the patch (doh), and there are probably a few places where I broke lines in the wrong place and some of the function parameter defs could be tidied up... what else did I miss?
The thing that hurts most is that it should not be: void gimp_display_shell_drop_color (GtkWidget *widget, gint x, gint y, const GimpRGB *color, gpointer data) but instead: void gimp_display_shell_drop_color (GtkWidget *widget, gint x, gint y, const GimpRGB *color, gpointer data) Then we have spaces after each comma, use spaces instead of tabs, etc. Stuff like: + { + /* nope - add it on the end */ + editor->color = gimp_palette_add_entry ( + palette, + NULL, + (GimpRGB *) color); + } Is also eeky. Just look at the surrounding code, you mentioned the places that need to be cleaed up yourself :) Most patches are cleaned up anyway when applying them, but your patch changes so many files that the bulk of them shouldn't need manual hacking after applying.
Created attachment 32705 [details] [review] Cleaned-up version of previous patch This patch should be a little cleaner than the previous one.
Thanks. That patch looks more or less fine. Don't worry about the remaining "uncleanness", we are coding style fanatics who clean up each patch upon applying anyway :-) Raising priority so it will be applied immediately after 2.2 to avoid bit-rot from hanging around in bugzilla.
Comment on attachment 32705 [details] [review] Cleaned-up version of previous patch Setting patch as accepted and in the queue.
The patch needs cleanup, it isn't accepted as such.
Fixed in CVS: 2004-12-31 Michael Natterer <mitch@gimp.org> Applied modified patch from Ben Campbell which adds drop coordinates to the color drop callback and uses it to insert colors in the palette editor. Extended the patch to add drop coordinates to all drop callbacks. * app/core/gimppalette.[ch]: added gimp_palette_insert_entry(). * app/display/gimpdisplayshell-dnd.[ch]: added drop coordinates to all drop callbacks. * app/dialogs/palette-import-dialog.c * app/widgets/gimpcolormapeditor.c * app/widgets/gimpcontainerview.c * app/widgets/gimpdnd.[ch] * app/widgets/gimpdrawabletreeview.c * app/widgets/gimpfgbgeditor.c * app/widgets/gimpgradienteditor.c * app/widgets/gimpitemtreeview.c * app/widgets/gimppaletteeditor.c * app/widgets/gimppropwidgets.c * app/widgets/gimpselectioneditor.c * app/widgets/gimptoolbox-dnd.c * app/widgets/gimptoolbox-image-area.c * app/widgets/gimptoolbox-indicator-area.c * app/widgets/gimptooloptionseditor.c * libgimpwidgets/gimpcolorselect.c: changed accordingly. The passed drop coordiantes are so far unused. * app/widgets/gimppaletteeditor.c: use the drop coordinates to insert the new color into the palette at the right place instead of always appending. Fixes bug #150030.
I must confess I haven't actually tried the patch yet, but from the discussion and the patch it seems to me like only half of the bug was fixed. You can now insert colors at arbitrary positions, but you still cannot swap colors.
You have created some confusion by mixing up two concepts: (1) a "palette" is a set of colors that can be used in RGB images; (2) a "colormap" or "indexed palette" is the set of colors used in an indexed image. To alter the colors in the colormap of an indexed image, you use the Colormap dialog. Clicking on a color entry and then doing "edit color" allows you to change it by bringing up a color editor. This color editor has several "storage slots", which you can use as temporary storage for swapping colors. This is a bit complicated, but swapping colors in an indexed image is a very unusual thing to do, so it isn't clear that it needs to be made easier.