Re: [Rosegarden-devel] An alternative to slotCommandExecuted()

[Chris C. <ca...@al...>]

Guillaume Laurent wrote:
> I've just committed what I believe is a better solution for post-command 
> execution refresh. [...]

I've taken a quick scan of the code and read most of the emails, but
I haven't finished building it yet.  Still, never too soon to comment.

In principle I think this is a very nice idea -- I'm all for techniques
that just make a note of the status and then work out what to do about
it later.  And there's a big win for things like CompoundCommand which
were previously dealt with in a particularly tasteless way.

A few questions:

 -- The SegmentRefreshStatus contains a single time range, right?
    (At first I'd expected it would contain a stack of time ranges,
    and the edit view would then have a "last read" index into that
    stack; the edit view's update method would then refresh all of
    the subsequent changes in turn.)  Presumably the way it's done
    now is quicker if you have many changes in a localised area of
    a segment, but slower if you change two things at opposite ends.

 -- Are you happy about the performance implications of having to
    update one RefreshStatus per view per event inserted?

 -- I'm not at all sure I like explicitly recording the refresh
    statuses in the Segment, including exposing them in the Segment
    API.  I tend to think that things stored in the Segment should
    all be candidates for writing to file, for example, and this
    obviously isn't.

 -- One thing I liked about the previous mechanism was our ability
    to effectively say "okay, I know nothing about this command but
    it's a pretty serious-looking one" (a TimeAndTempoChangeCommand,
    say) and respond by refreshing everything.  The corresponding
    behaviour with this mechanism will presumably be to modify a
    RefreshStatus in the Composition ("this Composition has changed"
    with no additional information) and always respond to that with
    a complete refresh.

 -- minor things: the accessor methods don't follow our guidelines
    or other Segment methods (should be "getXXX"); RefreshStatus::
    setNeedsRefresh doesn't return the bool it's declared to.

I may have more to say about it once I've actually tested it.


Chris