Any fundamental objections before I would merge this into the Trunk? :-)
=============== Summary ===============
Change Set: mcSlips Date: 16 January 2022 Author: Christoph Thiede
This changeset integrates the slip detection mechanism, previously only known from the domain of changesets, into the Monticello system. When users save a new version, it will automatically notify them about any possible slips and let them decide whether to browse them first or proceed the saving. Users can also scan their patch for slips manually by invocating the item from the patch list menu. The value of the preference #checkForSlips is still honored for the automatic detection, and the description of that preference is updated to mention the new usage.
=============== Postscript ===============
"Postscript:"
(Preferences preferenceAt: #checkForSlips) helpString: 'If true, then whenever you file out a change set or save a Monticello version, it is checked for ''slips'' and if any are found, you are so informed and given a chance to open a browser on them.'.
=============== Diff ===============
CompiledMethod>>hasReportableSlip {testing} · ct 1/15/2022 22:32 (changed) hasReportableSlip "Answer whether the receiver contains anything that should be brought to the attention of the author when filing out. Customize the lists here to suit your preferences. If slips do not get reported in spite of your best efforts here, make certain that the Preference 'checkForSlips' is set to true." - #(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: ) + #(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: #haltIfNil #haltOnCount: #halt:onCount: #break ) do: [:aLit | (self hasLiteral: aLit) ifTrue: [^ true]]. #(#Transcript #AA #BB #CC #DD #EE ) do: [:aSymbol | | assoc | (assoc := Smalltalk globals associationAt: aSymbol ifAbsent: []) ifNotNil: [(self hasLiteral: assoc) ifTrue: [^ true]]]. ^ false
MCChangeSelector>>select {actions} · ct 1/15/2022 22:46 (changed) select + + (Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self]. + self answer: (MCPatch operations: kept)
MCDefinition>>hasReportableSlip {testing} · ct 1/15/2022 22:49 + hasReportableSlip + + ^ false
MCMethodDefinition (changed) MCDefinition subclass: #MCMethodDefinition - instanceVariableNames: 'classIsMeta source category selector className timeStamp' + instanceVariableNames: 'classIsMeta source category selector className timeStamp hasReportableSlip' classVariableNames: '' poolDictionaries: '' category: 'Monticello-Modeling'
MCMethodDefinition class instanceVariableNames: 'definitions'
""
MCMethodDefinition class>>forMethodReference: {create} · ct 1/15/2022 22:53 (changed) forMethodReference: aMethodReference | definition | definition := self cachedDefinitions at: aMethodReference compiledMethod ifAbsent: []. (definition isNil or: [definition selector ~= aMethodReference methodSymbol or: [definition className ~= aMethodReference classSymbol or: [definition classIsMeta ~= aMethodReference classIsMeta or: [definition category ~= aMethodReference category]]]]) ifTrue: [definition := self className: aMethodReference classSymbol classIsMeta: aMethodReference classIsMeta selector: aMethodReference methodSymbol category: aMethodReference category timeStamp: aMethodReference timeStamp source: aMethodReference source. + definition hasReportableSlip: aMethodReference compiledMethod hasReportableSlip. self cachedDefinitions at: aMethodReference compiledMethod put: definition]. ^ definition
MCMethodDefinition>>hasReportableSlip {accessing} · ct 1/15/2022 22:54 + hasReportableSlip + + ^ hasReportableSlip ifNil: [false]
MCMethodDefinition>>hasReportableSlip: {accessing} · ct 1/15/2022 22:53 + hasReportableSlip: aBoolean + + hasReportableSlip := aBoolean.
MCPatchBrowser>>checkForSlips {actions} · ct 1/15/2022 23:12 + checkForSlips + + ^ self checkForSlipsIn: self items
MCPatchBrowser>>checkForSlipsIn: {actions} · ct 1/15/2022 23:07 + checkForSlipsIn: patchItems + + ^ patchItems select: [:ea | ea hasReportableSlip]
MCPatchBrowser>>lookForSlips {actions} · ct 1/15/2022 23:11 + lookForSlips + "Look for slips in all changes. If there are any, ask the user how to treat them. Answer true if the operation should go on, or false if it should be canceled." + + | slips | + slips := self checkForSlips ifEmpty: [^ true]. + (Project uiManager + chooseFromLabeledValues: (OrderedDictionary new + at: 'Browse slips' translated put: [self systemNavigation + browseMessageList: (slips collect: [:ea | ea definition asMethodReference]) + name: 'Possible slips in new version' translated]; + at: 'Ignore slips and continue' translated put: [^ true]; + at: 'Cancel' translated put: [^ false]; + yourself) + title: ('This patch contains {1} halts, Transcript sends, or other possible slips.\Save the new version anyway?' withCRs translated format: {slips size})) value. + ^ false
MCPatchBrowser>>methodListMenu: {menus} · ct 1/15/2022 23:01 + methodListMenu: aMenu + + super methodListMenu: aMenu. + + aMenu add: 'check for slips' translated help: 'Check all changes for possible slips such as forgotten breakpoints or logging statements.' translated action: #reportSlips. + + ^ aMenu
MCPatchBrowser>>reportSlips {actions} · ct 1/15/2022 23:05 + reportSlips + "Look for slips in all changes and report to the user what was found." + + | slips | + slips := self checkForSlips ifEmpty: [^ self inform: 'There were no obvious slips in this patch' translated]. + self systemNavigation + browseMessageList: (slips collect: [:ea | ea definition asMethodReference]) + name: 'Possible slips in new version' translated.
MCPatchOperation>>hasReportableSlip {testing} · ct 1/15/2022 22:49 + hasReportableSlip + + ^ self targetDefinition + ifNil: [false] + ifNotNil: [:def | def hasReportableSlip]
MCSaveVersionDialog>>accept {actions} · ct 1/15/2022 23:06 (changed) accept | logMessage logMessageWidget | self updateItems. + + (Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self]. + logMessage := (logMessageWidget := self findTextMorph: #logMessage) text asString. (logMessage isEmpty or: [logMessage beginsWith: 'empty log message']) ifTrue: - [(UIManager confirm: 'the log message is empty; are you sure you want to commit') ifFalse: [^ self]] + [(self confirm: 'The log message is empty; are you sure you want to commit?' translated) ifFalse: [^ self]] ifFalse: [logMessageWidget accept]. + self answer: { (self findTextMorph: #versionName) text asString. logMessage. ignore }
MCSaveVersionDialog>>checkForSlips {actions} · ct 1/15/2022 23:12 + checkForSlips + + ^ super checkForSlipsIn: (self items copyWithoutAll: self ignore)
Preference>>helpString: {menu} · ct 1/16/2022 01:31 + helpString: aString + + helpString := aString
--- Sent from Squeak Inbox Talk ["mcSlips.1.cs"] ["mcSlips.png"]
I have no opinion one way or another and I cannot review it now, but use your judgment here. If you think that there is a risk of unintended side effects, then wait until after the release. Otherwise go ahead. But there is no harm in waiting a few weeks so I would advise waiting if you are not sure.
Dave
On Sun, Jan 16, 2022 at 01:40:39AM +0100, christoph.thiede@student.hpi.uni-potsdam.de wrote:
Any fundamental objections before I would merge this into the Trunk? :-)
=============== Summary ===============
Change Set: mcSlips Date: 16 January 2022 Author: Christoph Thiede
This changeset integrates the slip detection mechanism, previously only known from the domain of changesets, into the Monticello system. When users save a new version, it will automatically notify them about any possible slips and let them decide whether to browse them first or proceed the saving. Users can also scan their patch for slips manually by invocating the item from the patch list menu. The value of the preference #checkForSlips is still honored for the automatic detection, and the description of that preference is updated to mention the new usage.
=============== Postscript ===============
"Postscript:"
(Preferences preferenceAt: #checkForSlips) helpString: 'If true, then whenever you file out a change set or save a Monticello version, it is checked for ''slips'' and if any are found, you are so informed and given a chance to open a browser on them.'.
=============== Diff ===============
CompiledMethod>>hasReportableSlip {testing} ? ct 1/15/2022 22:32 (changed) hasReportableSlip "Answer whether the receiver contains anything that should be brought to the attention of the author when filing out. Customize the lists here to suit your preferences. If slips do not get reported in spite of your best efforts here, make certain that the Preference 'checkForSlips' is set to true."
- #(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: )
- #(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: #haltIfNil #haltOnCount: #halt:onCount: #break ) do: [:aLit | (self hasLiteral: aLit) ifTrue: [^ true]]. #(#Transcript #AA #BB #CC #DD #EE ) do: [:aSymbol | | assoc | (assoc := Smalltalk globals associationAt: aSymbol ifAbsent: []) ifNotNil: [(self hasLiteral: assoc) ifTrue: [^ true]]]. ^ false
MCChangeSelector>>select {actions} ? ct 1/15/2022 22:46 (changed) select
- (Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- self answer: (MCPatch operations: kept)
MCDefinition>>hasReportableSlip {testing} ? ct 1/15/2022 22:49
- hasReportableSlip
- ^ false
MCMethodDefinition (changed) MCDefinition subclass: #MCMethodDefinition
- instanceVariableNames: 'classIsMeta source category selector className timeStamp'
- instanceVariableNames: 'classIsMeta source category selector className timeStamp hasReportableSlip' classVariableNames: '' poolDictionaries: '' category: 'Monticello-Modeling'
MCMethodDefinition class instanceVariableNames: 'definitions'
""
MCMethodDefinition class>>forMethodReference: {create} ? ct 1/15/2022 22:53 (changed) forMethodReference: aMethodReference | definition | definition := self cachedDefinitions at: aMethodReference compiledMethod ifAbsent: []. (definition isNil or: [definition selector ~= aMethodReference methodSymbol or: [definition className ~= aMethodReference classSymbol or: [definition classIsMeta ~= aMethodReference classIsMeta or: [definition category ~= aMethodReference category]]]]) ifTrue: [definition := self className: aMethodReference classSymbol classIsMeta: aMethodReference classIsMeta selector: aMethodReference methodSymbol category: aMethodReference category timeStamp: aMethodReference timeStamp source: aMethodReference source.
^ definitiondefinition hasReportableSlip: aMethodReference compiledMethod hasReportableSlip. self cachedDefinitions at: aMethodReference compiledMethod put: definition].
MCMethodDefinition>>hasReportableSlip {accessing} ? ct 1/15/2022 22:54
- hasReportableSlip
- ^ hasReportableSlip ifNil: [false]
MCMethodDefinition>>hasReportableSlip: {accessing} ? ct 1/15/2022 22:53
- hasReportableSlip: aBoolean
- hasReportableSlip := aBoolean.
MCPatchBrowser>>checkForSlips {actions} ? ct 1/15/2022 23:12
- checkForSlips
- ^ self checkForSlipsIn: self items
MCPatchBrowser>>checkForSlipsIn: {actions} ? ct 1/15/2022 23:07
- checkForSlipsIn: patchItems
- ^ patchItems select: [:ea | ea hasReportableSlip]
MCPatchBrowser>>lookForSlips {actions} ? ct 1/15/2022 23:11
- lookForSlips
- "Look for slips in all changes. If there are any, ask the user how to treat them. Answer true if the operation should go on, or false if it should be canceled."
- | slips |
- slips := self checkForSlips ifEmpty: [^ true].
- (Project uiManager
chooseFromLabeledValues: (OrderedDictionary new
at: 'Browse slips' translated put: [self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated];
at: 'Ignore slips and continue' translated put: [^ true];
at: 'Cancel' translated put: [^ false];
yourself)
title: ('This patch contains {1} halts, Transcript sends, or other possible slips.\\Save the new version anyway?' withCRs translated format: {slips size})) value.
- ^ false
MCPatchBrowser>>methodListMenu: {menus} ? ct 1/15/2022 23:01
- methodListMenu: aMenu
- super methodListMenu: aMenu.
- aMenu add: 'check for slips' translated help: 'Check all changes for possible slips such as forgotten breakpoints or logging statements.' translated action: #reportSlips.
- ^ aMenu
MCPatchBrowser>>reportSlips {actions} ? ct 1/15/2022 23:05
- reportSlips
- "Look for slips in all changes and report to the user what was found."
- | slips |
- slips := self checkForSlips ifEmpty: [^ self inform: 'There were no obvious slips in this patch' translated].
- self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated.
MCPatchOperation>>hasReportableSlip {testing} ? ct 1/15/2022 22:49
- hasReportableSlip
- ^ self targetDefinition
ifNil: [false]
ifNotNil: [:def | def hasReportableSlip]
MCSaveVersionDialog>>accept {actions} ? ct 1/15/2022 23:06 (changed) accept | logMessage logMessageWidget | self updateItems.
- (Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- logMessage := (logMessageWidget := self findTextMorph: #logMessage) text asString. (logMessage isEmpty or: [logMessage beginsWith: 'empty log message']) ifTrue:
[(UIManager confirm: 'the log message is empty; are you sure you want to commit') ifFalse: [^ self]]
ifFalse: [logMessageWidget accept].[(self confirm: 'The log message is empty; are you sure you want to commit?' translated) ifFalse: [^ self]]
- self answer: { (self findTextMorph: #versionName) text asString. logMessage. ignore }
MCSaveVersionDialog>>checkForSlips {actions} ? ct 1/15/2022 23:12
- checkForSlips
- ^ super checkForSlipsIn: (self items copyWithoutAll: self ignore)
Preference>>helpString: {menu} ? ct 1/16/2022 01:31
- helpString: aString
- helpString := aString
Sent from Squeak Inbox Talk ["mcSlips.1.cs"] ["mcSlips.png"]
Hi Christoph,
I like the idea of enabling users to check for slips, what do you think about adding that command (either via button or menu item) somewhere in both the Monticello AND ChangeSet windows.
The reason being, this function is no good implemented as a global "mode" that the user would certainly wish to vary depending on which package they were saving, leading to cases where it starts checking a big package like Morphic before remembering what the switch was set to and didn't necessarily want it to, forcing you to decide whether to wait or risk aborting. Or, the opposite, you wanted to check slips first, but forgot you turned it off, assumed all is fine.
We add preferences all the time, moving this to JIT activation buttons for the user could afford us the opportunity to trim one back for once (including its associated methods / pragmas, etc), while improving the UX.
So, -1 on further buttressing the existence of this function implemented as a global mode and Preference.
Best, Chris
On Sat, Jan 15, 2022 at 6:41 PM christoph.thiede@student.hpi.uni-potsdam.de wrote:
Any fundamental objections before I would merge this into the Trunk? :-)
=============== Summary ===============
Change Set: mcSlips Date: 16 January 2022 Author: Christoph Thiede
This changeset integrates the slip detection mechanism, previously only known from the domain of changesets, into the Monticello system. When users save a new version, it will automatically notify them about any possible slips and let them decide whether to browse them first or proceed the saving. Users can also scan their patch for slips manually by invocating the item from the patch list menu. The value of the preference #checkForSlips is still honored for the automatic detection, and the description of that preference is updated to mention the new usage.
=============== Postscript ===============
"Postscript:"
(Preferences preferenceAt: #checkForSlips) helpString: 'If true, then whenever you file out a change set or save a Monticello version, it is checked for ''slips'' and if any are found, you are so informed and given a chance to open a browser on them.'.
=============== Diff ===============
CompiledMethod>>hasReportableSlip {testing} · ct 1/15/2022 22:32 (changed) hasReportableSlip "Answer whether the receiver contains anything that should be brought to the attention of the author when filing out. Customize the lists here to suit your preferences. If slips do not get reported in spite of your best efforts here, make certain that the Preference 'checkForSlips' is set to true."
#(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: )
#(#Transcript #AA #BB #CC #DD #EE ) do: [:aSymbol | | assoc | (assoc := Smalltalk globals associationAt: aSymbol ifAbsent: []) ifNotNil: [(self hasLiteral: assoc) ifTrue: [^ true]]]. ^ false#(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: #haltIfNil #haltOnCount: #halt:onCount: #break ) do: [:aLit | (self hasLiteral: aLit) ifTrue: [^ true]].
MCChangeSelector>>select {actions} · ct 1/15/2022 22:46 (changed) select
(Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- self answer: (MCPatch operations: kept)
MCDefinition>>hasReportableSlip {testing} · ct 1/15/2022 22:49
- hasReportableSlip
^ false
MCMethodDefinition (changed) MCDefinition subclass: #MCMethodDefinition
instanceVariableNames: 'classIsMeta source category selector className timeStamp'
classVariableNames: '' poolDictionaries: '' category: 'Monticello-Modeling'instanceVariableNames: 'classIsMeta source category selector className timeStamp hasReportableSlip'
MCMethodDefinition class instanceVariableNames: 'definitions'
""
MCMethodDefinition class>>forMethodReference: {create} · ct 1/15/2022 22:53 (changed) forMethodReference: aMethodReference | definition | definition := self cachedDefinitions at: aMethodReference compiledMethod ifAbsent: []. (definition isNil or: [definition selector ~= aMethodReference methodSymbol or: [definition className ~= aMethodReference classSymbol or: [definition classIsMeta ~= aMethodReference classIsMeta or: [definition category ~= aMethodReference category]]]]) ifTrue: [definition := self className: aMethodReference classSymbol classIsMeta: aMethodReference classIsMeta selector: aMethodReference methodSymbol category: aMethodReference category timeStamp: aMethodReference timeStamp source: aMethodReference source.
^ definitiondefinition hasReportableSlip: aMethodReference compiledMethod hasReportableSlip. self cachedDefinitions at: aMethodReference compiledMethod put: definition].
MCMethodDefinition>>hasReportableSlip {accessing} · ct 1/15/2022 22:54
- hasReportableSlip
^ hasReportableSlip ifNil: [false]
MCMethodDefinition>>hasReportableSlip: {accessing} · ct 1/15/2022 22:53
- hasReportableSlip: aBoolean
hasReportableSlip := aBoolean.
MCPatchBrowser>>checkForSlips {actions} · ct 1/15/2022 23:12
- checkForSlips
^ self checkForSlipsIn: self items
MCPatchBrowser>>checkForSlipsIn: {actions} · ct 1/15/2022 23:07
- checkForSlipsIn: patchItems
^ patchItems select: [:ea | ea hasReportableSlip]
MCPatchBrowser>>lookForSlips {actions} · ct 1/15/2022 23:11
- lookForSlips
"Look for slips in all changes. If there are any, ask the user how to treat them. Answer true if the operation should go on, or false if it should be canceled."
| slips |
slips := self checkForSlips ifEmpty: [^ true].
(Project uiManager
chooseFromLabeledValues: (OrderedDictionary new
at: 'Browse slips' translated put: [self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated];
at: 'Ignore slips and continue' translated put: [^ true];
at: 'Cancel' translated put: [^ false];
yourself)
title: ('This patch contains {1} halts, Transcript sends, or other possible slips.\\Save the new version anyway?' withCRs translated format: {slips size})) value.
^ false
MCPatchBrowser>>methodListMenu: {menus} · ct 1/15/2022 23:01
- methodListMenu: aMenu
super methodListMenu: aMenu.
aMenu add: 'check for slips' translated help: 'Check all changes for possible slips such as forgotten breakpoints or logging statements.' translated action: #reportSlips.
^ aMenu
MCPatchBrowser>>reportSlips {actions} · ct 1/15/2022 23:05
- reportSlips
"Look for slips in all changes and report to the user what was found."
| slips |
slips := self checkForSlips ifEmpty: [^ self inform: 'There were no obvious slips in this patch' translated].
self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated.
MCPatchOperation>>hasReportableSlip {testing} · ct 1/15/2022 22:49
- hasReportableSlip
^ self targetDefinition
ifNil: [false]
ifNotNil: [:def | def hasReportableSlip]
MCSaveVersionDialog>>accept {actions} · ct 1/15/2022 23:06 (changed) accept | logMessage logMessageWidget | self updateItems.
(Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- logMessage := (logMessageWidget := self findTextMorph: #logMessage) text asString. (logMessage isEmpty or: [logMessage beginsWith: 'empty log message']) ifTrue:
[(UIManager confirm: 'the log message is empty; are you sure you want to commit') ifFalse: [^ self]]
[(self confirm: 'The log message is empty; are you sure you want to commit?' translated) ifFalse: [^ self]] ifFalse: [logMessageWidget accept].
- self answer: { (self findTextMorph: #versionName) text asString. logMessage. ignore }
MCSaveVersionDialog>>checkForSlips {actions} · ct 1/15/2022 23:12
- checkForSlips
^ super checkForSlipsIn: (self items copyWithoutAll: self ignore)
Preference>>helpString: {menu} · ct 1/16/2022 01:31
- helpString: aString
helpString := aString
Sent from Squeak Inbox Talk ["mcSlips.1.cs"] ["mcSlips.png"]
Christoph,
BTW, your font is really large in your emails. Are you doing this because you're using a high-dpi screen? :) Are you open to increasing your local font size instead? Your emails are hard to read.
Regards, Chris
On Sat, Jan 15, 2022 at 6:41 PM christoph.thiede@student.hpi.uni-potsdam.de wrote:
Any fundamental objections before I would merge this into the Trunk? :-)
=============== Summary ===============
Change Set: mcSlips Date: 16 January 2022 Author: Christoph Thiede
This changeset integrates the slip detection mechanism, previously only known from the domain of changesets, into the Monticello system. When users save a new version, it will automatically notify them about any possible slips and let them decide whether to browse them first or proceed the saving. Users can also scan their patch for slips manually by invocating the item from the patch list menu. The value of the preference #checkForSlips is still honored for the automatic detection, and the description of that preference is updated to mention the new usage.
=============== Postscript ===============
"Postscript:"
(Preferences preferenceAt: #checkForSlips) helpString: 'If true, then whenever you file out a change set or save a Monticello version, it is checked for ''slips'' and if any are found, you are so informed and given a chance to open a browser on them.'.
=============== Diff ===============
CompiledMethod>>hasReportableSlip {testing} · ct 1/15/2022 22:32 (changed) hasReportableSlip "Answer whether the receiver contains anything that should be brought to the attention of the author when filing out. Customize the lists here to suit your preferences. If slips do not get reported in spite of your best efforts here, make certain that the Preference 'checkForSlips' is set to true."
#(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: )
#(#Transcript #AA #BB #CC #DD #EE ) do: [:aSymbol | | assoc | (assoc := Smalltalk globals associationAt: aSymbol ifAbsent: []) ifNotNil: [(self hasLiteral: assoc) ifTrue: [^ true]]]. ^ false#(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: #haltIfNil #haltOnCount: #halt:onCount: #break ) do: [:aLit | (self hasLiteral: aLit) ifTrue: [^ true]].
MCChangeSelector>>select {actions} · ct 1/15/2022 22:46 (changed) select
(Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- self answer: (MCPatch operations: kept)
MCDefinition>>hasReportableSlip {testing} · ct 1/15/2022 22:49
- hasReportableSlip
^ false
MCMethodDefinition (changed) MCDefinition subclass: #MCMethodDefinition
instanceVariableNames: 'classIsMeta source category selector className timeStamp'
classVariableNames: '' poolDictionaries: '' category: 'Monticello-Modeling'instanceVariableNames: 'classIsMeta source category selector className timeStamp hasReportableSlip'
MCMethodDefinition class instanceVariableNames: 'definitions'
""
MCMethodDefinition class>>forMethodReference: {create} · ct 1/15/2022 22:53 (changed) forMethodReference: aMethodReference | definition | definition := self cachedDefinitions at: aMethodReference compiledMethod ifAbsent: []. (definition isNil or: [definition selector ~= aMethodReference methodSymbol or: [definition className ~= aMethodReference classSymbol or: [definition classIsMeta ~= aMethodReference classIsMeta or: [definition category ~= aMethodReference category]]]]) ifTrue: [definition := self className: aMethodReference classSymbol classIsMeta: aMethodReference classIsMeta selector: aMethodReference methodSymbol category: aMethodReference category timeStamp: aMethodReference timeStamp source: aMethodReference source.
^ definitiondefinition hasReportableSlip: aMethodReference compiledMethod hasReportableSlip. self cachedDefinitions at: aMethodReference compiledMethod put: definition].
MCMethodDefinition>>hasReportableSlip {accessing} · ct 1/15/2022 22:54
- hasReportableSlip
^ hasReportableSlip ifNil: [false]
MCMethodDefinition>>hasReportableSlip: {accessing} · ct 1/15/2022 22:53
- hasReportableSlip: aBoolean
hasReportableSlip := aBoolean.
MCPatchBrowser>>checkForSlips {actions} · ct 1/15/2022 23:12
- checkForSlips
^ self checkForSlipsIn: self items
MCPatchBrowser>>checkForSlipsIn: {actions} · ct 1/15/2022 23:07
- checkForSlipsIn: patchItems
^ patchItems select: [:ea | ea hasReportableSlip]
MCPatchBrowser>>lookForSlips {actions} · ct 1/15/2022 23:11
- lookForSlips
"Look for slips in all changes. If there are any, ask the user how to treat them. Answer true if the operation should go on, or false if it should be canceled."
| slips |
slips := self checkForSlips ifEmpty: [^ true].
(Project uiManager
chooseFromLabeledValues: (OrderedDictionary new
at: 'Browse slips' translated put: [self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated];
at: 'Ignore slips and continue' translated put: [^ true];
at: 'Cancel' translated put: [^ false];
yourself)
title: ('This patch contains {1} halts, Transcript sends, or other possible slips.\\Save the new version anyway?' withCRs translated format: {slips size})) value.
^ false
MCPatchBrowser>>methodListMenu: {menus} · ct 1/15/2022 23:01
- methodListMenu: aMenu
super methodListMenu: aMenu.
aMenu add: 'check for slips' translated help: 'Check all changes for possible slips such as forgotten breakpoints or logging statements.' translated action: #reportSlips.
^ aMenu
MCPatchBrowser>>reportSlips {actions} · ct 1/15/2022 23:05
- reportSlips
"Look for slips in all changes and report to the user what was found."
| slips |
slips := self checkForSlips ifEmpty: [^ self inform: 'There were no obvious slips in this patch' translated].
self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated.
MCPatchOperation>>hasReportableSlip {testing} · ct 1/15/2022 22:49
- hasReportableSlip
^ self targetDefinition
ifNil: [false]
ifNotNil: [:def | def hasReportableSlip]
MCSaveVersionDialog>>accept {actions} · ct 1/15/2022 23:06 (changed) accept | logMessage logMessageWidget | self updateItems.
(Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- logMessage := (logMessageWidget := self findTextMorph: #logMessage) text asString. (logMessage isEmpty or: [logMessage beginsWith: 'empty log message']) ifTrue:
[(UIManager confirm: 'the log message is empty; are you sure you want to commit') ifFalse: [^ self]]
[(self confirm: 'The log message is empty; are you sure you want to commit?' translated) ifFalse: [^ self]] ifFalse: [logMessageWidget accept].
- self answer: { (self findTextMorph: #versionName) text asString. logMessage. ignore }
MCSaveVersionDialog>>checkForSlips {actions} · ct 1/15/2022 23:12
- checkForSlips
^ super checkForSlipsIn: (self items copyWithoutAll: self ignore)
Preference>>helpString: {menu} · ct 1/16/2022 01:31
- helpString: aString
helpString := aString
Sent from Squeak Inbox Talk ["mcSlips.1.cs"] ["mcSlips.png"]
Hi Dave, hi all,
thank you for the feedback! :-)
If you think that there is a risk of unintended side effects, then wait until after the release. Otherwise go ahead. But there is no harm in waiting a few weeks so I would advise waiting if you are not sure.
Right now I cannot think of any possible unintended consequences, but this also applied to my most recent mistakes, so I guess I will wait for some more time. I have no hurry, just wanted to avoid further incidents such as Monticello-ct.767. :-) I should focus more on contributing to the preparation of the next release than implementing new features anyway. :D
I like the idea of enabling users to check for slips, what do you think about adding that command (either via button or menu item) somewhere in both the Monticello AND ChangeSet windows.
It's all there: :-) ["changesorter-check for slips.png"] And with this patch also: ["mcSlips-menu.png"]
Regarding the preference, and, if I understood your point correctly, the performance aspect:
The menu items are available independently of the preference value. In the case of my proposed Monticello integration, the slip detection is run eagerly, independently of the preference value, as soon as you compile a method (Monticello assembles the working copy eagerly). For the change sorter, slips are collected on demand only, but even in a very large changeset, this happens without a noticeable delay for me. Have you made different experiences with this in the past?
Best, Christoph
--- Sent from Squeak Inbox Talk
On 2022-01-15T20:27:58-06:00, asqueaker@gmail.com wrote:
Christoph,
BTW, your font is really large in your emails. Are you doing this because you're using a high-dpi screen? :) Are you open to increasing your local font size instead? Your emails are hard to read.
Regards, Chris
On Sat, Jan 15, 2022 at 6:41 PM <christoph.thiede at student.hpi.uni-potsdam.de> wrote:
Any fundamental objections before I would merge this into the Trunk? :-)
=============== Summary ===============
Change Set: mcSlips Date: 16 January 2022 Author: Christoph Thiede
This changeset integrates the slip detection mechanism, previously only known from the domain of changesets, into the Monticello system. When users save a new version, it will automatically notify them about any possible slips and let them decide whether to browse them first or proceed the saving. Users can also scan their patch for slips manually by invocating the item from the patch list menu. The value of the preference #checkForSlips is still honored for the automatic detection, and the description of that preference is updated to mention the new usage.
=============== Postscript ===============
"Postscript:"
(Preferences preferenceAt: #checkForSlips) helpString: 'If true, then whenever you file out a change set or save a Monticello version, it is checked for ''slips'' and if any are found, you are so informed and given a chance to open a browser on them.'.
=============== Diff ===============
CompiledMethod>>hasReportableSlip {testing} · ct 1/15/2022 22:32 (changed) hasReportableSlip "Answer whether the receiver contains anything that should be brought to the attention of the author when filing out. Customize the lists here to suit your preferences. If slips do not get reported in spite of your best efforts here, make certain that the Preference 'checkForSlips' is set to true."
#(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: )
#(#Transcript #AA #BB #CC #DD #EE ) do: [:aSymbol | | assoc | (assoc := Smalltalk globals associationAt: aSymbol ifAbsent: []) ifNotNil: [(self hasLiteral: assoc) ifTrue: [^ true]]]. ^ false#(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: #haltIfNil #haltOnCount: #halt:onCount: #break ) do: [:aLit | (self hasLiteral: aLit) ifTrue: [^ true]].
MCChangeSelector>>select {actions} · ct 1/15/2022 22:46 (changed) select
(Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- self answer: (MCPatch operations: kept)
MCDefinition>>hasReportableSlip {testing} · ct 1/15/2022 22:49
- hasReportableSlip
^ false
MCMethodDefinition (changed) MCDefinition subclass: #MCMethodDefinition
instanceVariableNames: 'classIsMeta source category selector className timeStamp'
classVariableNames: '' poolDictionaries: '' category: 'Monticello-Modeling'instanceVariableNames: 'classIsMeta source category selector className timeStamp hasReportableSlip'
MCMethodDefinition class instanceVariableNames: 'definitions'
""
MCMethodDefinition class>>forMethodReference: {create} · ct 1/15/2022 22:53 (changed) forMethodReference: aMethodReference | definition | definition := self cachedDefinitions at: aMethodReference compiledMethod ifAbsent: []. (definition isNil or: [definition selector ~= aMethodReference methodSymbol or: [definition className ~= aMethodReference classSymbol or: [definition classIsMeta ~= aMethodReference classIsMeta or: [definition category ~= aMethodReference category]]]]) ifTrue: [definition := self className: aMethodReference classSymbol classIsMeta: aMethodReference classIsMeta selector: aMethodReference methodSymbol category: aMethodReference category timeStamp: aMethodReference timeStamp source: aMethodReference source.
^ definitiondefinition hasReportableSlip: aMethodReference compiledMethod hasReportableSlip. self cachedDefinitions at: aMethodReference compiledMethod put: definition].
MCMethodDefinition>>hasReportableSlip {accessing} · ct 1/15/2022 22:54
- hasReportableSlip
^ hasReportableSlip ifNil: [false]
MCMethodDefinition>>hasReportableSlip: {accessing} · ct 1/15/2022 22:53
- hasReportableSlip: aBoolean
hasReportableSlip := aBoolean.
MCPatchBrowser>>checkForSlips {actions} · ct 1/15/2022 23:12
- checkForSlips
^ self checkForSlipsIn: self items
MCPatchBrowser>>checkForSlipsIn: {actions} · ct 1/15/2022 23:07
- checkForSlipsIn: patchItems
^ patchItems select: [:ea | ea hasReportableSlip]
MCPatchBrowser>>lookForSlips {actions} · ct 1/15/2022 23:11
- lookForSlips
"Look for slips in all changes. If there are any, ask the user how to treat them. Answer true if the operation should go on, or false if it should be canceled."
| slips |
slips := self checkForSlips ifEmpty: [^ true].
(Project uiManager
chooseFromLabeledValues: (OrderedDictionary new
at: 'Browse slips' translated put: [self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated];
at: 'Ignore slips and continue' translated put: [^ true];
at: 'Cancel' translated put: [^ false];
yourself)
title: ('This patch contains {1} halts, Transcript sends, or other possible slips.\\Save the new version anyway?' withCRs translated format: {slips size})) value.
^ false
MCPatchBrowser>>methodListMenu: {menus} · ct 1/15/2022 23:01
- methodListMenu: aMenu
super methodListMenu: aMenu.
aMenu add: 'check for slips' translated help: 'Check all changes for possible slips such as forgotten breakpoints or logging statements.' translated action: #reportSlips.
^ aMenu
MCPatchBrowser>>reportSlips {actions} · ct 1/15/2022 23:05
- reportSlips
"Look for slips in all changes and report to the user what was found."
| slips |
slips := self checkForSlips ifEmpty: [^ self inform: 'There were no obvious slips in this patch' translated].
self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated.
MCPatchOperation>>hasReportableSlip {testing} · ct 1/15/2022 22:49
- hasReportableSlip
^ self targetDefinition
ifNil: [false]
ifNotNil: [:def | def hasReportableSlip]
MCSaveVersionDialog>>accept {actions} · ct 1/15/2022 23:06 (changed) accept | logMessage logMessageWidget | self updateItems.
(Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- logMessage := (logMessageWidget := self findTextMorph: #logMessage) text asString. (logMessage isEmpty or: [logMessage beginsWith: 'empty log message']) ifTrue:
[(UIManager confirm: 'the log message is empty; are you sure you want to commit') ifFalse: [^ self]]
[(self confirm: 'The log message is empty; are you sure you want to commit?' translated) ifFalse: [^ self]] ifFalse: [logMessageWidget accept].
- self answer: { (self findTextMorph: #versionName) text asString. logMessage. ignore }
MCSaveVersionDialog>>checkForSlips {actions} · ct 1/15/2022 23:12
- checkForSlips
^ super checkForSlipsIn: (self items copyWithoutAll: self ignore)
Preference>>helpString: {menu} · ct 1/16/2022 01:31
- helpString: aString
helpString := aString
Sent from Squeak Inbox Talk ["mcSlips.1.cs"] ["mcSlips.png"]
Hi Chris,
friendly reminder of this conversation. :-) Are your concerns still up to date regarding the performance and user experience of adding the existing slip detection from changeset fileouts to Monticello version savings? Would you accept this proposal if I turned off the slip detection unless the preference is enabled?
Best, Christoph
On 2022-01-16T19:11:11+01:00, christoph.thiede@student.hpi.uni-potsdam.de wrote:
Hi Dave, hi all,
thank you for the feedback! :-)
If you think that there is a risk of unintended side effects, then wait until after the release. Otherwise go ahead. But there is no harm in waiting a few weeks so I would advise waiting if you are not sure.
Right now I cannot think of any possible unintended consequences, but this also applied to my most recent mistakes, so I guess I will wait for some more time. I have no hurry, just wanted to avoid further incidents such as Monticello-ct.767. :-) I should focus more on contributing to the preparation of the next release than implementing new features anyway. :D
I like the idea of enabling users to check for slips, what do you think about adding that command (either via button or menu item) somewhere in both the Monticello AND ChangeSet windows.
It's all there: :-) ["changesorter-check for slips.png"] And with this patch also: ["mcSlips-menu.png"]
Regarding the preference, and, if I understood your point correctly, the performance aspect:
The menu items are available independently of the preference value. In the case of my proposed Monticello integration, the slip detection is run eagerly, independently of the preference value, as soon as you compile a method (Monticello assembles the working copy eagerly). For the change sorter, slips are collected on demand only, but even in a very large changeset, this happens without a noticeable delay for me. Have you made different experiences with this in the past?
Best, Christoph
Sent from Squeak Inbox Talk
On 2022-01-15T20:27:58-06:00, asqueaker(a)gmail.com wrote:
Christoph,
BTW, your font is really large in your emails. Are you doing this because you're using a high-dpi screen? :) Are you open to increasing your local font size instead? Your emails are hard to read.
Regards, Chris
On Sat, Jan 15, 2022 at 6:41 PM <christoph.thiede at student.hpi.uni-potsdam.de> wrote:
Any fundamental objections before I would merge this into the Trunk? :-)
=============== Summary ===============
Change Set: mcSlips Date: 16 January 2022 Author: Christoph Thiede
This changeset integrates the slip detection mechanism, previously only known from the domain of changesets, into the Monticello system. When users save a new version, it will automatically notify them about any possible slips and let them decide whether to browse them first or proceed the saving. Users can also scan their patch for slips manually by invocating the item from the patch list menu. The value of the preference #checkForSlips is still honored for the automatic detection, and the description of that preference is updated to mention the new usage.
=============== Postscript ===============
"Postscript:"
(Preferences preferenceAt: #checkForSlips) helpString: 'If true, then whenever you file out a change set or save a Monticello version, it is checked for ''slips'' and if any are found, you are so informed and given a chance to open a browser on them.'.
=============== Diff ===============
CompiledMethod>>hasReportableSlip {testing} · ct 1/15/2022 22:32 (changed) hasReportableSlip "Answer whether the receiver contains anything that should be brought to the attention of the author when filing out. Customize the lists here to suit your preferences. If slips do not get reported in spite of your best efforts here, make certain that the Preference 'checkForSlips' is set to true."
#(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: )
#(#Transcript #AA #BB #CC #DD #EE ) do: [:aSymbol | | assoc | (assoc := Smalltalk globals associationAt: aSymbol ifAbsent: []) ifNotNil: [(self hasLiteral: assoc) ifTrue: [^ true]]]. ^ false#(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: #haltIfNil #haltOnCount: #halt:onCount: #break ) do: [:aLit | (self hasLiteral: aLit) ifTrue: [^ true]].
MCChangeSelector>>select {actions} · ct 1/15/2022 22:46 (changed) select
(Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- self answer: (MCPatch operations: kept)
MCDefinition>>hasReportableSlip {testing} · ct 1/15/2022 22:49
- hasReportableSlip
^ false
MCMethodDefinition (changed) MCDefinition subclass: #MCMethodDefinition
instanceVariableNames: 'classIsMeta source category selector className timeStamp'
classVariableNames: '' poolDictionaries: '' category: 'Monticello-Modeling'instanceVariableNames: 'classIsMeta source category selector className timeStamp hasReportableSlip'
MCMethodDefinition class instanceVariableNames: 'definitions'
""
MCMethodDefinition class>>forMethodReference: {create} · ct 1/15/2022 22:53 (changed) forMethodReference: aMethodReference | definition | definition := self cachedDefinitions at: aMethodReference compiledMethod ifAbsent: []. (definition isNil or: [definition selector ~= aMethodReference methodSymbol or: [definition className ~= aMethodReference classSymbol or: [definition classIsMeta ~= aMethodReference classIsMeta or: [definition category ~= aMethodReference category]]]]) ifTrue: [definition := self className: aMethodReference classSymbol classIsMeta: aMethodReference classIsMeta selector: aMethodReference methodSymbol category: aMethodReference category timeStamp: aMethodReference timeStamp source: aMethodReference source.
^ definitiondefinition hasReportableSlip: aMethodReference compiledMethod hasReportableSlip. self cachedDefinitions at: aMethodReference compiledMethod put: definition].
MCMethodDefinition>>hasReportableSlip {accessing} · ct 1/15/2022 22:54
- hasReportableSlip
^ hasReportableSlip ifNil: [false]
MCMethodDefinition>>hasReportableSlip: {accessing} · ct 1/15/2022 22:53
- hasReportableSlip: aBoolean
hasReportableSlip := aBoolean.
MCPatchBrowser>>checkForSlips {actions} · ct 1/15/2022 23:12
- checkForSlips
^ self checkForSlipsIn: self items
MCPatchBrowser>>checkForSlipsIn: {actions} · ct 1/15/2022 23:07
- checkForSlipsIn: patchItems
^ patchItems select: [:ea | ea hasReportableSlip]
MCPatchBrowser>>lookForSlips {actions} · ct 1/15/2022 23:11
- lookForSlips
"Look for slips in all changes. If there are any, ask the user how to treat them. Answer true if the operation should go on, or false if it should be canceled."
| slips |
slips := self checkForSlips ifEmpty: [^ true].
(Project uiManager
chooseFromLabeledValues: (OrderedDictionary new
at: 'Browse slips' translated put: [self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated];
at: 'Ignore slips and continue' translated put: [^ true];
at: 'Cancel' translated put: [^ false];
yourself)
title: ('This patch contains {1} halts, Transcript sends, or other possible slips.\\Save the new version anyway?' withCRs translated format: {slips size})) value.
^ false
MCPatchBrowser>>methodListMenu: {menus} · ct 1/15/2022 23:01
- methodListMenu: aMenu
super methodListMenu: aMenu.
aMenu add: 'check for slips' translated help: 'Check all changes for possible slips such as forgotten breakpoints or logging statements.' translated action: #reportSlips.
^ aMenu
MCPatchBrowser>>reportSlips {actions} · ct 1/15/2022 23:05
- reportSlips
"Look for slips in all changes and report to the user what was found."
| slips |
slips := self checkForSlips ifEmpty: [^ self inform: 'There were no obvious slips in this patch' translated].
self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated.
MCPatchOperation>>hasReportableSlip {testing} · ct 1/15/2022 22:49
- hasReportableSlip
^ self targetDefinition
ifNil: [false]
ifNotNil: [:def | def hasReportableSlip]
MCSaveVersionDialog>>accept {actions} · ct 1/15/2022 23:06 (changed) accept | logMessage logMessageWidget | self updateItems.
(Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- logMessage := (logMessageWidget := self findTextMorph: #logMessage) text asString. (logMessage isEmpty or: [logMessage beginsWith: 'empty log message']) ifTrue:
[(UIManager confirm: 'the log message is empty; are you sure you want to commit') ifFalse: [^ self]]
[(self confirm: 'The log message is empty; are you sure you want to commit?' translated) ifFalse: [^ self]] ifFalse: [logMessageWidget accept].
- self answer: { (self findTextMorph: #versionName) text asString. logMessage. ignore }
MCSaveVersionDialog>>checkForSlips {actions} · ct 1/15/2022 23:12
- checkForSlips
^ super checkForSlipsIn: (self items copyWithoutAll: self ignore)
Preference>>helpString: {menu} · ct 1/16/2022 01:31
- helpString: aString
helpString := aString
Sent from Squeak Inbox Talk ["mcSlips.1.cs"] ["mcSlips.png"]
Hi Christoph! :)
I believe the value of this feature lies in its ability to be available dynamically based on the package or changes being made, while also never becoming an obstacle when it's not required. Unfortunately, I find the current user experience design, which involves a global setting with pop-ups, somewhat disruptive to this end.
As a user of an IDE, I believe strongly in a user-interface philosophy that reinforces the user's workflow and minimizes disruptions. The frequent use of modal pop-ups, in my experience, tends to diverge from this principle. Therefore, I would kindly ask you to consider alternate methods of conveying information or asking for user input which avoids modal pop-ups whenever possible.
A potential solution for this particular scenario could be visually differentiating MCDefinitions with slips by utilizing color cues. This way, when a user clicks on them, the slip description could be displayed in the existing annotation pane or another suitable location.
Alternatively, if integrating the feature in such a way proves challenging, a "Check for slips" button situated near the save button might be an adequate solution. This would offer users dynamic access to the feature without necessitating a global preference setting or introducing additional preferences and modal interactions.
This is the second thing that's come up relative to the MC save dialog recently. I think it's itching for an upgrade. :)
It's nice to see your presence again, thank you for considering my feedback! :)
Best, Chris
On Thu, May 18, 2023 at 3:56 PM christoph.thiede@student.hpi.uni-potsdam.de wrote:
Hi Chris,
friendly reminder of this conversation. :-) Are your concerns still up to date regarding the performance and user experience of adding the existing slip detection from changeset fileouts to Monticello version savings? Would you accept this proposal if I turned off the slip detection unless the preference is enabled?
Best, Christoph
On 2022-01-16T19:11:11+01:00, christoph.thiede@student.hpi.uni-potsdam.de wrote:
Hi Dave, hi all,
thank you for the feedback! :-)
If you think that there is a risk of unintended side effects, then wait until after the release. Otherwise go ahead. But there is no harm in waiting a few weeks so I would advise waiting if you are not sure.
Right now I cannot think of any possible unintended consequences, but this also applied to my most recent mistakes, so I guess I will wait for some more time. I have no hurry, just wanted to avoid further incidents such as Monticello-ct.767. :-) I should focus more on contributing to the preparation of the next release than implementing new features anyway. :D
I like the idea of enabling users to check for slips, what do you think about adding that command (either via button or menu item) somewhere in both the Monticello AND ChangeSet windows.
It's all there: :-) ["changesorter-check for slips.png"] And with this patch also: ["mcSlips-menu.png"]
Regarding the preference, and, if I understood your point correctly, the performance aspect:
The menu items are available independently of the preference value. In the case of my proposed Monticello integration, the slip detection is run eagerly, independently of the preference value, as soon as you compile a method (Monticello assembles the working copy eagerly). For the change sorter, slips are collected on demand only, but even in a very large changeset, this happens without a noticeable delay for me. Have you made different experiences with this in the past?
Best, Christoph
Sent from Squeak Inbox Talk
On 2022-01-15T20:27:58-06:00, asqueaker(a)gmail.com wrote:
Christoph,
BTW, your font is really large in your emails. Are you doing this because you're using a high-dpi screen? :) Are you open to increasing your local font size instead? Your emails are hard to read.
Regards, Chris
On Sat, Jan 15, 2022 at 6:41 PM <christoph.thiede at student.hpi.uni-potsdam.de> wrote:
Any fundamental objections before I would merge this into the Trunk? :-)
=============== Summary ===============
Change Set: mcSlips Date: 16 January 2022 Author: Christoph Thiede
This changeset integrates the slip detection mechanism, previously only known from the domain of changesets, into the Monticello system. When users save a new version, it will automatically notify them about any possible slips and let them decide whether to browse them first or proceed the saving. Users can also scan their patch for slips manually by invocating the item from the patch list menu. The value of the preference #checkForSlips is still honored for the automatic detection, and the description of that preference is updated to mention the new usage.
=============== Postscript ===============
"Postscript:"
(Preferences preferenceAt: #checkForSlips) helpString: 'If true, then whenever you file out a change set or save a Monticello version, it is checked for ''slips'' and if any are found, you are so informed and given a chance to open a browser on them.'.
=============== Diff ===============
CompiledMethod>>hasReportableSlip {testing} · ct 1/15/2022 22:32 (changed) hasReportableSlip "Answer whether the receiver contains anything that should be brought to the attention of the author when filing out. Customize the lists here to suit your preferences. If slips do not get reported in spite of your best efforts here, make certain that the Preference 'checkForSlips' is set to true."
#(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: )
#(#Transcript #AA #BB #CC #DD #EE ) do: [:aSymbol | | assoc | (assoc := Smalltalk globals associationAt: aSymbol ifAbsent: []) ifNotNil: [(self hasLiteral: assoc) ifTrue: [^ true]]]. ^ false#(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: #haltIfNil #haltOnCount: #halt:onCount: #break ) do: [:aLit | (self hasLiteral: aLit) ifTrue: [^ true]].
MCChangeSelector>>select {actions} · ct 1/15/2022 22:46 (changed) select
(Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- self answer: (MCPatch operations: kept)
MCDefinition>>hasReportableSlip {testing} · ct 1/15/2022 22:49
- hasReportableSlip
^ false
MCMethodDefinition (changed) MCDefinition subclass: #MCMethodDefinition
instanceVariableNames: 'classIsMeta source category selector className timeStamp'
classVariableNames: '' poolDictionaries: '' category: 'Monticello-Modeling'instanceVariableNames: 'classIsMeta source category selector className timeStamp hasReportableSlip'
MCMethodDefinition class instanceVariableNames: 'definitions'
""
MCMethodDefinition class>>forMethodReference: {create} · ct 1/15/2022 22:53 (changed) forMethodReference: aMethodReference | definition | definition := self cachedDefinitions at: aMethodReference compiledMethod ifAbsent: []. (definition isNil or: [definition selector ~= aMethodReference methodSymbol or: [definition className ~= aMethodReference classSymbol or: [definition classIsMeta ~= aMethodReference classIsMeta or: [definition category ~= aMethodReference category]]]]) ifTrue: [definition := self className: aMethodReference classSymbol classIsMeta: aMethodReference classIsMeta selector: aMethodReference methodSymbol category: aMethodReference category timeStamp: aMethodReference timeStamp source: aMethodReference source.
^ definitiondefinition hasReportableSlip: aMethodReference compiledMethod hasReportableSlip. self cachedDefinitions at: aMethodReference compiledMethod put: definition].
MCMethodDefinition>>hasReportableSlip {accessing} · ct 1/15/2022 22:54
- hasReportableSlip
^ hasReportableSlip ifNil: [false]
MCMethodDefinition>>hasReportableSlip: {accessing} · ct 1/15/2022 22:53
- hasReportableSlip: aBoolean
hasReportableSlip := aBoolean.
MCPatchBrowser>>checkForSlips {actions} · ct 1/15/2022 23:12
- checkForSlips
^ self checkForSlipsIn: self items
MCPatchBrowser>>checkForSlipsIn: {actions} · ct 1/15/2022 23:07
- checkForSlipsIn: patchItems
^ patchItems select: [:ea | ea hasReportableSlip]
MCPatchBrowser>>lookForSlips {actions} · ct 1/15/2022 23:11
- lookForSlips
"Look for slips in all changes. If there are any, ask the user how to treat them. Answer true if the operation should go on, or false if it should be canceled."
| slips |
slips := self checkForSlips ifEmpty: [^ true].
(Project uiManager
chooseFromLabeledValues: (OrderedDictionary new
at: 'Browse slips' translated put: [self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated];
at: 'Ignore slips and continue' translated put: [^ true];
at: 'Cancel' translated put: [^ false];
yourself)
title: ('This patch contains {1} halts, Transcript sends, or other possible slips.\\Save the new version anyway?' withCRs translated format: {slips size})) value.
^ false
MCPatchBrowser>>methodListMenu: {menus} · ct 1/15/2022 23:01
- methodListMenu: aMenu
super methodListMenu: aMenu.
aMenu add: 'check for slips' translated help: 'Check all changes for possible slips such as forgotten breakpoints or logging statements.' translated action: #reportSlips.
^ aMenu
MCPatchBrowser>>reportSlips {actions} · ct 1/15/2022 23:05
- reportSlips
"Look for slips in all changes and report to the user what was found."
| slips |
slips := self checkForSlips ifEmpty: [^ self inform: 'There were no obvious slips in this patch' translated].
self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated.
MCPatchOperation>>hasReportableSlip {testing} · ct 1/15/2022 22:49
- hasReportableSlip
^ self targetDefinition
ifNil: [false]
ifNotNil: [:def | def hasReportableSlip]
MCSaveVersionDialog>>accept {actions} · ct 1/15/2022 23:06 (changed) accept | logMessage logMessageWidget | self updateItems.
(Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- logMessage := (logMessageWidget := self findTextMorph: #logMessage) text asString. (logMessage isEmpty or: [logMessage beginsWith: 'empty log message']) ifTrue:
[(UIManager confirm: 'the log message is empty; are you sure you want to commit') ifFalse: [^ self]]
[(self confirm: 'The log message is empty; are you sure you want to commit?' translated) ifFalse: [^ self]] ifFalse: [logMessageWidget accept].
- self answer: { (self findTextMorph: #versionName) text asString. logMessage. ignore }
MCSaveVersionDialog>>checkForSlips {actions} · ct 1/15/2022 23:12
- checkForSlips
^ super checkForSlipsIn: (self items copyWithoutAll: self ignore)
Preference>>helpString: {menu} · ct 1/16/2022 01:31
- helpString: aString
helpString := aString
Sent from Squeak Inbox Talk ["mcSlips.1.cs"] ["mcSlips.png"]
Hi Chris,
thank you for your arguments!
First, I share your concerns about too much interruptions from a UI. However, I would differentiate between hints and warnings. Hints should be optional and non-interrupting, but IMHO, warnings should indeed assure to attract the attention of the user. IMHO, "there is a superfluous halt in this method" goes into the category of warnings. Thus, I don't want to press an additional button to get that information. Similar to commiting a version without a message, I would consider this a situation where it would be appropriate for the tool to interrupt me and ask me to confirm my intention.
Also, how often do you actually see a detected slip? Keep in mind that this feature is enabled by default for changesets. I'm working quite a lot with changesets (just search the list for "[Review Request]" :-)), and I would estimate that I see a warning in maybe 1 of 50 file-outs. Given such a low positive rate, my (and probably many other poeople's) motivation to press an extra button would be pretty small.
Second, a good portion of your arguments reads to me as if they would relate to the existing checkForSlips preference/behavior in general but not to a - kind of missing - application of the already existing concept to the Monticello UI. Maybe we should consider to turn off this feature by default or make it non-interrupting. I like your idea of color cues, which could be applied both to the ChangeSorter and to the MCSaveVersionDialog. We could also colorize the "accept" button itself to make it more obvious. I want to give this approach a try. I will send another changeset for that. Still, I am not convinced that we should *not* interrupt users before they are saving an isThisEverCalled to the Trunk. :-)
Best, Christoph
On 2023-05-18T18:10:57-05:00, ma.chris.m@gmail.com wrote:
Hi Christoph! :)
I believe the value of this feature lies in its ability to be available dynamically based on the package or changes being made, while also never becoming an obstacle when it's not required. Unfortunately, I find the current user experience design, which involves a global setting with pop-ups, somewhat disruptive to this end.
As a user of an IDE, I believe strongly in a user-interface philosophy that reinforces the user's workflow and minimizes disruptions. The frequent use of modal pop-ups, in my experience, tends to diverge from this principle. Therefore, I would kindly ask you to consider alternate methods of conveying information or asking for user input which avoids modal pop-ups whenever possible.
A potential solution for this particular scenario could be visually differentiating MCDefinitions with slips by utilizing color cues. This way, when a user clicks on them, the slip description could be displayed in the existing annotation pane or another suitable location.
Alternatively, if integrating the feature in such a way proves challenging, a "Check for slips" button situated near the save button might be an adequate solution. This would offer users dynamic access to the feature without necessitating a global preference setting or introducing additional preferences and modal interactions.
This is the second thing that's come up relative to the MC save dialog recently. I think it's itching for an upgrade. :)
It's nice to see your presence again, thank you for considering my feedback! :)
Best, Chris
On Thu, May 18, 2023 at 3:56 PM <christoph.thiede(a)student.hpi.uni-potsdam.de> wrote:
Hi Chris,
friendly reminder of this conversation. :-) Are your concerns still up to date regarding the performance and user experience of adding the existing slip detection from changeset fileouts to Monticello version savings? Would you accept this proposal if I turned off the slip detection unless the preference is enabled?
Best, Christoph
On 2022-01-16T19:11:11+01:00, christoph.thiede(a)student.hpi.uni-potsdam.de wrote:
Hi Dave, hi all,
thank you for the feedback! :-)
If you think that there is a risk of unintended side effects, then wait until after the release. Otherwise go ahead. But there is no harm in waiting a few weeks so I would advise waiting if you are not sure.
Right now I cannot think of any possible unintended consequences, but this also applied to my most recent mistakes, so I guess I will wait for some more time. I have no hurry, just wanted to avoid further incidents such as Monticello-ct.767. :-) I should focus more on contributing to the preparation of the next release than implementing new features anyway. :D
I like the idea of enabling users to check for slips, what do you think about adding that command (either via button or menu item) somewhere in both the Monticello AND ChangeSet windows.
It's all there: :-) ["changesorter-check for slips.png"] And with this patch also: ["mcSlips-menu.png"]
Regarding the preference, and, if I understood your point correctly, the performance aspect:
The menu items are available independently of the preference value. In the case of my proposed Monticello integration, the slip detection is run eagerly, independently of the preference value, as soon as you compile a method (Monticello assembles the working copy eagerly). For the change sorter, slips are collected on demand only, but even in a very large changeset, this happens without a noticeable delay for me. Have you made different experiences with this in the past?
Best, Christoph
Sent from Squeak Inbox Talk
On 2022-01-15T20:27:58-06:00, asqueaker(a)gmail.com wrote:
Christoph,
BTW, your font is really large in your emails. Are you doing this because you're using a high-dpi screen? :) Are you open to increasing your local font size instead? Your emails are hard to read.
Regards, Chris
On Sat, Jan 15, 2022 at 6:41 PM <christoph.thiede at student.hpi.uni-potsdam.de> wrote:
Any fundamental objections before I would merge this into the Trunk? :-)
=============== Summary ===============
Change Set: mcSlips Date: 16 January 2022 Author: Christoph Thiede
This changeset integrates the slip detection mechanism, previously only known from the domain of changesets, into the Monticello system. When users save a new version, it will automatically notify them about any possible slips and let them decide whether to browse them first or proceed the saving. Users can also scan their patch for slips manually by invocating the item from the patch list menu. The value of the preference #checkForSlips is still honored for the automatic detection, and the description of that preference is updated to mention the new usage.
=============== Postscript ===============
"Postscript:"
(Preferences preferenceAt: #checkForSlips) helpString: 'If true, then whenever you file out a change set or save a Monticello version, it is checked for ''slips'' and if any are found, you are so informed and given a chance to open a browser on them.'.
=============== Diff ===============
CompiledMethod>>hasReportableSlip {testing} · ct 1/15/2022 22:32 (changed) hasReportableSlip "Answer whether the receiver contains anything that should be brought to the attention of the author when filing out. Customize the lists here to suit your preferences. If slips do not get reported in spite of your best efforts here, make certain that the Preference 'checkForSlips' is set to true."
#(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: )
#(#Transcript #AA #BB #CC #DD #EE ) do: [:aSymbol | | assoc | (assoc := Smalltalk globals associationAt: aSymbol ifAbsent: []) ifNotNil: [(self hasLiteral: assoc) ifTrue: [^ true]]]. ^ false#(#doOnlyOnce: #halt #halt: #hottest #printDirectlyToDisplay #toRemove #personal #urgent #haltOnce #haltOnce: #haltIf: #haltIfNil #haltOnCount: #halt:onCount: #break ) do: [:aLit | (self hasLiteral: aLit) ifTrue: [^ true]].
MCChangeSelector>>select {actions} · ct 1/15/2022 22:46 (changed) select
(Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- self answer: (MCPatch operations: kept)
MCDefinition>>hasReportableSlip {testing} · ct 1/15/2022 22:49
- hasReportableSlip
^ false
MCMethodDefinition (changed) MCDefinition subclass: #MCMethodDefinition
instanceVariableNames: 'classIsMeta source category selector className timeStamp'
classVariableNames: '' poolDictionaries: '' category: 'Monticello-Modeling'instanceVariableNames: 'classIsMeta source category selector className timeStamp hasReportableSlip'
MCMethodDefinition class instanceVariableNames: 'definitions'
""
MCMethodDefinition class>>forMethodReference: {create} · ct 1/15/2022 22:53 (changed) forMethodReference: aMethodReference | definition | definition := self cachedDefinitions at: aMethodReference compiledMethod ifAbsent: []. (definition isNil or: [definition selector ~= aMethodReference methodSymbol or: [definition className ~= aMethodReference classSymbol or: [definition classIsMeta ~= aMethodReference classIsMeta or: [definition category ~= aMethodReference category]]]]) ifTrue: [definition := self className: aMethodReference classSymbol classIsMeta: aMethodReference classIsMeta selector: aMethodReference methodSymbol category: aMethodReference category timeStamp: aMethodReference timeStamp source: aMethodReference source.
^ definitiondefinition hasReportableSlip: aMethodReference compiledMethod hasReportableSlip. self cachedDefinitions at: aMethodReference compiledMethod put: definition].
MCMethodDefinition>>hasReportableSlip {accessing} · ct 1/15/2022 22:54
- hasReportableSlip
^ hasReportableSlip ifNil: [false]
MCMethodDefinition>>hasReportableSlip: {accessing} · ct 1/15/2022 22:53
- hasReportableSlip: aBoolean
hasReportableSlip := aBoolean.
MCPatchBrowser>>checkForSlips {actions} · ct 1/15/2022 23:12
- checkForSlips
^ self checkForSlipsIn: self items
MCPatchBrowser>>checkForSlipsIn: {actions} · ct 1/15/2022 23:07
- checkForSlipsIn: patchItems
^ patchItems select: [:ea | ea hasReportableSlip]
MCPatchBrowser>>lookForSlips {actions} · ct 1/15/2022 23:11
- lookForSlips
"Look for slips in all changes. If there are any, ask the user how to treat them. Answer true if the operation should go on, or false if it should be canceled."
| slips |
slips := self checkForSlips ifEmpty: [^ true].
(Project uiManager
chooseFromLabeledValues: (OrderedDictionary new
at: 'Browse slips' translated put: [self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated];
at: 'Ignore slips and continue' translated put: [^ true];
at: 'Cancel' translated put: [^ false];
yourself)
title: ('This patch contains {1} halts, Transcript sends, or other possible slips.\\Save the new version anyway?' withCRs translated format: {slips size})) value.
^ false
MCPatchBrowser>>methodListMenu: {menus} · ct 1/15/2022 23:01
- methodListMenu: aMenu
super methodListMenu: aMenu.
aMenu add: 'check for slips' translated help: 'Check all changes for possible slips such as forgotten breakpoints or logging statements.' translated action: #reportSlips.
^ aMenu
MCPatchBrowser>>reportSlips {actions} · ct 1/15/2022 23:05
- reportSlips
"Look for slips in all changes and report to the user what was found."
| slips |
slips := self checkForSlips ifEmpty: [^ self inform: 'There were no obvious slips in this patch' translated].
self systemNavigation
browseMessageList: (slips collect: [:ea | ea definition asMethodReference])
name: 'Possible slips in new version' translated.
MCPatchOperation>>hasReportableSlip {testing} · ct 1/15/2022 22:49
- hasReportableSlip
^ self targetDefinition
ifNil: [false]
ifNotNil: [:def | def hasReportableSlip]
MCSaveVersionDialog>>accept {actions} · ct 1/15/2022 23:06 (changed) accept | logMessage logMessageWidget | self updateItems.
(Preferences checkForSlips ==> [self lookForSlips]) ifFalse: [^ self].
- logMessage := (logMessageWidget := self findTextMorph: #logMessage) text asString. (logMessage isEmpty or: [logMessage beginsWith: 'empty log message']) ifTrue:
[(UIManager confirm: 'the log message is empty; are you sure you want to commit') ifFalse: [^ self]]
[(self confirm: 'The log message is empty; are you sure you want to commit?' translated) ifFalse: [^ self]] ifFalse: [logMessageWidget accept].
- self answer: { (self findTextMorph: #versionName) text asString. logMessage. ignore }
MCSaveVersionDialog>>checkForSlips {actions} · ct 1/15/2022 23:12
- checkForSlips
^ super checkForSlipsIn: (self items copyWithoutAll: self ignore)
Preference>>helpString: {menu} · ct 1/16/2022 01:31
- helpString: aString
helpString := aString
Sent from Squeak Inbox Talk ["mcSlips.1.cs"] ["mcSlips.png"]
Hi Christoph,
thank you for your arguments!
Your genuine appreciation for good design makes discussing and searching for ideas an enjoyable experience for me as well, thanks. :)
First, I share your concerns about too much interruptions from a UI. However, I would differentiate between hints and warnings. Hints should be optional and non-interrupting, but IMHO, warnings should indeed assure to attract the attention of the user. IMHO, "there is a superfluous halt in this method" goes into the category of warnings. Thus, I don't want to press an additional button to get that information. Similar to commiting a version without a message, I would consider this a situation where it would be appropriate for the tool to interrupt me and ask me to confirm my intention.
I agree with those sentiments about hints vs. warnings, however, I don't think they're germaine to this feature about slips. For the "maintenance" phase of a project, where basically only bug fixes are being done, I can appreciate your position better. Developing a large complex project from scratch over a period of months, however, often involves making "checkpoints" along the way that one can easily revert to, for example, to conduct a design experiment that might turn out to be infeasible, and require a reversion. The project is not close to done, and so there's a lot of debugging code still lying around. It's a time when the developer WANTS her halts, and doesn't want to be distracted by a bunch of pop ups about lint just to begin working on the experiment. And yet, if an update to a CORE package were necessary along the way in order to support the new project, she might want to check for slips in THAT package before committing. It really needs that on-demand capability which a global preference cannot do.
Also, how often do you actually see a detected slip? Keep in mind that this feature is enabled by default for changesets. I'm working quite a lot with changesets (just search the list for "[Review Request]" :-)), and I would estimate that I see a warning in maybe 1 of 50 file-outs. Given such a low positive rate, my (and probably many other poeople's) motivation to press an extra button would be pretty small.
I don't care for economic arguments like this because they can have a compromising effect on quality prior to asking the question, "what is the flat out best design, independent of development cost?" I feel this should be asked first, THEN figure out how to make it economical. I also believe it's important to also always favor the *user's* economy over the developer's, because the developer's cost is finite, whereas the user's costs (and benefits) are theoretically infinite. I don't think the ideas we're tossing about are overly difficult, so they're at least worth considering.
Second, a good portion of your arguments reads to me as if they would relate to the existing checkForSlips preference/behavior in general but not to a - kind of missing - application of the already existing concept to the Monticello UI. Maybe we should consider to turn off this feature by default or make it non-interrupting. I like your idea of color cues, which could be applied both to the ChangeSorter and to the MCSaveVersionDialog. We could also colorize the "accept" button itself to make it more obvious. I want to give this approach a try. I will send another changeset for that.
Thanks for exploring options using colorization and/or emphasis to instantly inform the user (possibly without any button presses)! I was only thinking about the MC save dialog during this discussion and not filing out changesets. I wouldn't want "consistency" to be a reason the MC save dialog would be restricted from being the best design it can be.
Still, I am not convinced that we should *not* interrupt users before they are saving an isThisEverCalled to the Trunk. :-)
I understand, but we should be careful, such an interruption will not catch other slips, nor guarantee quality. Having tools that "kinda sorta halfway help," could potentially deter the development of other true processes and tools for guaranteeing quality, while accumulating complexity into the system over time.
Best, Chris
Hey Christoph, I just remembered, we have a "Show message icons" preference. I don't know what all kinds of slips we need to detect, but if MC Save could be made to honor that preference, it could for starters at least indicate the methods with halts. And if we need more, maybe more slips icons would be useful for the IDE, as well..?
BTW, your font is really large in your emails. Are you doing this because you're using a high-dpi screen? :) Are you open to increasing your local font size instead? Your emails are hard to read.
Sorry for that! Apparently Preferences standardCodeFont is automatically added to any text that is opened in a workspace via #edit (see the #font: send in MorphicToolBuilder>>#buildPluggableText:). This only happens since ToolBuilder-Morphic-mt.276 because before, the font was ignored by an empty workspace and the text was only added to it after building. I'm not sure whether the tool builder should append any attributes to the displayed text, and I don't know how PluggableTextMorph should implement font changes in the right way ...
For now, I have turned off the automatic font assignment in the toolbuilder in order not to disturb you with any further ugly large letters! :-)
Best, Christoph
--- Sent from Squeak Inbox Talk
On 2022-01-15T20:27:58-06:00, asqueaker@gmail.com wrote:
Christoph,
BTW, your font is really large in your emails. Are you doing this because you're using a high-dpi screen? :) Are you open to increasing your local font size instead? Your emails are hard to read.
Regards, Chris
squeak-dev@lists.squeakfoundation.org