diff --git a/ChangeLog b/ChangeLog index 7ef7eff377..1c52ef0d54 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,23 @@ +1999-12-10 Garry R. Osgood + + * app/undo.c : An inadvertent substitution of UndoTypes LAYER_ADD_UNDO + and LAYER_REMOVE_UNDO in undo_pop_layer_mask() and undo_free_layer_mask() + sanity checks prevented the proper disposal of GimpLayerMasks and + associated tile managers and tiles. Changed to LAYER_MASK_ADD_UNDO + and LAYER_MASK_REMOVE_UNDO to be consistent with the undo_push_layer_mask() + function and to invoke proper cleanup and release of retiring alpha layer masks. + * docs/undo.txt: New file, an overview of undo logic written by + Austin Donnelly so that I could write this change log entry with + panache and flair. ;) + * app/undo.c + * app/undo_types.h + * app/gimpimage.c : Introduced a new UndoType, UNDO_NULL, which maps + to zero, introducing that value into the enumerated type and preventing + strict ANSI compilers from complaining about mixing enumerated and + unenumerated types. Use the type to signal type unknown/error/untyped + conditions. + Full patch documentation at http://idt.net/~gosgood/gimp-patch/patch02.html + 1999-12-08 Michael Natterer * plug-ins/common/jpeg.c: The plugin allocated memory chunks of diff --git a/app/core/gimpimage-guides.c b/app/core/gimpimage-guides.c index 418203a190..b3d26a12a8 100644 --- a/app/core/gimpimage-guides.c +++ b/app/core/gimpimage-guides.c @@ -181,7 +181,7 @@ gimp_image_init (GimpImage *gimage) gimage->undo_bytes = 0; gimage->undo_levels = 0; gimage->group_count = 0; - gimage->pushing_undo_group = 0; + gimage->pushing_undo_group = UNDO_NULL; gimage->comp_preview_valid[0] = FALSE; gimage->comp_preview_valid[1] = FALSE; gimage->comp_preview_valid[2] = FALSE; diff --git a/app/core/gimpimage-merge.c b/app/core/gimpimage-merge.c index 418203a190..b3d26a12a8 100644 --- a/app/core/gimpimage-merge.c +++ b/app/core/gimpimage-merge.c @@ -181,7 +181,7 @@ gimp_image_init (GimpImage *gimage) gimage->undo_bytes = 0; gimage->undo_levels = 0; gimage->group_count = 0; - gimage->pushing_undo_group = 0; + gimage->pushing_undo_group = UNDO_NULL; gimage->comp_preview_valid[0] = FALSE; gimage->comp_preview_valid[1] = FALSE; gimage->comp_preview_valid[2] = FALSE; diff --git a/app/core/gimpimage-projection.c b/app/core/gimpimage-projection.c index 418203a190..b3d26a12a8 100644 --- a/app/core/gimpimage-projection.c +++ b/app/core/gimpimage-projection.c @@ -181,7 +181,7 @@ gimp_image_init (GimpImage *gimage) gimage->undo_bytes = 0; gimage->undo_levels = 0; gimage->group_count = 0; - gimage->pushing_undo_group = 0; + gimage->pushing_undo_group = UNDO_NULL; gimage->comp_preview_valid[0] = FALSE; gimage->comp_preview_valid[1] = FALSE; gimage->comp_preview_valid[2] = FALSE; diff --git a/app/core/gimpimage-resize.c b/app/core/gimpimage-resize.c index 418203a190..b3d26a12a8 100644 --- a/app/core/gimpimage-resize.c +++ b/app/core/gimpimage-resize.c @@ -181,7 +181,7 @@ gimp_image_init (GimpImage *gimage) gimage->undo_bytes = 0; gimage->undo_levels = 0; gimage->group_count = 0; - gimage->pushing_undo_group = 0; + gimage->pushing_undo_group = UNDO_NULL; gimage->comp_preview_valid[0] = FALSE; gimage->comp_preview_valid[1] = FALSE; gimage->comp_preview_valid[2] = FALSE; diff --git a/app/core/gimpimage-scale.c b/app/core/gimpimage-scale.c index 418203a190..b3d26a12a8 100644 --- a/app/core/gimpimage-scale.c +++ b/app/core/gimpimage-scale.c @@ -181,7 +181,7 @@ gimp_image_init (GimpImage *gimage) gimage->undo_bytes = 0; gimage->undo_levels = 0; gimage->group_count = 0; - gimage->pushing_undo_group = 0; + gimage->pushing_undo_group = UNDO_NULL; gimage->comp_preview_valid[0] = FALSE; gimage->comp_preview_valid[1] = FALSE; gimage->comp_preview_valid[2] = FALSE; diff --git a/app/core/gimpimage-undo-push.c b/app/core/gimpimage-undo-push.c index 96872b143b..583c9b0db7 100644 --- a/app/core/gimpimage-undo-push.c +++ b/app/core/gimpimage-undo-push.c @@ -316,7 +316,7 @@ undo_push (GImage *gimage, gimage->dirty = 10000; } - if (!gimage->pushing_undo_group) + if (gimage->pushing_undo_group ==UNDO_NULL ) if (! undo_free_up_space (gimage)) return NULL; @@ -325,7 +325,7 @@ undo_push (GImage *gimage, gimage->undo_bytes += size; /* only increment levels if not in a group */ - if (!gimage->pushing_undo_group) + if (gimage->pushing_undo_group == UNDO_NULL) gimage->undo_levels ++; gimage->undo_stack = g_slist_prepend (gimage->undo_stack, (void *) new); @@ -333,7 +333,7 @@ undo_push (GImage *gimage, /* lastly, tell people about the newly pushed undo (must come after * modification of undo_stack). */ - if (!gimage->pushing_undo_group) + if (gimage->pushing_undo_group == UNDO_NULL) gimp_image_undo_event (gimage, UNDO_PUSHED); return new; @@ -456,7 +456,7 @@ undo_pop (GImage *gimage) * leave unbalanced group start marker earlier in the stack too, * causing much confusion when it's later reached and * mis-interpreted as a group start. */ - g_return_val_if_fail (gimage->pushing_undo_group == 0, FALSE); + g_return_val_if_fail (gimage->pushing_undo_group == UNDO_NULL, FALSE); return pop_stack (gimage, &gimage->undo_stack, &gimage->redo_stack, UNDO); } @@ -466,7 +466,7 @@ int undo_redo (GImage *gimage) { /* ditto for redo stack */ - g_return_val_if_fail (gimage->pushing_undo_group == 0, FALSE); + g_return_val_if_fail (gimage->pushing_undo_group == UNDO_NULL, FALSE); return pop_stack (gimage, &gimage->redo_stack, &gimage->undo_stack, REDO); } @@ -501,7 +501,7 @@ undo_get_topitem_type (GSList *stack) Undo *object; if (!stack) - return 0; + return UNDO_NULL; object = stack->data; @@ -529,11 +529,11 @@ undo_get_undo_name (GImage *gimage) UndoType undo_get_undo_top_type (GImage *gimage) { - g_return_val_if_fail (gimage != NULL, 0); + g_return_val_if_fail (gimage != NULL, UNDO_NULL); /* don't want to encourage undo while a group is open */ - if (gimage->pushing_undo_group != 0) - return 0; + if (gimage->pushing_undo_group != UNDO_NULL) + return UNDO_NULL; return undo_get_topitem_type (gimage->undo_stack); } @@ -545,7 +545,7 @@ undo_get_redo_name (GImage *gimage) g_return_val_if_fail (gimage != NULL, NULL); /* don't want to encourage redo while a group is open */ - if (gimage->pushing_undo_group != 0) + if (gimage->pushing_undo_group != UNDO_NULL) return NULL; return undo_get_topitem_name (gimage->redo_stack); @@ -594,7 +594,7 @@ undo_map_over_undo_stack (GImage *gimage, void *data) { /* shouldn't have group open */ - g_return_if_fail (gimage->pushing_undo_group == 0); + g_return_if_fail (gimage->pushing_undo_group == UNDO_NULL); undo_map_over_stack (gimage->undo_stack, fn, data); } @@ -604,7 +604,7 @@ undo_map_over_redo_stack (GImage *gimage, void *data) { /* shouldn't have group open */ - g_return_if_fail (gimage->pushing_undo_group == 0); + g_return_if_fail (gimage->pushing_undo_group == UNDO_NULL); undo_map_over_stack (gimage->redo_stack, fn, data); } @@ -707,7 +707,7 @@ undo_push_group_end (GImage *gimage) boundary_marker); gimage->undo_bytes += boundary_marker->bytes; - gimage->pushing_undo_group = 0; + gimage->pushing_undo_group = UNDO_NULL; /* Do it here, since undo_push doesn't emit this event while in the * middle of a group */ @@ -1637,8 +1637,8 @@ undo_pop_layer_mask (GImage *gimage, lmu = (LayerMaskUndo *) lmu_ptr; /* remove layer mask */ - if ((state == UNDO && type == LAYER_ADD_UNDO) || - (state == REDO && type == LAYER_REMOVE_UNDO)) + if ((state == UNDO && type == LAYER_MASK_ADD_UNDO) || + (state == REDO && type == LAYER_MASK_REMOVE_UNDO)) { /* remove the layer mask */ lmu->layer->mask = NULL; @@ -1690,8 +1690,8 @@ undo_free_layer_mask (UndoState state, * stack and it's a layer add, or if we're freeing from * the undo stack and it's a layer remove */ - if ((state == REDO && type == LAYER_ADD_UNDO) || - (state == UNDO && type == LAYER_REMOVE_UNDO)) + if ((state == REDO && type == LAYER_MASK_ADD_UNDO) || + (state == UNDO && type == LAYER_MASK_REMOVE_UNDO)) layer_mask_delete (lmu->mask); g_free (lmu); @@ -2852,7 +2852,7 @@ static struct undo_name_t { UndoType type; const char *name; } undo_name[] = { - {0, N_("<>")}, + {UNDO_NULL, N_("<>")}, {IMAGE_UNDO, N_("image")}, {IMAGE_MOD_UNDO, N_("image mod")}, {MASK_UNDO, N_("mask")}, diff --git a/app/core/gimpimage.c b/app/core/gimpimage.c index 418203a190..b3d26a12a8 100644 --- a/app/core/gimpimage.c +++ b/app/core/gimpimage.c @@ -181,7 +181,7 @@ gimp_image_init (GimpImage *gimage) gimage->undo_bytes = 0; gimage->undo_levels = 0; gimage->group_count = 0; - gimage->pushing_undo_group = 0; + gimage->pushing_undo_group = UNDO_NULL; gimage->comp_preview_valid[0] = FALSE; gimage->comp_preview_valid[1] = FALSE; gimage->comp_preview_valid[2] = FALSE; diff --git a/app/core/gimpprojection-construct.c b/app/core/gimpprojection-construct.c index 418203a190..b3d26a12a8 100644 --- a/app/core/gimpprojection-construct.c +++ b/app/core/gimpprojection-construct.c @@ -181,7 +181,7 @@ gimp_image_init (GimpImage *gimage) gimage->undo_bytes = 0; gimage->undo_levels = 0; gimage->group_count = 0; - gimage->pushing_undo_group = 0; + gimage->pushing_undo_group = UNDO_NULL; gimage->comp_preview_valid[0] = FALSE; gimage->comp_preview_valid[1] = FALSE; gimage->comp_preview_valid[2] = FALSE; diff --git a/app/gimpimage.c b/app/gimpimage.c index 418203a190..b3d26a12a8 100644 --- a/app/gimpimage.c +++ b/app/gimpimage.c @@ -181,7 +181,7 @@ gimp_image_init (GimpImage *gimage) gimage->undo_bytes = 0; gimage->undo_levels = 0; gimage->group_count = 0; - gimage->pushing_undo_group = 0; + gimage->pushing_undo_group = UNDO_NULL; gimage->comp_preview_valid[0] = FALSE; gimage->comp_preview_valid[1] = FALSE; gimage->comp_preview_valid[2] = FALSE; diff --git a/app/undo.c b/app/undo.c index 96872b143b..583c9b0db7 100644 --- a/app/undo.c +++ b/app/undo.c @@ -316,7 +316,7 @@ undo_push (GImage *gimage, gimage->dirty = 10000; } - if (!gimage->pushing_undo_group) + if (gimage->pushing_undo_group ==UNDO_NULL ) if (! undo_free_up_space (gimage)) return NULL; @@ -325,7 +325,7 @@ undo_push (GImage *gimage, gimage->undo_bytes += size; /* only increment levels if not in a group */ - if (!gimage->pushing_undo_group) + if (gimage->pushing_undo_group == UNDO_NULL) gimage->undo_levels ++; gimage->undo_stack = g_slist_prepend (gimage->undo_stack, (void *) new); @@ -333,7 +333,7 @@ undo_push (GImage *gimage, /* lastly, tell people about the newly pushed undo (must come after * modification of undo_stack). */ - if (!gimage->pushing_undo_group) + if (gimage->pushing_undo_group == UNDO_NULL) gimp_image_undo_event (gimage, UNDO_PUSHED); return new; @@ -456,7 +456,7 @@ undo_pop (GImage *gimage) * leave unbalanced group start marker earlier in the stack too, * causing much confusion when it's later reached and * mis-interpreted as a group start. */ - g_return_val_if_fail (gimage->pushing_undo_group == 0, FALSE); + g_return_val_if_fail (gimage->pushing_undo_group == UNDO_NULL, FALSE); return pop_stack (gimage, &gimage->undo_stack, &gimage->redo_stack, UNDO); } @@ -466,7 +466,7 @@ int undo_redo (GImage *gimage) { /* ditto for redo stack */ - g_return_val_if_fail (gimage->pushing_undo_group == 0, FALSE); + g_return_val_if_fail (gimage->pushing_undo_group == UNDO_NULL, FALSE); return pop_stack (gimage, &gimage->redo_stack, &gimage->undo_stack, REDO); } @@ -501,7 +501,7 @@ undo_get_topitem_type (GSList *stack) Undo *object; if (!stack) - return 0; + return UNDO_NULL; object = stack->data; @@ -529,11 +529,11 @@ undo_get_undo_name (GImage *gimage) UndoType undo_get_undo_top_type (GImage *gimage) { - g_return_val_if_fail (gimage != NULL, 0); + g_return_val_if_fail (gimage != NULL, UNDO_NULL); /* don't want to encourage undo while a group is open */ - if (gimage->pushing_undo_group != 0) - return 0; + if (gimage->pushing_undo_group != UNDO_NULL) + return UNDO_NULL; return undo_get_topitem_type (gimage->undo_stack); } @@ -545,7 +545,7 @@ undo_get_redo_name (GImage *gimage) g_return_val_if_fail (gimage != NULL, NULL); /* don't want to encourage redo while a group is open */ - if (gimage->pushing_undo_group != 0) + if (gimage->pushing_undo_group != UNDO_NULL) return NULL; return undo_get_topitem_name (gimage->redo_stack); @@ -594,7 +594,7 @@ undo_map_over_undo_stack (GImage *gimage, void *data) { /* shouldn't have group open */ - g_return_if_fail (gimage->pushing_undo_group == 0); + g_return_if_fail (gimage->pushing_undo_group == UNDO_NULL); undo_map_over_stack (gimage->undo_stack, fn, data); } @@ -604,7 +604,7 @@ undo_map_over_redo_stack (GImage *gimage, void *data) { /* shouldn't have group open */ - g_return_if_fail (gimage->pushing_undo_group == 0); + g_return_if_fail (gimage->pushing_undo_group == UNDO_NULL); undo_map_over_stack (gimage->redo_stack, fn, data); } @@ -707,7 +707,7 @@ undo_push_group_end (GImage *gimage) boundary_marker); gimage->undo_bytes += boundary_marker->bytes; - gimage->pushing_undo_group = 0; + gimage->pushing_undo_group = UNDO_NULL; /* Do it here, since undo_push doesn't emit this event while in the * middle of a group */ @@ -1637,8 +1637,8 @@ undo_pop_layer_mask (GImage *gimage, lmu = (LayerMaskUndo *) lmu_ptr; /* remove layer mask */ - if ((state == UNDO && type == LAYER_ADD_UNDO) || - (state == REDO && type == LAYER_REMOVE_UNDO)) + if ((state == UNDO && type == LAYER_MASK_ADD_UNDO) || + (state == REDO && type == LAYER_MASK_REMOVE_UNDO)) { /* remove the layer mask */ lmu->layer->mask = NULL; @@ -1690,8 +1690,8 @@ undo_free_layer_mask (UndoState state, * stack and it's a layer add, or if we're freeing from * the undo stack and it's a layer remove */ - if ((state == REDO && type == LAYER_ADD_UNDO) || - (state == UNDO && type == LAYER_REMOVE_UNDO)) + if ((state == REDO && type == LAYER_MASK_ADD_UNDO) || + (state == UNDO && type == LAYER_MASK_REMOVE_UNDO)) layer_mask_delete (lmu->mask); g_free (lmu); @@ -2852,7 +2852,7 @@ static struct undo_name_t { UndoType type; const char *name; } undo_name[] = { - {0, N_("<>")}, + {UNDO_NULL, N_("<>")}, {IMAGE_UNDO, N_("image")}, {IMAGE_MOD_UNDO, N_("image mod")}, {MASK_UNDO, N_("mask")}, diff --git a/app/undo_types.h b/app/undo_types.h index dba9381a90..10a2678385 100644 --- a/app/undo_types.h +++ b/app/undo_types.h @@ -26,10 +26,11 @@ * the bottom of undo.c as well. */ typedef enum { - /* Type 0 is special - in the gimpimage structure it means + /* Type UNDO_NULL (0) is special - in the gimpimage structure it means * there is no undo group currently being added to. */ - IMAGE_UNDO = 1, + UNDO_NULL = 0, /* Picky compilers demand this in the enumeration - gosgood@idt.net */ + IMAGE_UNDO, IMAGE_MOD_UNDO, MASK_UNDO, LAYER_DISPLACE_UNDO, diff --git a/docs/undo.txt b/docs/undo.txt new file mode 100644 index 0000000000..03457a35ba --- /dev/null +++ b/docs/undo.txt @@ -0,0 +1,73 @@ +A quick overview of the undo system +----------------------------------- + +Actions on the image by the user are pushed onto an undo stack. Each +action object includes all the information needed to undo or redo an +operation, plus an UndoType. The type can be converted to text to +show to the user. Actions may be run forwards (UndoState == REDO) or +backwards (UndoState == UNDO). As the action is run, it swaps the +image's current state and the recorded state. A run action is moved +from the undo stack to the redo stack (or vice-versa if UndoState == +REDO). Pushing something onto the undo stack causes the redo stack to +be cleared, since the actions on the redo stack may depend on the +image being in a particular state (eg consider: layer add, rename, +undo rename, layer delete. If the redo stack weren't cleared on undo, +then there would still be a "rename" operation on the redo stack which +could be run on a non-existent layer. Bad news.) + +Undo groups +----------- +In order to group many basic operations together into a more useful +whole, code can push group start and end markers. A group is treated +as a single action for the purposes of the undo and redo user +commands. It is legal to nest groups, in which case the outermost +group is the only user-visible one. + +Groups boundaries used to be implemented by pushing a NULL pointer on +the undo (or redo) stack. Now they are a special action which has the +"group_boundary" bit set. This allows the group boundaries to include +the undo type associated with the whole group. The individual actions +need to preserve their own undo type since the undo_free_* functions +sometimes need to know which action is being freed. + +Undo events +----------- +Images emit UNDO_EVENT signals, to say that the user has performed an +undo or redo action on that image. This allows interested parties to +track image mutation actions. So far, only the undo history dialog +uses this feature. The other way to discover the undo status of an +image is to use the iterator functions undo_map_over_undo_stack() and +undo_map_over_redo_stack(). These call your function on each action +(or group) on the stack. There is also undo_get_undo_name() and +undo_get_redo_name() to peek at the top items on each stack. This +could be used (eg) to change the undo/redo menu strings to something +more meaningful, but currently lack synchronisation. + +Dirtying images +--------------- +NOTE about the gimage->dirty counter: + If 0, then the image is clean (ie, copy on disk is the same as the one + in memory). + If positive, then that's the number of dirtying operations done + on the image since the last save. + If negative, then user has hit undo and gone back in time prior + to the saved copy. Hitting redo will eventually come back to + the saved copy. + The image is dirty (ie, needs saving) if counter is non-zero. + If the counter is around 10000, this is due to undo-ing back + before a saved version, then mutating the image (thus destroying + the redo stack). Once this has happened, it's impossible to get + the image back to the state on disk, since the redo info has been + freed. See undo.c for the gorey details. + +NEVER CALL gimp_image_dirty() directly! + +If your code has just dirtied the image, push an undo instead. +Failing that, push the trivial undo which tells the user the +command is not undoable: undo_push_cantundo() (But really, it would +be best to push a proper undo). If you just dirty the image +without pushing an undo then the dirty count is increased, but +popping that many undo actions won't lead to a clean image. + +Austin +