The Inbox: Monticello-bf.532.mcz

Bert Freudenberg uploaded a new version of Monticello to project The Inbox:
http://source.squeak.org/inbox/Monticello-bf.532.mcz

==================== Summary ====================

Name: Monticello-bf.532
Author: bf
Time: 24 January 2013, 4:05:51.496 pm
UUID: 71bddd0f-d5ac-488b-a8e1-729f2a06474e
Ancestors: Monticello-bf.531

Save dialog now shows a list of changes to be submitted. Clicking a list item shows a diff in the lower pane. Advanced users can also make this save ignore individual changes using the item's context menu.

=============== Diff against Monticello-bf.531 ===============

Item was added:
+ ----- Method: MCPatch>>ignoring: (in category 'accessing') -----
+ ignoring: ignoredOperations
+ ^ MCPatch operations: (operations difference: ignoredOperations)!

Item was changed:
+ MCPatchBrowser subclass: #MCSaveVersionDialog
+ instanceVariableNames: 'name message ignore'
- MCTool subclass: #MCSaveVersionDialog
- instanceVariableNames: 'name message'
  classVariableNames: ''
  poolDictionaries: ''
  category: 'Monticello-UI'!

Item was changed:
  ----- Method: MCSaveVersionDialog>>accept (in category 'as yet unclassified') -----
  accept
  self answer:
  (Array
  with: (self findTextMorph: #versionName) text asString
+ with: (self findTextMorph: #logMessage) text asString
+ with: ignore)
+ !
- with: (self findTextMorph: #logMessage) text asString)
- !

Item was added:
+ ----- Method: MCSaveVersionDialog>>ignore (in category 'as yet unclassified') -----
+ ignore
+ ^ ignore ifNil: [ignore := Set new]!

Item was added:
+ ----- Method: MCSaveVersionDialog>>ignoreSelection (in category 'as yet unclassified') -----
+ ignoreSelection
+ selection
+ ifNil: [ignore size = items size
+ ifFalse: [ignore addAll: items]
+ ifTrue: [ignore removeAll]]
+ ifNotNil: [
+ ignore remove: selection ifAbsent: [
+ ignore add: selection]].
+ self changed: #list
+ !

Item was added:
+ ----- Method: MCSaveVersionDialog>>installSelection (in category 'as yet unclassified') -----
+ installSelection
+ super installSelection.
+ selection ifNotNil: [
+ ignore remove: selection ifAbsent: [].
+ self changed: #list].
+
+ !

Item was added:
+ ----- Method: MCSaveVersionDialog>>list (in category 'as yet unclassified') -----
+ list
+ ^ self items collect: [:ea |
+ (self ignore includes: ea)
+ ifFalse: [ea summary]
+ ifTrue: [Text string: '( ', ea summary, ' )' attribute: TextEmphasis struckOut ]]!

Item was added:
+ ----- Method: MCSaveVersionDialog>>methodListKey:from: (in category 'as yet unclassified') -----
+ methodListKey: aKeystroke from: aListMorph
+ aKeystroke caseOf: {
+ [$I] -> [self ignoreSelection].
+ } otherwise: [super methodListKey: aKeystroke from: aListMorph ]!

Item was added:
+ ----- Method: MCSaveVersionDialog>>methodListMenu: (in category 'as yet unclassified') -----
+ methodListMenu: aMenu
+ aMenu addList:#(
+ ('ignore (I)' ignoreSelection 'Do not include this change when saving')
+ -).
+ super methodListMenu: aMenu.
+ ^aMenu!

Item was added:
+ ----- Method: MCSaveVersionDialog>>revertSelection (in category 'as yet unclassified') -----
+ revertSelection
+ super revertSelection.
+ selection ifNotNil: [
+ ignore add: selection.
+ self changed: #list].
+ !

Item was changed:
  ----- Method: MCSaveVersionDialog>>widgetSpecs (in category 'as yet unclassified') -----
  widgetSpecs
  ^ #(
  ((textMorph: versionName) (0 0 1 0) (0 0 0 30))
+ ((textMorph: logMessage) (0 0 1 0.3) (0 30 0 -30))
+ ((buttonRow) (0 0.3 1 0.3) (0 -40 0 0))
+ ((listMorph:selection:menu:keystroke: list selection methodListMenu: methodListKey:from:) (0 0.3 1 0.6) (0 0 0 0))
+ ((textMorph: text) (0 0.6 1 1) (0 0 0 0))
- ((textMorph: logMessage) (0 0 1 1) (0 30 0 -30))
- ((buttonRow) (0 1 1 1) (0 -40 0 0))
  )!

Item was changed:
  Notification subclass: #MCVersionNameAndMessageRequest
+ instanceVariableNames: 'suggestion initialMessage patch'
- instanceVariableNames: 'suggestion initialMessage'
  classVariableNames: ''
  poolDictionaries: ''
  category: 'Monticello-Versioning'!

Item was changed:
  ----- Method: MCVersionNameAndMessageRequest>>defaultAction (in category 'as yet unclassified') -----
  defaultAction
  ^ MCSaveVersionDialog new
  versionName: suggestion;
  logMessage: initialMessage;
+ patch: patch;
  showModally!

Item was added:
+ ----- Method: MCVersionNameAndMessageRequest>>patch (in category 'as yet unclassified') -----
+ patch
+ ^ patch!

Item was added:
+ ----- Method: MCVersionNameAndMessageRequest>>patch: (in category 'as yet unclassified') -----
+ patch: aPatch
+ patch := aPatch
+ !

Item was changed:
  ----- Method: MCWorkingCopy>>newVersion (in category 'operations') -----
  newVersion
+ | packageSnapshot parentSnapshot patch |
+ parentSnapshot := self parentSnapshot.
+ packageSnapshot := package snapshot.
+ patch := packageSnapshot patchRelativeToBase: parentSnapshot.
  ^ (self requestVersionNameAndMessageWithSuggestion: self uniqueVersionName
+ initialMessage: self patchMessageDefault
+ patch: patch) ifNotNil:
+ [:tuple |
+ self newVersionWithName: tuple first withBlanksTrimmed
+ message: (self patchMessageStripped: tuple second)
+ snapshot: (tuple third
+ ifEmpty: [packageSnapshot]
+ ifNotEmpty: [
+ MCPatcher apply: (patch ignoring: tuple third)
+ to: parentSnapshot])]
- initialMessage: self patchMessageSuggestion) ifNotNil:
- [:pair |
- self newVersionWithName: pair first withBlanksTrimmed
- message: (self patchMessageStripped: pair last)].
  !

Item was changed:
  ----- Method: MCWorkingCopy>>newVersionWithName:message: (in category 'operations') -----
  newVersionWithName: nameString message: messageString
+ ^self newVersionWithName: nameString message: messageString snapshot: package snapshot!
- | info deps |
- info := ancestry infoWithName: nameString message: messageString.
- ancestry := MCWorkingAncestry new addAncestor: info.
- self modified: true; modified: false.
-
- deps := self requiredPackages collect:
- [:ea |
- MCVersionDependency
- package: ea
- info: ea workingCopy currentVersionInfo].
-
- ^ MCVersion
- package: package
- info: info
- snapshot: package snapshot
- dependencies: deps!

Item was added:
+ ----- Method: MCWorkingCopy>>newVersionWithName:message:snapshot: (in category 'operations') -----
+ newVersionWithName: nameString message: messageString snapshot: aSnapshot
+ | info deps clean |
+ info := ancestry infoWithName: nameString message: messageString.
+ ancestry := MCWorkingAncestry new addAncestor: info.
+ clean := (package snapshot patchRelativeToBase: aSnapshot) isEmpty.
+ self modified: clean; modified: clean not. "hack to ensure label is updated"
+
+ deps := self requiredPackages collect:
+ [:ea |
+ MCVersionDependency
+ package: ea
+ info: ea workingCopy currentVersionInfo].
+
+ ^ MCVersion
+ package: package
+ info: info
+ snapshot: aSnapshot
+ dependencies: deps!

Item was added:
+ ----- Method: MCWorkingCopy>>parentSnapshot (in category 'private') -----
+ parentSnapshot
+ ^ self findSnapshotWithVersionInfo: (self ancestors
+ ifEmpty: [nil]
+ ifNotEmpty: [self ancestors first])!

Item was changed:
  ----- Method: MCWorkingCopy>>patchMessageChanges (in category 'operations') -----
  patchMessageChanges
+ | changes |
+ changes := package snapshot patchRelativeToBase: self parentSnapshot.
- | changes parentInfo parentSnapshot |
- parentInfo := self ancestors
- ifEmpty: [nil]
- ifNotEmpty: [self ancestors first].
- parentSnapshot := self findSnapshotWithVersionInfo: parentInfo.
- changes := package snapshot patchRelativeToBase: parentSnapshot.
  ^ (MCPatchMessage new patch: changes) message!

Item was removed:
- ----- Method: MCWorkingCopy>>requestVersionNameAndMessageWithSuggestion:initialMessage: (in category 'private') -----
- requestVersionNameAndMessageWithSuggestion: nameString initialMessage: msgString
- ^ (MCVersionNameAndMessageRequest new
- suggestedName: nameString;
- initialMessage: msgString
- ) signal!

Item was added:
+ ----- Method: MCWorkingCopy>>requestVersionNameAndMessageWithSuggestion:initialMessage:patch: (in category 'private') -----
+ requestVersionNameAndMessageWithSuggestion: nameString initialMessage: msgString patch: aPatch
+ ^ (MCVersionNameAndMessageRequest new
+ suggestedName: nameString;
+ initialMessage: msgString;
+ patch: aPatch
+ ) signal!

On 25.01.2013, at 00:06, [hidden email] wrote:



Please try this to see if you like it better than the current save dialog. I've been using it for a few days and it seemed to work fine. In my workflow I always press the "changes" button before saving anyway, so this saves time. Plus being able to commit even a "dirty" package is something I had wanted for a while.

One change in behavior is that the package snapshot is already taken when pressing the save button, not after accepting the version name dialog.

- Bert -

Am 25.01.2013 um 01:12 schrieb Bert Freudenberg <[hidden email]>:

If this means cherry-picking what you are about to commit (a la  'git add'), want!

Best
        -Tobias

On 25.01.2013, at 00:11, Tobias Pape <[hidden email]> wrote:

Precisely. Unlike git, however, the default is to commit everything, just like it used to be.

- Bert -

> Please try this to see if you like it better than the current save dialog. I've been using it for a few days and it seemed to work fine. In my workflow I always press the "changes" button before saving anyway, so this saves time.

I, too, always end up pressing Changes and Save together (and, often,
History so I can see what my prior version included), so this seems
good.  But...

> Plus being able to commit even a "dirty" package is something I had wanted for a while.

What is this?  Is this the new "(I)gnore" option on the menu?  When I
want to ignore a particular change, what I always did was leave the
Changes list open and, after saving, simply click "install" on the
change I wanted to ignore.

> One change in behavior is that the package snapshot is already taken when pressing the save button, not after accepting the version name dialog.

Hmmmm....  Ok, so this is to support the new Ignore option.  I don't
think I like this -- because a regular part of _reviewing_ is being
able to make a tweak fix!  If you can't remove a leftover "self halt"
or make any other fixes then what's the point of reviewing it in this
fashion?  I'll have to close the dialog, make the change, and then
re-click the Save button -- which is quite a few more gestures than
what we have today.

What if we just made the save button open up the Save dialog *as well
as* the Changes dialog?

On 25.01.2013, at 08:43, Chris Muller <[hidden email]> wrote:


Try it: if you revert a change from the save dialog, it will also be ignored. This modifies the snapshot taken earlier before saving. But it will not see changes you make outside the save dialog, yes. In my workflow, when I made changes, I always used to cancel the save dialog and bring it up again. So for me it's the same.

- Bert -

I sometimes like to leave Save dialogs open for a _long_ time;
sometimes days, so I can accumulate the notes as I develop them.

Even when I *don't* do that, I can still accumulate a lot of changes
between versions and so there is a lot of reviewing to make sure
everything is ok and to document the why of everything I did.  But
because I've accumulated a lot of changes, the process of saving can
take more than just a few minutes, during which often I will get
interrupted by something external.

When I come back, the Save dialog is staring at me but there is this
"doubt" about whether I had changed anything in the image.  Or, maybe
just during review I see something that needs a spelling correction,
or add a comment, but not reverted.  Did I do that BEFORE this save
dialog or was that the LAST save dialog?  I'm forced to access my
short-term memory to make that judgement and so there is always this
lingering doubt about whether what I'm saving is congruent with the
state of my image.  I just can't take that.

I do like what you're trying to do here in terms of streamlining the
Review & Save use-case.  How about, upon clicking Save, it simply
re-snapshots the package, then enumerates the definitions and
re-ignore the ones which the user said to (I)gnore the first time and
re-ignore them just before saving?

> ...  How about, upon clicking Save, it simply
> re-snapshots the package, then...

I meant upon clicking the final "Ok" button on the Save dialog, not
the "Save" button on the MC browser..

+1
I like it because it fits my model
I generally have several changes/experiments on the grill and this is
a great improvment of efficiency

Of course, there is always a danger because we did not commit exactly
what we tested.
But we're supposed to know what we are doing.

It would be even better is cmd+I would select the next item in the
list (like keep or reject in a merge)

Nicolas

2013/1/25 Bert Freudenberg <[hidden email]>:

On 25.01.2013, at 13:30, Nicolas Cellier <[hidden email]> wrote:

> +1
> I like it because it fits my model
> I generally have several changes/experiments on the grill and this is
> a great improvment of efficiency

Right, same here.

> Of course, there is always a danger because we did not commit exactly
> what we tested.
> But we're supposed to know what we are doing.

;)

> It would be even better is cmd+I would select the next item in the
> list (like keep or reject in a merge)

Good point, that would be an improvement.

- Bert -

On 25.01.2013, at 11:52, Chris Muller <[hidden email]> wrote:

I have to think about that.

The advantage of what I proposed is that in the changes list you see exactly what will be committed, even if it does not match exactly what's in your image.

But I see that this would not mash with your working style.

Maybe the best of both worlds would be if any change done while the save dialog is open would be added to the changes list automatically? Not sure how to implement that though, MC is thoroughly snapshot-based, and taking a snapshot is not cheap.

Easier to implement might be to warn about intermediate changes. Upon accepting the save dialog I snapshot the package anyway, to mark the working copy dirty or clean. If this snapshot differs from the earlier snapshot, it could bring up a warning. Or even better, it could update the changes list with the current one. How does that sound?

- Bert -

>> I do like what you're trying to do here in terms of streamlining the
>> Review & Save use-case.  How about, upon clicking Save, it simply
>> re-snapshots the package, then enumerates the definitions and
>> re-ignore the ones which the user said to (I)gnore the first time and
>> re-ignore them just before saving?
>
> I have to think about that.

The goal is to streamline the Review & Save use-case so we don't have
to click two buttons (Changes and Save) to open two windows up every
time.  Is that right?

But by not allowing *corrections* to methods during Review, it
actually makes things a lot worse.  Let's detail the process steps of
the Review & Save use-case.  First with what we have today:

   1) click the Save button.
   2) click the Changes button.
   3) review the changes, typing release-notes text as you go.  Make
any necessary improvements right then and there based on the review.
           (3.1)   -- If the phone rings, pick up the phone and have a
conversation --.)
   4) click the Ok button and be assured, everything that was reviewed
and corrected is now saved.  Serenity!

With Monticello-bf.532.mcz here are the steps:

   1) click the Save button.
   2) review the changes, typing release-notes text as you go.  Make
any necessary improvements right then and there based on the review.
   3) if no changes were made, skip to step 8.
   4) copy the release-notes to the clipboard,
   5) click the Cancel button to close the Save window,
   6) click the Save button again to reopen it.
   7) re-paste the release-notes text.
             (7.1)  -- If the phone rings, pick up the phone and have
a conversation --.)
   8) click "Ok" this time to save what was listed in the.
   9) Feel insecure about whether all the review updates are, in fact,
saved.  Stress!  Better double check, step 10??

The irony is that by every effort to integrate Review+Save, they are
being made wholly separate activities; exactly the opposite of the
intended goal.

> Maybe the best of both worlds would be if any change done while the save dialog is open would be added to the changes list automatically? Not sure how to implement that though, MC is thoroughly snapshot-based, and taking a snapshot is not cheap.

No, the solution is to simply do what we have today.


> Easier to implement might be to warn about intermediate changes.

No, we need to be moving toward a _less_ rather than more intrusive
UI.  Warnings create a modal situation and end up getting ignored.
The user must stay in control.  See

    http://en.wikipedia.org/wiki/The_Humane_Interface

> Upon accepting the save dialog I snapshot the package anyway,

Yes!

> to mark the working copy dirty or clean. If this snapshot differs from the earlier snapshot, it could bring up a
> warning.

No!

> Or even better, it could update the changes list with the current one. How does that sound?

Yes, we must preserve the original save behavior by taking a final
snapshot and if you want that new (I)gnore feature, then simply let
that function capture the #description of the Definitions being
ignored.  Then when the user clicks OK on the Save dialog, after the
new snapshot, THEN enumerate those definitions of the new snapshot and
skip the ones whose description matches the ones previously ignored.

This way the acts of Reviewing and Saving can truly be integrated into
one step.  Thanks.

On 26.01.2013, at 00:50, Chris Muller <[hidden email]> wrote:

That, and being able to selectively commit changes.

5) Nothing happens. Why? Oh, I remember, an error popped up while I was reviewing and fixing stuff, which killed the process that the save dialog is supposed to be waiting on.
6) copy the release-notes to the clipboard,
7) click the Cancel button to close the Save window,
8) click the Save button again to reopen it.
9) re-paste the release-notes text.
10) click "Ok" this time to save what was listed in the.

(this problem is actually not addressed by my version, but it's the reason why I always open a new save dialog when I do want to save)

You should start writing comedy ;)

> The irony is that by every effort to integrate Review+Save, they are
> being made wholly separate activities; exactly the opposite of the
> intended goal.

It also encourages beginners (and reminds seasoned developers) to actually review the changes they are about to commit. That was the purpose of the textual list of changes in the save dialog, and it is made even more in-the-face by the proposed change.

>> Maybe the best of both worlds would be if any change done while the save dialog is open would be added to the changes list automatically? Not sure how to implement that though, MC is thoroughly snapshot-based, and taking a snapshot is not cheap.
>
> No, the solution is to simply do what we have today.

Other folks seem to like where I was heading. Let's find a solution. I'm pretty sure we can have our cake and eat it, too.

>> Easier to implement might be to warn about intermediate changes.
>
> No, we need to be moving toward a _less_ rather than more intrusive
> UI.  Warnings create a modal situation and end up getting ignored.
> The user must stay in control.  See
>
>    http://en.wikipedia.org/wiki/The_Humane_Interface

Agreed, I was thinking out loud and had a better idea below.

So we are in agreement after all? Why do you make it sound all bad then?

- Bert -

Am 25.01.2013 um 17:19 schrieb Bert Freudenberg <[hidden email]>:



\o/

Best
        -Tobias

That's what I was doing!  :)  Just having a little fun while we
discuss, I hope you didn't mind.

> It also encourages beginners (and reminds seasoned developers) to actually review the changes they are about
> to commit. That was the purpose of the textual list of changes in the save dialog, and it is made even more in
> -the-face by the proposed change.

Oh yes, I love having the changes list right in there, that's great!
My only objection is the change to not re-snapshot when the OK button
is pressed.  We must be able to go out into browsers and make fixes,
add comments, etc. during the review without needing to close/reopen
that Save dialog.

Ok cool!  Hey, in my heart there is nothing but affection and
admiration toward u and most everyone here.  :)  I know I don't always
have the best delivery.  I truly was just having "fun" with a little
melodrama and those lists, I hope you were entertained.  I need to
remember e-mail is low-bandwidth..