The Trunk: Compiler-nice.427.mcz

Nicolas Cellier uploaded a new version of Compiler to project The Trunk:
http://source.squeak.org/trunk/Compiler-nice.427.mcz

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

Name: Compiler-nice.427
Author: nice
Time: 8 May 2020, 6:59:54.689381 pm
UUID: e2c7d4b2-b79d-4d02-a576-872003ec88ed
Ancestors: Compiler-ct.426

Avoid sending select/deselect to the editor in case of interactive correction.

These are st80 specific, forcing Morphic to have stubs, and are considered too intrusive: we should better let such responsibility to the editor, that's its own business.

The send of #select was un-necessary anyway, because selectFrom:to: will send #selectAndScroll, which will send #select.

However, deselect was still required for st80, so replace the sequence deselect; selectInvisiblyFrom:to: with a single message #selectIntervalInvisibly: which is provided by ST80-nice.254 and Morphic-nice.1657.

Why is this needed? As explained in the st80 commit, we want to restore the (eventually slightly modified) user selection once the corrections are performed.

We must do that at each correction, so as to correctly track addition or delection of characters (the selection interval might need to grow or shrink accordingly).

But we do not want to make the user selection visible instantly, otherwise the selection will go back to user selection and forth to next zone of interactive correction repeatedly creating an annoying flashing effect.

Care to let future reader know about it with a comment, there's nothing that obvious which could make the comment superfluous!

=============== Diff against Compiler-ct.426 ===============

Item was changed:
  ----- Method: Parser>>ambiguousSelector:inRange: (in category 'error correction') -----
  ambiguousSelector: aString inRange: anInterval
+       | correctedSelector userSelection intervalWithOffset |
-       | correctedSelector userSelection offset intervalWithOffset |

        self interactive ifFalse: [
                "In non interactive mode, compile with backward comapatibility: $- is part of literal argument"
                Transcript cr; store: encoder classEncoding; nextPutAll:#'>>';store: encoder selector; show: ' would send ' , token , '-'.
                ^super ambiguousSelector: aString inRange: anInterval].

        "handle the text selection"
        userSelection := cue requestor selectionInterval.
        intervalWithOffset := anInterval first + requestorOffset to: anInterval last + requestorOffset.
        cue requestor selectFrom: intervalWithOffset first to: intervalWithOffset last.
-       cue requestor select.

        "Build the menu with alternatives"
        correctedSelector := AmbiguousSelector
                        signalName: aString
                        inRange: intervalWithOffset.
        correctedSelector ifNil: [^self fail].

+       "Restore the user selection state, but do not display selection yet
+        This will avoid flashing effect when chaining multiple corrections."
+       cue requestor selectIntervalInvisibly: userSelection.
+       
        "Execute the selected action"
+       self substituteWord: correctedSelector wordInterval: intervalWithOffset offset: 0.
-       offset := self substituteWord: correctedSelector wordInterval: intervalWithOffset offset: 0.
-       cue requestor deselect.
-       cue requestor selectInvisiblyFrom: userSelection first to: userSelection last + offset.
        token := (correctedSelector readStream upTo: Character space) asSymbol!

Item was changed:
  ----- Method: Parser>>correctSelector:wordIntervals:exprInterval:ifAbort: (in category 'error correction') -----
  correctSelector: proposedKeyword wordIntervals: spots exprInterval: expInt ifAbort: abortAction
        "Correct the proposedKeyword to some selector symbol, correcting the original text if such action is indicated.  abortAction is invoked if the proposedKeyword couldn't be converted into a valid selector.  Spots is an ordered collection of intervals within the test stream of the for each of the keyword parts."

        | correctSelector userSelection |
        "If we can't ask the user, assume that the keyword will be defined later"
        self interactive ifFalse: [^proposedKeyword asSymbol].

        userSelection := cue requestor selectionInterval.
        cue requestor selectFrom: spots first first to: spots last last.
-       cue requestor select.

        correctSelector := UnknownSelector name: proposedKeyword.
        correctSelector ifNil: [^abortAction value].

+       "Restore the user selection state, but do not display selection yet
+        This will avoid flashing effect when chaining multiple corrections."
+       cue requestor selectIntervalInvisibly: userSelection.
-       cue requestor deselect.
-       cue requestor selectInvisiblyFrom: userSelection first to: userSelection last.

        self substituteSelector: correctSelector keywords wordIntervals: spots.
        ^(proposedKeyword last ~~ $:
           and: [correctSelector last == $:])
                ifTrue: [abortAction value]
                ifFalse: [correctSelector]!

Item was changed:
  ----- Method: Parser>>correctVariable:interval: (in category 'error correction') -----
  correctVariable: proposedVariable interval: spot
        "Correct the proposedVariable to a known variable, or declare it as a new
        variable if such action is requested.  We support declaring lowercase
        variables as temps or inst-vars, and uppercase variables as Globals or
        ClassVars, depending on whether the context is nil (class=UndefinedObject).
        Spot is the interval within the test stream of the variable.
        rr 3/4/2004 10:26 : adds the option to define a new class. "

        "Check if this is an i-var, that has been corrected already (ugly)"

        "Display the pop-up menu"

        | binding userSelection action |
        (encoder classEncoding instVarNames includes: proposedVariable) ifTrue:
                [^InstanceVariableNode new
                        name: proposedVariable
                        index: (encoder classEncoding allInstVarNames indexOf: proposedVariable)].

        "First check to see if the requestor knows anything about the variable"
        (binding := cue requestor ifNotNil: [:object | object bindingOf: proposedVariable])
                ifNotNil: [^encoder global: binding name: proposedVariable].

        "If we can't ask the user for correction, make it undeclared"
        self interactive ifFalse: [^encoder undeclared: proposedVariable].

        userSelection := cue requestor selectionInterval.
        cue requestor selectFrom: spot first to: spot last.
-       cue requestor select.

        "Build the menu with alternatives"
        action := UndeclaredVariable
                                signalFor: self
                                name: proposedVariable
                                inRange: spot.
        action ifNil: [^self fail].

+       "Restore the user selection state, but do not display selection yet
+        This will avoid flashing effect when chaining multiple corrections."
+       cue requestor selectIntervalInvisibly: userSelection.
+       
        "Execute the selected action"
-       cue requestor deselect.
-       cue requestor selectInvisiblyFrom: userSelection first to: userSelection last.
        ^action value!

Item was changed:
  ----- Method: Parser>>queryUndefined (in category 'error correction') -----
  queryUndefined
        | varStart varName |
        varName := parseNode key.
        varStart := self endOfLastToken + requestorOffset - varName size + 1.
+       cue requestor selectFrom: varStart to: varStart + varName size - 1.
-       cue requestor selectFrom: varStart to: varStart + varName size - 1; select.
        (UndefinedVariable name: varName) ifFalse: [^ self fail]!

Item was changed:
  ----- Method: Parser>>substituteSelector:wordIntervals: (in category 'error correction') -----
  substituteSelector: selectorParts wordIntervals: spots
+       "Substitute the correctSelector into the (presumed interactive) receiver."
-       "Substitute the correctSelector into the (presuamed interactive) receiver."
        | offset |
        offset := 0.
        selectorParts with: spots do:
                [ :word :interval |
                offset := self substituteWord: word wordInterval: interval offset: offset ]
  !

Hi Nicolas,

I did not yet study your full commit, but this introduces a conflict with Compiler-ct.423 from the Inbox. See also Morphic-ct.1640 and Tests-ct.429, both still in the Inbox. Would be nice if someone could review them before the merge conflicts get even worse. :-)

Best,

Christoph

---

Von: Squeak-dev <[hidden email]> im Auftrag von [hidden email] <[hidden email]>
Gesendet: Freitag, 8. Mai 2020 18:59:56
An: [hidden email]; [hidden email]
Betreff: [squeak-dev] The Trunk: Compiler-nice.427.mcz

 

Nicolas Cellier uploaded a new version of Compiler to project The Trunk:
http://source.squeak.org/trunk/Compiler-nice.427.mcz

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

Name: Compiler-nice.427
Author: nice
Time: 8 May 2020, 6:59:54.689381 pm
UUID: e2c7d4b2-b79d-4d02-a576-872003ec88ed
Ancestors: Compiler-ct.426

Avoid sending select/deselect to the editor in case of interactive correction.

These are st80 specific, forcing Morphic to have stubs, and are considered too intrusive: we should better let such responsibility to the editor, that's its own business.

The send of #select was un-necessary anyway, because selectFrom:to: will send #selectAndScroll, which will send #select.

However, deselect was still required for st80, so replace the sequence deselect; selectInvisiblyFrom:to: with a single message #selectIntervalInvisibly: which is provided by ST80-nice.254 and Morphic-nice.1657.

Why is this needed? As explained in the st80 commit, we want to restore the (eventually slightly modified) user selection once the corrections are performed.

We must do that at each correction, so as to correctly track addition or delection of characters (the selection interval might need to grow or shrink accordingly).

But we do not want to make the user selection visible instantly, otherwise the selection will go back to user selection and forth to next zone of interactive correction repeatedly creating an annoying flashing effect.

Care to let future reader know about it with a comment, there's nothing that obvious which could make the comment superfluous!

=============== Diff against Compiler-ct.426 ===============

Item was changed:
  ----- Method: Parser>>ambiguousSelector:inRange: (in category 'error correction') -----
  ambiguousSelector: aString inRange: anInterval
+        | correctedSelector userSelection intervalWithOffset |
-        | correctedSelector userSelection offset intervalWithOffset |
        
         self interactive ifFalse: [
                 "In non interactive mode, compile with backward comapatibility: $- is part of literal argument"
                 Transcript cr; store: encoder classEncoding; nextPutAll:#'>>';store: encoder selector; show: ' would send ' , token , '-'.
                 ^super ambiguousSelector: aString inRange: anInterval].
        
         "handle the text selection"
         userSelection := cue requestor selectionInterval.
         intervalWithOffset := anInterval first + requestorOffset to: anInterval last + requestorOffset.
         cue requestor selectFrom: intervalWithOffset first to: intervalWithOffset last.
-        cue requestor select.
 
         "Build the menu with alternatives"
         correctedSelector := AmbiguousSelector
                         signalName: aString
                         inRange: intervalWithOffset.
         correctedSelector ifNil: [^self fail].
 
+        "Restore the user selection state, but do not display selection yet
+         This will avoid flashing effect when chaining multiple corrections."
+        cue requestor selectIntervalInvisibly: userSelection.
+       
         "Execute the selected action"
+        self substituteWord: correctedSelector wordInterval: intervalWithOffset offset: 0.
-        offset := self substituteWord: correctedSelector wordInterval: intervalWithOffset offset: 0.
-        cue requestor deselect.
-        cue requestor selectInvisiblyFrom: userSelection first to: userSelection last + offset.
         token := (correctedSelector readStream upTo: Character space) asSymbol!

Item was changed:
  ----- Method: Parser>>correctSelector:wordIntervals:exprInterval:ifAbort: (in category 'error correction') -----
  correctSelector: proposedKeyword wordIntervals: spots exprInterval: expInt ifAbort: abortAction
         "Correct the proposedKeyword to some selector symbol, correcting the original text if such action is indicated.  abortAction is invoked if the proposedKeyword couldn't be converted into a valid selector.  Spots is an ordered collection of intervals within the test stream of the for each of the keyword parts."
 
         | correctSelector userSelection |
         "If we can't ask the user, assume that the keyword will be defined later"
         self interactive ifFalse: [^proposedKeyword asSymbol].
 
         userSelection := cue requestor selectionInterval.
         cue requestor selectFrom: spots first first to: spots last last.
-        cue requestor select.
 
         correctSelector := UnknownSelector name: proposedKeyword.
         correctSelector ifNil: [^abortAction value].
 
+        "Restore the user selection state, but do not display selection yet
+         This will avoid flashing effect when chaining multiple corrections."
+        cue requestor selectIntervalInvisibly: userSelection.
-        cue requestor deselect.
-        cue requestor selectInvisiblyFrom: userSelection first to: userSelection last.
 
         self substituteSelector: correctSelector keywords wordIntervals: spots.
         ^(proposedKeyword last ~~ $:
            and: [correctSelector last == $:])
                 ifTrue: [abortAction value]
                 ifFalse: [correctSelector]!

Item was changed:
  ----- Method: Parser>>correctVariable:interval: (in category 'error correction') -----
  correctVariable: proposedVariable interval: spot
         "Correct the proposedVariable to a known variable, or declare it as a new
         variable if such action is requested.  We support declaring lowercase
         variables as temps or inst-vars, and uppercase variables as Globals or
         ClassVars, depending on whether the context is nil (class=UndefinedObject).
         Spot is the interval within the test stream of the variable.
         rr 3/4/2004 10:26 : adds the option to define a new class. "
 
         "Check if this is an i-var, that has been corrected already (ugly)"
 
         "Display the pop-up menu"
 
         | binding userSelection action |
         (encoder classEncoding instVarNames includes: proposedVariable) ifTrue:
                 [^InstanceVariableNode new
                         name: proposedVariable
                         index: (encoder classEncoding allInstVarNames indexOf: proposedVariable)].
 
         "First check to see if the requestor knows anything about the variable"
         (binding := cue requestor ifNotNil: [:object | object bindingOf: proposedVariable])
                 ifNotNil: [^encoder global: binding name: proposedVariable].
 
         "If we can't ask the user for correction, make it undeclared"
         self interactive ifFalse: [^encoder undeclared: proposedVariable].
 
         userSelection := cue requestor selectionInterval.
         cue requestor selectFrom: spot first to: spot last.
-        cue requestor select.
 
         "Build the menu with alternatives"
         action := UndeclaredVariable
                                 signalFor: self
                                 name: proposedVariable
                                 inRange: spot.
         action ifNil: [^self fail].
 
+        "Restore the user selection state, but do not display selection yet
+         This will avoid flashing effect when chaining multiple corrections."
+        cue requestor selectIntervalInvisibly: userSelection.
+       
         "Execute the selected action"
-        cue requestor deselect.
-        cue requestor selectInvisiblyFrom: userSelection first to: userSelection last.
         ^action value!

Item was changed:
  ----- Method: Parser>>queryUndefined (in category 'error correction') -----
  queryUndefined
         | varStart varName |
         varName := parseNode key.
         varStart := self endOfLastToken + requestorOffset - varName size + 1.
+        cue requestor selectFrom: varStart to: varStart + varName size - 1.
-        cue requestor selectFrom: varStart to: varStart + varName size - 1; select.
         (UndefinedVariable name: varName) ifFalse: [^ self fail]!

Item was changed:
  ----- Method: Parser>>substituteSelector:wordIntervals: (in category 'error correction') -----
  substituteSelector: selectorParts wordIntervals: spots
+        "Substitute the correctSelector into the (presumed interactive) receiver."
-        "Substitute the correctSelector into the (presuamed interactive) receiver."
         | offset |
         offset := 0.
         selectorParts with: spots do:
                 [ :word :interval |
                 offset := self substituteWord: word wordInterval: interval offset: offset ]
  !

Hi Nicolas,

thank you for merging my commits! But why did you reject the #selectFrom:to:during: if you like it? Also, #testUndefinedVariable from Tests-ct.429 still fails (all the tests need to be adjusted to your Compiler changes, but you can even reproduce the second example manually). And yes, adding some comments sounds like a good idea :-)

Best,

Christoph

---

Von: Squeak-dev <[hidden email]> im Auftrag von Nicolas Cellier <[hidden email]>
Gesendet: Freitag, 8. Mai 2020 20:59:57
An: The general-purpose Squeak developers list
Betreff: Re: [squeak-dev] The Trunk: Compiler-nice.427.mcz

 

Hi Christoph,

yes, I see that now, you were on the same track a bit sooner.

I did not want to correct anything, just simplify the accumulated cruft that prevents me finding a bug, so we came on the same track by 2 different paths.

It's funny, I first considered introducing exactly that selectFrom:to:during: but on the editor side, which was not a so great idea and made me renounce for a simpler change. I still like it at Parser side, it could help factoring some more code (and explanation about why we need to select invisibly).

Le ven. 8 mai 2020 à 19:12, Thiede, Christoph <[hidden email]> a écrit :

Hi Nicolas,

I did not yet study your full commit, but this introduces a conflict with Compiler-ct.423 from the Inbox. See also Morphic-ct.1640 and Tests-ct.429, both still in the Inbox. Would be nice if someone could review them before the merge conflicts get even worse. :-)

Best,

Christoph

---

Von: Squeak-dev <[hidden email]> im Auftrag von [hidden email] <[hidden email]>
Gesendet: Freitag, 8. Mai 2020 18:59:56
An: [hidden email]; [hidden email]
Betreff: [squeak-dev] The Trunk: Compiler-nice.427.mcz

 

Nicolas Cellier uploaded a new version of Compiler to project The Trunk:
http://source.squeak.org/trunk/Compiler-nice.427.mcz

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

Name: Compiler-nice.427
Author: nice
Time: 8 May 2020, 6:59:54.689381 pm
UUID: e2c7d4b2-b79d-4d02-a576-872003ec88ed
Ancestors: Compiler-ct.426

Avoid sending select/deselect to the editor in case of interactive correction.

These are st80 specific, forcing Morphic to have stubs, and are considered too intrusive: we should better let such responsibility to the editor, that's its own business.

The send of #select was un-necessary anyway, because selectFrom:to: will send #selectAndScroll, which will send #select.

However, deselect was still required for st80, so replace the sequence deselect; selectInvisiblyFrom:to: with a single message #selectIntervalInvisibly: which is provided by ST80-nice.254 and Morphic-nice.1657.

Why is this needed? As explained in the st80 commit, we want to restore the (eventually slightly modified) user selection once the corrections are performed.

We must do that at each correction, so as to correctly track addition or delection of characters (the selection interval might need to grow or shrink accordingly).

But we do not want to make the user selection visible instantly, otherwise the selection will go back to user selection and forth to next zone of interactive correction repeatedly creating an annoying flashing effect.

Care to let future reader know about it with a comment, there's nothing that obvious which could make the comment superfluous!

=============== Diff against Compiler-ct.426 ===============

Item was changed:
  ----- Method: Parser>>ambiguousSelector:inRange: (in category 'error correction') -----
  ambiguousSelector: aString inRange: anInterval
+        | correctedSelector userSelection intervalWithOffset |
-        | correctedSelector userSelection offset intervalWithOffset |
        
         self interactive ifFalse: [
                 "In non interactive mode, compile with backward comapatibility: $- is part of literal argument"
                 Transcript cr; store: encoder classEncoding; nextPutAll:#'>>';store: encoder selector; show: ' would send ' , token , '-'.
                 ^super ambiguousSelector: aString inRange: anInterval].
        
         "handle the text selection"
         userSelection := cue requestor selectionInterval.
         intervalWithOffset := anInterval first + requestorOffset to: anInterval last + requestorOffset.
         cue requestor selectFrom: intervalWithOffset first to: intervalWithOffset last.
-        cue requestor select.
 
         "Build the menu with alternatives"
         correctedSelector := AmbiguousSelector
                         signalName: aString
                         inRange: intervalWithOffset.
         correctedSelector ifNil: [^self fail].
 
+        "Restore the user selection state, but do not display selection yet
+         This will avoid flashing effect when chaining multiple corrections."
+        cue requestor selectIntervalInvisibly: userSelection.
+       
         "Execute the selected action"
+        self substituteWord: correctedSelector wordInterval: intervalWithOffset offset: 0.
-        offset := self substituteWord: correctedSelector wordInterval: intervalWithOffset offset: 0.
-        cue requestor deselect.
-        cue requestor selectInvisiblyFrom: userSelection first to: userSelection last + offset.
         token := (correctedSelector readStream upTo: Character space) asSymbol!

Item was changed:
  ----- Method: Parser>>correctSelector:wordIntervals:exprInterval:ifAbort: (in category 'error correction') -----
  correctSelector: proposedKeyword wordIntervals: spots exprInterval: expInt ifAbort: abortAction
         "Correct the proposedKeyword to some selector symbol, correcting the original text if such action is indicated.  abortAction is invoked if the proposedKeyword couldn't be converted into a valid selector.  Spots is an ordered collection of intervals within the test stream of the for each of the keyword parts."
 
         | correctSelector userSelection |
         "If we can't ask the user, assume that the keyword will be defined later"
         self interactive ifFalse: [^proposedKeyword asSymbol].
 
         userSelection := cue requestor selectionInterval.
         cue requestor selectFrom: spots first first to: spots last last.
-        cue requestor select.
 
         correctSelector := UnknownSelector name: proposedKeyword.
         correctSelector ifNil: [^abortAction value].
 
+        "Restore the user selection state, but do not display selection yet
+         This will avoid flashing effect when chaining multiple corrections."
+        cue requestor selectIntervalInvisibly: userSelection.
-        cue requestor deselect.
-        cue requestor selectInvisiblyFrom: userSelection first to: userSelection last.
 
         self substituteSelector: correctSelector keywords wordIntervals: spots.
         ^(proposedKeyword last ~~ $:
            and: [correctSelector last == $:])
                 ifTrue: [abortAction value]
                 ifFalse: [correctSelector]!

Item was changed:
  ----- Method: Parser>>correctVariable:interval: (in category 'error correction') -----
  correctVariable: proposedVariable interval: spot
         "Correct the proposedVariable to a known variable, or declare it as a new
         variable if such action is requested.  We support declaring lowercase
         variables as temps or inst-vars, and uppercase variables as Globals or
         ClassVars, depending on whether the context is nil (class=UndefinedObject).
         Spot is the interval within the test stream of the variable.
         rr 3/4/2004 10:26 : adds the option to define a new class. "
 
         "Check if this is an i-var, that has been corrected already (ugly)"
 
         "Display the pop-up menu"
 
         | binding userSelection action |
         (encoder classEncoding instVarNames includes: proposedVariable) ifTrue:
                 [^InstanceVariableNode new
                         name: proposedVariable
                         index: (encoder classEncoding allInstVarNames indexOf: proposedVariable)].
 
         "First check to see if the requestor knows anything about the variable"
         (binding := cue requestor ifNotNil: [:object | object bindingOf: proposedVariable])
                 ifNotNil: [^encoder global: binding name: proposedVariable].
 
         "If we can't ask the user for correction, make it undeclared"
         self interactive ifFalse: [^encoder undeclared: proposedVariable].
 
         userSelection := cue requestor selectionInterval.
         cue requestor selectFrom: spot first to: spot last.
-        cue requestor select.
 
         "Build the menu with alternatives"
         action := UndeclaredVariable
                                 signalFor: self
                                 name: proposedVariable
                                 inRange: spot.
         action ifNil: [^self fail].
 
+        "Restore the user selection state, but do not display selection yet
+         This will avoid flashing effect when chaining multiple corrections."
+        cue requestor selectIntervalInvisibly: userSelection.
+       
         "Execute the selected action"
-        cue requestor deselect.
-        cue requestor selectInvisiblyFrom: userSelection first to: userSelection last.
         ^action value!

Item was changed:
  ----- Method: Parser>>queryUndefined (in category 'error correction') -----
  queryUndefined
         | varStart varName |
         varName := parseNode key.
         varStart := self endOfLastToken + requestorOffset - varName size + 1.
+        cue requestor selectFrom: varStart to: varStart + varName size - 1.
-        cue requestor selectFrom: varStart to: varStart + varName size - 1; select.
         (UndefinedVariable name: varName) ifFalse: [^ self fail]!

Item was changed:
  ----- Method: Parser>>substituteSelector:wordIntervals: (in category 'error correction') -----
  substituteSelector: selectorParts wordIntervals: spots
+        "Substitute the correctSelector into the (presumed interactive) receiver."
-        "Substitute the correctSelector into the (presuamed interactive) receiver."
         | offset |
         offset := 0.
         selectorParts with: spots do:
                 [ :word :interval |
                 offset := self substituteWord: word wordInterval: interval offset: offset ]
  !