From 48c7c036ad9e56e4caafd568f4f0433977f3a237 Mon Sep 17 00:00:00 2001 From: David Beswick Date: Thu, 8 Feb 2024 20:32:40 +1100 Subject: [PATCH] Fix my regression (b7d25b0) in sequence loop end/play end handling. Removed "len-patterns" property setter. This property may not agree to set itself to a value that would cause pattern truncation, so the value supplied to g_object_set may not match g_object_get. Not only does this confuse the automated tests (check_readwrite_long_param) but sequence data size management should ideally be a hidden implementation detail. --- src/lib/core/sequence.c | 85 +++++++++++++++++++------------- src/lib/core/sequence.h | 4 ++ src/ui/edit/main-page-sequence.c | 6 +-- tests/lib/core/t-sequence.c | 4 +- 4 files changed, 60 insertions(+), 39 deletions(-) diff --git a/src/lib/core/sequence.c b/src/lib/core/sequence.c index 23dd4d086..0adde40dd 100644 --- a/src/lib/core/sequence.c +++ b/src/lib/core/sequence.c @@ -369,6 +369,45 @@ bt_sequence_get_nonnull_length (const BtSequence * const self) { return 0; } +/* + * bt_sequence_update_post_length_change: + * @length: the new value requested for length + * + * Handles making sure that loop-end doesn't extend past song length, and also + * makes sure that loop-end is set to the song length, then the loop-end is + * updated to match the new length. This is to prevent annoying song authors + * when their song length changes, so the previous loop-end doesn't hang around + * floating somewhere in the middle of the song. + * + * Also sets play-end appropriately when length is to change. + * + * Should be called after self->length changes. + */ +static void +bt_sequence_post_length_change (const BtSequence * const self, + guint old_length) +{ + if (self->priv->loop_end != -1) { + // clip loopend to length or extend loop-end as well if loop_end was + // old length + if ((self->priv->loop_end > self->priv->length) || + (self->priv->loop_end == old_length)) { + self->priv->play_end = self->priv->loop_end = self->priv->length; + g_object_notify ((GObject *) self, "loop-end"); + if (self->priv->loop_end <= self->priv->loop_start) { + self->priv->loop_start = self->priv->loop_end = -1; + self->priv->loop = FALSE; + g_object_notify ((GObject *) self, "loop-start"); + g_object_notify ((GObject *) self, "loop"); + } + } + } else { + self->priv->play_end = self->priv->length; + } + + bt_sequence_limit_play_pos_internal (self); +} + /* * bt_sequence_resize_data_length: * @self: the sequence to resize the length @@ -376,14 +415,18 @@ bt_sequence_get_nonnull_length (const BtSequence * const self) { * * Resizes the pattern data grid to the new length. Keeps previous values. */ -static void +void bt_sequence_resize_data_length (const BtSequence * const self, const gulong length) { const gulong tracks = self->priv->tracks; const gulong old_length = self->priv->len_patterns; - // try to shrink the pattern up to the row having the final non-empty pattern cell + /* Try to shrink the pattern up to the row having the final non-empty pattern + * cell if possible, but if the song end has been requested to be set to a + * point before that, then make sure pattern data isn't resized away and + * lost. + */ const gulong new_length = MAX(length, bt_sequence_get_nonnull_length (self)); @@ -441,27 +484,11 @@ bt_sequence_resize_data_length (const BtSequence * const self, const gulong leng } self->priv->len_patterns = new_length; - self->priv->length = MIN (self->priv->length, self->priv->len_patterns); - if (self->priv->loop_end != -1) { - // clip loopend to length or extend loop-end as well if loop_end was - // old length - if ((self->priv->loop_end > self->priv->length) || - (self->priv->loop_end == length)) { - self->priv->play_end = self->priv->loop_end = self->priv->length; - g_object_notify ((GObject *) self, "loop-end"); - if (self->priv->loop_end <= self->priv->loop_start) { - self->priv->loop_start = self->priv->loop_end = -1; - self->priv->loop = FALSE; - g_object_notify ((GObject *) self, "loop-start"); - g_object_notify ((GObject *) self, "loop"); - } - } - } else { - self->priv->play_end = self->priv->length; - } - - bt_sequence_limit_play_pos_internal (self); + // len_patterns should never be smaller than length, which is the song end. + // If this happened, there would be OOB memory reads. + g_assert (self->priv->len_patterns >= self->priv->length); + bt_sequence_post_length_change (self, length); g_object_notify ((GObject *) self, "len-patterns"); } @@ -1710,6 +1737,7 @@ bt_sequence_set_property (GObject * const object, const guint property_id, GST_DEBUG ("set the length for sequence: %lu", self->priv->length); bt_sequence_resize_data_length (self, self->priv->length); } + bt_sequence_post_length_change (self, length); break; } case SEQUENCE_TRACKS:{ @@ -1786,17 +1814,6 @@ bt_sequence_set_property (GObject * const object, const guint property_id, -1) ? self->priv->loop_end : self->priv->length; bt_sequence_limit_play_pos_internal (self); break; - case SEQUENCE_LEN_PATTERNS:{ - // TODO(ensonic): remove or better stop the song - // if(self->priv->is_playing) bt_sequence_stop(self); - // prepare new data - const gulong len_patterns_in = g_value_get_ulong (value); - if (len_patterns_in != self->priv->len_patterns) { - GST_DEBUG ("set the pattern length for sequence: %lu", len_patterns_in); - bt_sequence_resize_data_length (self, len_patterns_in); - } - break; - } default: G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec); break; @@ -2002,7 +2019,7 @@ bt_sequence_class_init (BtSequenceClass * const klass) g_object_class_install_property (gobject_class, SEQUENCE_LEN_PATTERNS, g_param_spec_ulong ("len-patterns", "patterns length prop", "total length of sequence in timeline bars", 0, G_MAXLONG, 0, - G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); + G_PARAM_READABLE | G_PARAM_STATIC_STRINGS)); g_object_class_install_property (gobject_class, SEQUENCE_TRACKS, g_param_spec_ulong ("tracks", diff --git a/src/lib/core/sequence.h b/src/lib/core/sequence.h index f10b01962..568d522bc 100644 --- a/src/lib/core/sequence.h +++ b/src/lib/core/sequence.h @@ -86,4 +86,8 @@ void bt_sequence_insert_full_rows(const BtSequence * const self, const gulong ti void bt_sequence_delete_rows(const BtSequence * const self, const gulong time, const glong track, const gulong rows); void bt_sequence_delete_full_rows(const BtSequence * const self, const gulong time, const gulong rows); +// This should only be needed by functions such as sequence data paste. +// It should ideally not be exposed at all, but this will do for now. +void bt_sequence_resize_data_length (const BtSequence * const self, const gulong length); + #endif // BT_SEQUENCE_H diff --git a/src/ui/edit/main-page-sequence.c b/src/ui/edit/main-page-sequence.c index 5d74f2a4d..f29be9d3e 100644 --- a/src/ui/edit/main-page-sequence.c +++ b/src/ui/edit/main-page-sequence.c @@ -1168,7 +1168,7 @@ on_sequence_label_edited (GtkCellRendererText * cellrenderertext, if (pos >= old_length) { new_length = pos + self->priv->bars; - g_object_set (self->priv->sequence, "len-patterns", new_length, NULL); + bt_sequence_resize_data_length (self->priv->sequence, new_length); sequence_calculate_visible_lines (self); sequence_update_model_length (self); @@ -3002,7 +3002,7 @@ on_sequence_table_key_press_event (GtkWidget * widget, GdkEventKey * event, if (row >= length) { new_length = row + self->priv->bars; - g_object_set (self->priv->sequence, "len-patterns", new_length, NULL); + bt_sequence_resize_data_length (self->priv->sequence, new_length); sequence_calculate_visible_lines (self); sequence_update_model_length (self); } @@ -4328,7 +4328,7 @@ sequence_clipboard_received_func (GtkClipboard * clipboard, end = beg + ticks; if (end > sequence_length) - g_object_set (self->priv->sequence, "len-patterns", end, NULL); + bt_sequence_resize_data_length (self->priv->sequence, end); GST_INFO ("pasting from row %d to %d", beg, end); diff --git a/tests/lib/core/t-sequence.c b/tests/lib/core/t-sequence.c index 8e546d1a3..047769e0f 100644 --- a/tests/lib/core/t-sequence.c +++ b/tests/lib/core/t-sequence.c @@ -265,7 +265,7 @@ START_TEST (test_bt_sequence_length_reduce_no_truncate) values are correctly updated and that no sequence data has been lost. */ GST_INFO ("-- act --"); g_object_set (sequence, "length", 4L, NULL); - g_object_set (sequence, "len-patterns", 8L, NULL); + bt_sequence_resize_data_length (sequence, 8L); GST_INFO ("-- assert --"); @@ -306,7 +306,7 @@ START_TEST (test_bt_sequence_length_reduce_and_persist) GST_INFO ("-- act --"); g_object_set (sequence, "length", 4L, NULL); - g_object_set (sequence, "len-patterns", 8L, NULL); + bt_sequence_resize_data_length (sequence, 8L); xmlDocPtr const doc = xmlNewDoc (XML_CHAR_PTR ("1.0")); xmlNodePtr node = bt_persistence_save (BT_PERSISTENCE (song), NULL, NULL);