Mental, could you please review my change to xml/repr-action.cpp in line 230, instead of
if (strcmp(action->act.chgattr.oldval, iter->act.chgattr.newval)) break;
I added extra checks
if (action->act.chgattr.oldval && iter->act.chgattr.newval && strcmp(action->act.chgattr.oldval, iter->act.chgattr.newval)) break;
Without this, it was crashing when you draw a rect and press and hold [ to rotate it, after several rotations. This crash appeared after I changed document_undo to maybe_undo with a key, so there's probably some deeper problem with your coalescing code that was not caught until now (because almost nobody used maybe_undo until now). So if you can discover what's really going wrong and fix it instead that would be great.
By the way I fixed the nasty "fly away" bug on rotation reported by Robert Crosbie and myself. Although I don't quite understand how that fix works :) See changelog.
_________________________________________________________________ Tired of spam? Get advanced junk mail protection with MSN 8. http://join.msn.com/?page=dept/bcomm&pgmarket=en-ca&RU=http%3a%2f%2f...
On Fri, 2004-01-23 at 11:09, bulia byak wrote:
Mental, could you please review my change to xml/repr-action.cpp in line 230, instead of
if (strcmp(action->act.chgattr.oldval, iter->act.chgattr.newval)) break;
I added extra checks
if (action->act.chgattr.oldval && iter->act.chgattr.newval && strcmp(action->act.chgattr.oldval, iter->act.chgattr.newval)) break;
I think that's nearly the right fix, though the intent is to break if the old and new values are different, which that won't quite do by itself.
Hrm. I wonder what the most succinct way of doing this is?
if ( action->act.chgattr.oldval && iter->act.chgattr.newval ) { if (strcmp(action->act.chgattr.oldval, iter->act.chgattr.newval)) { break; } } else { if ( action->act.chgattr.oldval != iter->act.chgattr.newval ) { break; } }
I think that's the best I can think of at the moment. The truth table is:
oldval NULL non-NULL +---------+---------------+ NULL | false | true | newval +---------+---------------+ non-NULL | true | strcmp() != 0 | +---------+---------------+
-mental
What I finally decided to do was remove the code that was doing the strcmp()s in the first place.
The existing optimization code was a little goofy; when a log was coalesced (at this point, mostly done by sp_document_maybe_done()), it iterated over the entire (combined) log, looking for consecutive actions to combine.
The new approach is to do that check every time an action is added to the log, and when two logs are joined by sp_repr_coalesce_log(), only the head and tail of the two logs are optimized in this fashion.
The new code doesn't use strcmp() anymore, and only tries to optimize one pair of events a time, so this should prove less expensive overall.
It probably is worth investigating at some point whether doing these optimizations is worth it at all, but for now this should do.
-mental
participants (2)
-
bulia byak -
MenTaLguY