[squeak-dev] DoIts in Deltas - give us feedback please

Hi!

Ok, one of the things we did in Brest was to add first support for DoIts
in Deltas. Up to now a recording DSDelta creates DSChange instances
(subclasses thereof) for each kind of change.

This means we DO NOT create doits in Deltas for method removals, class
creation nor class initialization (although that actually will cause
code to be run that of course potentially could screw things up) etc.
All these three are instead covered by different DSChange objects.

BUT... if we want to replace Changesets we need to support doits at
least as much as Changesets do. A Changeset can have a preamble and a
postscript with code in them. This means you can include an arbitrary
snippet of code to be executed pre/post the install of the Changeset.

A Delta consists of an sequence of DSChanges. This means that we can put
a DSDoIt anywhere in that list - even in the middle. Ok, so we are
"better" than Changesets :)

BUT... a Delta is meant to be reversable etc, so by allowing doIts we
are moving into undefined territory. Thus, the medicine looks like this:

1. We do NOT record doits by default. Changesets actually don't record
doits either btw. :) But you can tell a DSDelta to record doits which of
course will log tons of crap, but it can be interesting to log and then
just prune it away. And yes, we could analyze the actual doits and do
some auto-pruning but whatever...

2. If you record a DSDoIt OR add one manually it will NOT be executed
when the Delta is applied! You need to EXPLICITLY mark it using
executeOnApply: true.

3. If a Delta contains a DSDoIt marked to execute on apply, the Delta
will be considered NON-reversable UNLESS you also supply an antiDoIt
code snippet! In this case DSDelta>>isReversable would say "true" BUT
this only means that it COULD PERHAPS be reversed.

Finally we will explicitly explain to the developer that:
--------------
If you still have a DoIt in your Delta that you intend to distribute,
make sure that:
                - It can cope with missing globals/classes!
                - It can be safely run multiple times without problems!
                - You marked it to be run on apply using #executeOnApply:. This is an
EXPLICIT step for a reason.
                - If it should do able to be REVERSED you need to supply an antiDoIt.
                - Put DoIts preferrably first or last in the Delta to avoid strange
situations. Remember that even if changes are ordered they may be
applied atomically by an applier.

NOTE: A Delta containing any DoIt will be visualized as "dangerous" in
the user interfaces so please avoid them unless you REALLY need them.
-------------


Well, something like that. Does the above handling of DoIts seem
reasonable? Any other suggestions or enhancements to all this?

Sidenote: If a Delta is set to record doits it will end up with *both* a
class creation change and a class creation doit. As the code stands
today the Delta can't tell the difference. I was inclined to perhaps use
the #kind: selector in AbstractEvent to distinguish the doits produced
by our own tools from doits performed by the developer himself in
ParagraphEditor etc. But I dropped that for now. As described above such
a recorded DoIt will not be executed on apply anyway - to be recording
doits is mainly useful as a "true log" of everything happening.

regards, Göran

PS. Any interesting thoughts about class initalization is also
appreciated. A Changeset slaps in an initialize-doit when being filedout
if touched class definitions (I think) has such a selector. Should we do
something similar on apply?

Hi Göran,

Göran Krampe wrote:
Sure.

> BUT... if we want to replace Changesets we need to support doits at
> least as much as Changesets do. A Changeset can have a preamble and a
> postscript with code in them. This means you can include an arbitrary
> snippet of code to be executed pre/post the install of the Changeset.

I really want to replace ChangeSets in Cuis. I don't want 2 mechanisms
for doing almost the same. Being able to remove ChangeSets is a must for me.

That's ok. The auto-prunning is not needed. If you log the doits, it is
because you want to face those tons of crap!

> 2. If you record a DSDoIt OR add one manually it will NOT be executed
> when the Delta is applied! You need to EXPLICITLY mark it using
> executeOnApply: true.
>
> 3. If a Delta contains a DSDoIt marked to execute on apply, the Delta
> will be considered NON-reversable UNLESS you also supply an antiDoIt
> code snippet! In this case DSDelta>>isReversable would say "true" BUT
> this only means that it COULD PERHAPS be reversed.

This is ok if class initialization is not done by a DSDoIt but, as you
suggest at the end, by a special mechanism.

If you support DoIts in the middle of the Delta, you also need to apply
the first part (perhaps atomically), then the DoIt, then the second part
(perhaps atomically). If this involves any complexity, then just only
handle DoIts at the ends of Deltas.

> NOTE: A Delta containing any DoIt will be visualized as "dangerous" in
> the user interfaces so please avoid them unless you REALLY need them.
> -------------
>
>
> Well, something like that. Does the above handling of DoIts seem
> reasonable? Any other suggestions or enhancements to all this?

Sounds great!

> Sidenote: If a Delta is set to record doits it will end up with *both*
> a class creation change and a class creation doit. As the code stands
> today the Delta can't tell the difference. I was inclined to perhaps
> use the #kind: selector in AbstractEvent to distinguish the doits
> produced by our own tools from doits performed by the developer
> himself in ParagraphEditor etc. But I dropped that for now. As
> described above such a recorded DoIt will not be executed on apply
> anyway - to be recording doits is mainly useful as a "true log" of
> everything happening.

Don't worry about that. If you're recording DoIts, you already have tons
of crap anyway.

>
> regards, Göran
>
> PS. Any interesting thoughts about class initalization is also
> appreciated. A Changeset slaps in an initialize-doit when being
> filedout if touched class definitions (I think) has such a selector.
> Should we do something similar on apply?

The most needed use of DoIts and postscripts is indeed class
initialization. ChangeSets add the doIt only if the #initialize method
is modified (at least in Cuis). I believe these DoIts have these properties:

a- They can be created automatically, if the #initialize method changes,
or (as you say) if the shape changes
b- They are not dangerous
c- They can be applied multiple times
d- Their reverse is just to evaluate them again after undoing the
changes (or doing nothing if the class doesn't exist anymore)

I think calling #initialize automatically on apply (and unapply) is ok.
There is a little risk, but not much bigger than being able to modify
any method in the system.

This all looks really great. I guess I'll be trying DS in Cuis pretty soon.

Cheers,
Juan Vuletich

Hi!

Juan Vuletich wrote:
> I really want to replace ChangeSets in Cuis. I don't want 2 mechanisms
> for doing almost the same. Being able to remove ChangeSets is a must for
> me.

Yes, I agree. If Deltas don't end up being 100% a superset of Changeset
then we will have a problem.

>> 1. We do NOT record doits by default. Changesets actually don't record
>> doits either btw. :) But you can tell a DSDelta to record doits which
>> of course will log tons of crap, but it can be interesting to log and
>> then just prune it away. And yes, we could analyze the actual doits
>> and do some auto-pruning but whatever...
>
> That's ok. The auto-prunning is not needed. If you log the doits, it is
> because you want to face those tons of crap!

Right, I agree.

Right. The approach above is to force people to "think" about what a
reverse doit needs to do. Today the implementation of asAntiChange
simply swaps the doIt and the antiDoIt source. So if the reverse should
just "run the same code again" - then you supply the exact same code as
the antiDoIt - we do not presume that as a default case. Again, forcing
the developer to be explicit in order to be on the "safe side".

Yeah... Splitting the "parts" between the doits into atoms is not hard
in SystemEditor. So yeah, we need to decide to do that - or to not
support "in between doits".

>> NOTE: A Delta containing any DoIt will be visualized as "dangerous" in
>> the user interfaces so please avoid them unless you REALLY need them.
>> -------------
>>
>>
>> Well, something like that. Does the above handling of DoIts seem
>> reasonable? Any other suggestions or enhancements to all this?
>
> Sounds great!

Good.

>> PS. Any interesting thoughts about class initalization is also
>> appreciated. A Changeset slaps in an initialize-doit when being
>> filedout if touched class definitions (I think) has such a selector.
>> Should we do something similar on apply?
>
> The most needed use of DoIts and postscripts is indeed class
> initialization. ChangeSets add the doIt only if the #initialize method
> is modified (at least in Cuis).

Ok, then I perhaps misread the code the first time. So if I add a class
var in a class it does NOT add an initializer? Ehrm.

 > I believe these DoIts have these
> properties:
>
> a- They can be created automatically, if the #initialize method changes,
> or (as you say) if the shape changes

Yes, intuitively it feels like they should be run after...

- class var changes (obviously)
- a changed or added #initialize method. (obviously)
- class ivar changes ...

ehm... darnit I need to read up on class initialization...

> b- They are not dangerous

Should not be :)

> c- They can be applied multiple times

Should be :)

> d- Their reverse is just to evaluate them again after undoing the
> changes (or doing nothing if the class doesn't exist anymore)

Right.

> I think calling #initialize automatically on apply (and unapply) is ok.
> There is a little risk, but not much bigger than being able to modify
> any method in the system.

Which implies we do NOT represent these calls as DSChanges, right? Just
put the logic into the applier?

> This all looks really great. I guess I'll be trying DS in Cuis pretty soon.

:)

regards, Göran

Hi Göran -

I'm not sure what the UI will look like but in general I like what
you're proposing. The most important property here is predictability. As
long as people know the constraints they can work within them, so
requiring that for a doIt to be reversible it needs to have an
"anti-doIt" is just fine (and was my preferred choice after reading the
first paragraph of your message ;-) For the records, many of the
problems I've seen with Monticello come from it doing things seemingly
unpredictably and consequently you can't address them beforehand since
you don't really know what might go wrong. It's a really important
lesson to keep in mind.

However, in point #2 below (i.e., about the doIt "it will NOT be
executed when the Delta is applied") I think you're being overly
cautious which will only lead to confusion. What is the point of
recording a doIt it if it's not applied? I cannot think of a case where
I'd like to cover a doIt and not apply it - if I have such an entity I
call it a comment, not a doIt and this distinction should be explicit.

In other words, if I mean to provide a doIt for information purposes
then I should add this as a comment instead of a doIt since it has no
implication on the delta stream. But if I record (or add) a doIt then I
*mean* to execute it, otherwise I'd use a comment. Having a doIt that's
also sort-of-a-comment will only cause confusion in the long term. Be
clear about the purpose of comments (you do have comments, don't you?)
and things will work out fine.

BTW, you say in your things to explain list that the doIt was marked "to
be run on apply using #executeOnApply:. This is an EXPLICIT step for a
reason." But you haven't actually provided a reasoning for it, just said
that the developer needs to do so and honestly I fail to see it :-)

Oh, and I think that marking a doIt as "dangerous" in the UI is silly. A
doIt is just as dangerous as any other bit of code. Just mark it as a
doIt, that is code executed directly when loaded[*] and it'll be fine.

[*] Funny, I only noticed when re-reading that my very *definition* of a
doIt is a bit of code that's executed directly when loaded so the whole
"executeOnApply" bit in my book is really a contradiction for something
called a doIt.

Lastly, class initialization. Same rules apply as above, just be
predictable. I don't think you'll get around to supporting class
initialization but the tricky part here is the ordering when you really
want it to be reversible. You may have to support out-of-order
anti-doIts that is if you have a series of changes followed by a doIt,
you must reverse all the preceding changes before applying the anti-doIt.

FWIW, I don't think the ability to reverse doIts and class
initialization is relevant in any practical context. Nobody's cared
about that with Monticello at all and things still work quite all right.
Just be predictable (he sez for one last time ;-)

Cheers,
   - Andreas


Göran Krampe wrote:

Hi Andreas!

Andreas Raab wrote:
[SNIP of stuff we agree on]
> However, in point #2 below (i.e., about the doIt "it will NOT be
> executed when the Delta is applied") I think you're being overly
> cautious which will only lead to confusion. What is the point of
> recording a doIt it if it's not applied? I cannot think of a case where
> I'd like to cover a doIt and not apply it - if I have such an entity I
> call it a comment, not a doIt and this distinction should be explicit.

Mmm, my point was probably that if you set a Delta to record doIts and
then just file it out without looking at it you will be bringing lots of
garbage along...

So the whole idea was to prevent such "accidents" by making sure that
the developer marks the doIts (a menu choice or such).

One simpler approach may be to just throw up a warning when one is about
to "distribute" a Delta (in whichever way that may be, outside the
image) that it indeed includes x number of doIts, proceed or cancel.

> In other words, if I mean to provide a doIt for information purposes
> then I should add this as a comment instead of a doIt since it has no
> implication on the delta stream. But if I record (or add) a doIt then I
> *mean* to execute it, otherwise I'd use a comment. Having a doIt that's
> also sort-of-a-comment will only cause confusion in the long term. Be
> clear about the purpose of comments (you do have comments, don't you?)
> and things will work out fine.

Hehe, comments... yeah, we do since the Delta itself and also each
Change has a "properties" dictionary which can contain all Tirade proper
keys/values. So adding a #comment property with accessors should be
trivial, thanks for reminding me. This should also be done to cover the
preamble using some naming convention.

> BTW, you say in your things to explain list that the doIt was marked "to
> be run on apply using #executeOnApply:. This is an EXPLICIT step for a
> reason." But you haven't actually provided a reasoning for it, just said
> that the developer needs to do so and honestly I fail to see it :-)

Again, the idea was to prevent doIts to "slip in" without the developer
doing it consciously. Perhaps a mere "warning" as described above would
be sufficient?

> Oh, and I think that marking a doIt as "dangerous" in the UI is silly. A
> doIt is just as dangerous as any other bit of code. Just mark it as a
> doIt, that is code executed directly when loaded[*] and it'll be fine.
>
> [*] Funny, I only noticed when re-reading that my very *definition* of a
> doIt is a bit of code that's executed directly when loaded so the whole
> "executeOnApply" bit in my book is really a contradiction for something
> called a doIt.

Mmmmm, not sure what you mean here [*] - a Delta is first deserialized
into the image and then applied. Ahh... you meant that it would be odd
to include doIts that are not executable on apply? Yes, that would
indeed be odd UNLESS you take into account that a Delta can do double
duty as a true log.

Also, I meant "dangerous" as in "makes the reversability unpredictable".
If you do not have doIts then Deltas can actually assure you
reversability (unless there is a bug of course).

> Lastly, class initialization. Same rules apply as above, just be
> predictable.

Yes, that is very important.

 > I don't think you'll get around to supporting class
> initialization but the tricky part here is the ordering when you really
> want it to be reversible. You may have to support out-of-order
> anti-doIts that is if you have a series of changes followed by a doIt,
> you must reverse all the preceding changes before applying the anti-doIt.

Humptidum... well, I wasn't going to use doIts for class initialization!

> FWIW, I don't think the ability to reverse doIts and class
> initialization is relevant in any practical context. Nobody's cared
> about that with Monticello at all and things still work quite all right.
> Just be predictable (he sez for one last time ;-)

So to sum it up, how about this then:

1. If we record doIts they are NOT marked to executeOnApply. To prevent
accidental replaying of a log. You can then use the UI, select such a
doIt and just use a menu choice to mark it as executeOnApply if you like to.

2. If you *add* a doIt using the UI for Deltas (which probably will be
the reasonable way to add doIts anyway, just like with
preamble/postscripts today) then the default will be executeOnApply =
true. A second pane will be available for the antiDoit.

3. Skip the warning on distribution, better to make sure the UI shows
that a Delta includes doIts that has executeOnApply = true. Fair?

4. Let the applier contain the class initialization logic. We do not add
any Changes specifically for that. And let's make it predictable. I
still would like to see a proposal for that logic. AFAICT it is a bit
tricksy, especially regarding class ivars and inherited class side
#initialize.

regards, Göran

My 2 cents about class initialization..
I think that user could tell us what classes he wants to initialize
after delta applied.
And this means that we don't need to record this as doit, but as
separate "change" type.
The apply logic of this change could be simple:
- send #initialize to object which can be found in system dictionary
under user's provided name.
- if system dictionary doesn't contains such global name, simply skip over.
- the anti-change of this change type returns self (but always should
be applied last, maybe?).
(because if you remove something from class, either you adding
something to class, or changing something,
you still have to send same #initialize message).

--
Best regards,
Igor Stasenko AKA sig.

Göran Krampe wrote:
> Mmm, my point was probably that if you set a Delta to record doIts and
> then just file it out without looking at it you will be bringing lots of
> garbage along...

Well, then don't do that! ;-) No, seriously, what is the use case for
setting a DS for recording doIts? Are you trying to replace the changes
file as well? I can see how this might be appealing but I think that's
bit of a distraction at this point. What the (Squeak) world needs isn't
really a better changes file. What it needs is a better change set
mechanism. I think that by not recording doIts by default you're not
loosing any value in the near term and you are avoiding issues that you
may not be ready to address yet.

> 1. If we record doIts they are NOT marked to executeOnApply. To prevent
> accidental replaying of a log. You can then use the UI, select such a
> doIt and just use a menu choice to mark it as executeOnApply if you like
> to.

See above. I would just say that for the time being you need to add
doIts explicitly, they don't get recorded. Just like change sets. It's a
simple model that people already understand.

> 2. If you *add* a doIt using the UI for Deltas (which probably will be
> the reasonable way to add doIts anyway, just like with
> preamble/postscripts today) then the default will be executeOnApply =
> true. A second pane will be available for the antiDoit.

Sounds good.

> 3. Skip the warning on distribution, better to make sure the UI shows
> that a Delta includes doIts that has executeOnApply = true. Fair?

Sounds good.

> 4. Let the applier contain the class initialization logic. We do not add
> any Changes specifically for that. And let's make it predictable. I
> still would like to see a proposal for that logic. AFAICT it is a bit
> tricksy, especially regarding class ivars and inherited class side
> #initialize.

I'd say that the general rules of thumb should be: Superclasses are
initialized before subclasses. Otherwise initialization occurs strictly
in alphabetical order. The applier probably needs to do that at the end
of the application / reversal of the delta stream and should do it
whenever the initialize method was touched.

Cheers,
   - Andreas

Hi!

Andreas Raab wrote:
I am all with ya, and it will not be set to true per default. :) So
that's fine.

>> 1. If we record doIts they are NOT marked to executeOnApply. To
>> prevent accidental replaying of a log. You can then use the UI, select
>> such a doIt and just use a menu choice to mark it as executeOnApply if
>> you like to.
>
> See above. I would just say that for the time being you need to add
> doIts explicitly, they don't get recorded. Just like change sets. It's a
> simple model that people already understand.

Sure, it will be false per default :)

Ah, ok, finally some proposed logic. :) Excuse me for being dumb here, but:
- Do we only send initialize to classes that implement it?
- Do we only trigger it by touching the method? What about shape
changes? (as in changed class vars, pools (?) or class ivars)

I am presuming you have spent tons of more thought on this - that is why
I am picking your brain.

regards, Göran

Göran Krampe wrote:
Yes.

> - Do we only trigger it by touching the method? What about shape
> changes? (as in changed class vars, pools (?) or class ivars)

Yes and No. Yes, when an initialize method is touched it should be
called (this "trick" proved extremely useful in Monticello). No, class
shape changes shouldn't trigger it (I think).

> I am presuming you have spent tons of more thought on this - that is why
> I am picking your brain.

Actually, not too much. I'm mostly going by my experience with
Monticello vs. change sets.

Cheers,
   - Andreas

2009/9/10 Andreas Raab <[hidden email]>:
I think it is completely useless to put such trick(s) in design, because they
a) do not cover 100% of different cases when something changed and
requires class (re)initialization
b) makes deltas less predictable (by introducing some fuzzy logic)
which contradicts the design principles we are declared first and
foremost :)

So, my point , that #initialize should be controlled by users
explicitly without any automagic.



--
Best regards,
Igor Stasenko AKA sig.

Hi guys!

Igor Stasenko wrote:
Ok, here is a suggestion:

- We add a Change object that is DSClassInitializationChange. So Igor is
right in a sense, better to put the developer in full control. This
change could just list a bunch of class names that it will send
initialize to - in that order.

- Thus the developer can edit/add such a change as needed.

- And we add some predictable logic to cover 95% of the cases by auto
creating such a change.

Given what Andreas said in the beginning about how class initialization
should be handled on revert, perhaps it gets tricky. I need to read that
again...

One definition of predictable logic could be:

- Trigger initialization by added or modified class side #initialize method.
- Make the sorting order be "least number of superclasses" first, class
name second.

regards, Göran

Göran Krampe wrote:
Works for me.

> Given what Andreas said in the beginning about how class initialization
> should be handled on revert, perhaps it gets tricky. I need to read that
> again...

Don't worry too much about it. I don't think that this is the most
critical aspect here and I'd much rather have something to play with
sooner ;-)

Cheers,
   - Andreas