[Pharo-project] selectionForDoitAsStream smells
Compare this which is like old squeak selectionAsStream SmalltalkEditor>>selectionForDoitAsStream ^ ReadWriteStream on: self string from: self startIndex to: self stopIndex - 1 and this which has been changed conforming to Cuis TextEditor>>selectionAsStream "Answer a ReadStream on the text in the paragraph that is currently selected." ^ReadStream on: (self string copyFrom: self startIndex to: self stopIndex - 1) (or self selection asString readStream) The two differ. The former stream position starts at editor startIndex - 1, while the later stream position starts at 0. The stream position is used by the Compiler/Parser for notifying the SmalltalkEditor (or TextMorph) requestor. Now look at SmalltalkEditor>>notify: aString at: anInteger in: aStream "The compilation of text failed. The syntax error is noted as the argument, aString. Insert it in the text at starting character position anInteger." | pos | pos := self selectionInterval notEmpty ifTrue: [self startIndex + anInteger - 1 ] ifFalse: [anInteger]. self insertAndSelect: aString at: (pos max: 1) You see that it adds (startIndex - 1) to the error message insertion position which is a Cuis change. (The old ParagraphEditor did not). When #notify:at:in: is used for evaluating #selectionAsStream, all is OK, the (startIndex - 1) is added only once. But when it is used with #selectionForDoitAsStream, the offset is counted twice (because already in the stream location passed in anInteger parameter). I guess the hack #selectionForDoitAsStream that undo the Cuis clean-up has been added because some other part of Parser/Encoder/Compiler is messing with the requestor selection (like highlighting some part of the source for user interaction in case of ambiguity). Tthis is not a valid workaround. In Squeak, I did revert the Cuis clean-up just to keep some simple compatibility with ParagraphEditor, but Pharo does not care about that, so it should apply full simplification. I suggest removing #selectionForDoitAsStream, and replace senders with #selectionAsStream. Then if there are some other feature broken we'll fix'em. OK? Nicolas
On Feb 26, 2012, at 9:41 PM, Nicolas Cellier wrote:
Compare this which is like old squeak selectionAsStream
SmalltalkEditor>>selectionForDoitAsStream ^ ReadWriteStream on: self string from: self startIndex to: self stopIndex - 1
and this which has been changed conforming to Cuis
TextEditor>>selectionAsStream "Answer a ReadStream on the text in the paragraph that is currently selected."
^ReadStream on: (self string copyFrom: self startIndex to: self stopIndex - 1)
(or self selection asString readStream) The two differ. The former stream position starts at editor startIndex - 1, while the later stream position starts at 0. The stream position is used by the Compiler/Parser for notifying the SmalltalkEditor (or TextMorph) requestor.
Now look at SmalltalkEditor>>notify: aString at: anInteger in: aStream "The compilation of text failed. The syntax error is noted as the argument, aString. Insert it in the text at starting character position anInteger."
| pos | pos := self selectionInterval notEmpty ifTrue: [self startIndex + anInteger - 1 ] ifFalse: [anInteger]. self insertAndSelect: aString at: (pos max: 1)
You see that it adds (startIndex - 1) to the error message insertion position which is a Cuis change. (The old ParagraphEditor did not).
When #notify:at:in: is used for evaluating #selectionAsStream, all is OK, the (startIndex - 1) is added only once. But when it is used with #selectionForDoitAsStream, the offset is counted twice (because already in the stream location passed in anInteger parameter).
I guess the hack #selectionForDoitAsStream that undo the Cuis clean-up has been added because some other part of Parser/Encoder/Compiler is messing with the requestor selection (like highlighting some part of the source for user interaction in case of ambiguity). Tthis is not a valid workaround.
In Squeak, I did revert the Cuis clean-up just to keep some simple compatibility with ParagraphEditor, but Pharo does not care about that, so it should apply full simplification. I suggest removing #selectionForDoitAsStream, and replace senders with #selectionAsStream. Then if there are some other feature broken we'll fix'em. OK?
Sure!!!
Nicolas
One of the area of high interaction is Parser>>correctSelector:wordIntervals:exprInterval:ifAbort: which has to then to deal with (self startIndex - 1) offset. It will have to be fixed too. Nicolas Le 26 février 2012 21:54, Stéphane Ducasse <stephane.ducasse@inria.fr> a écrit :
On Feb 26, 2012, at 9:41 PM, Nicolas Cellier wrote:
Compare this which is like old squeak selectionAsStream
SmalltalkEditor>>selectionForDoitAsStream    ^ ReadWriteStream        on: self string        from: self startIndex        to: self stopIndex - 1
and this which has been changed conforming to Cuis
TextEditor>>selectionAsStream    "Answer a ReadStream on the text in the paragraph that is currently    selected."
   ^ReadStream        on: (self string copyFrom: self startIndex to: self stopIndex - 1)
(or self selection asString readStream) The two differ. The former stream position starts at editor startIndex - 1, while the later stream position starts at 0. The stream position is used by the Compiler/Parser for notifying the SmalltalkEditor (or TextMorph) requestor.
Now look at SmalltalkEditor>>notify: aString at: anInteger in: aStream    "The compilation of text failed. The syntax error is noted as the argument,    aString. Insert it in the text at starting character position anInteger."
   | pos |    pos := self selectionInterval notEmpty        ifTrue: [self startIndex + anInteger - 1 ]        ifFalse: [anInteger].    self insertAndSelect: aString at: (pos max: 1)
You see that it adds (startIndex - 1) to the error message insertion position which is a Cuis change. (The old ParagraphEditor did not).
When #notify:at:in: is used for evaluating #selectionAsStream, all is OK, the (startIndex - 1) is added only once. But when it is used with #selectionForDoitAsStream, the offset is counted twice (because already in the stream location passed in anInteger parameter).
I guess the hack #selectionForDoitAsStream that undo the Cuis clean-up has been added because some other part of Parser/Encoder/Compiler is messing with the requestor selection (like highlighting some part of the source for user interaction in case of ambiguity). Tthis is not a valid workaround.
In Squeak, I did revert the Cuis clean-up just to keep some simple compatibility with ParagraphEditor, but Pharo does not care about that, so it should apply full simplification. I suggest removing #selectionForDoitAsStream, and replace senders with #selectionAsStream. Then if there are some other feature broken we'll fix'em. OK?
Sure!!!
Nicolas
http://code.google.com/p/pharo/issues/detail?id=5406 SmalltalkEditor>>notify:at:in: is wrong and has been reverted in Cuis too. I have a plan explained in above issue if we want to get rid of ReadStream on:from:to: Nicolas Le 26 février 2012 21:57, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com> a écrit :
One of the area of high interaction is Parser>>correctSelector:wordIntervals:exprInterval:ifAbort: which has to then to deal with (self startIndex - 1) offset. It will have to be fixed too.
Nicolas
Le 26 février 2012 21:54, Stéphane Ducasse <stephane.ducasse@inria.fr> a écrit :
On Feb 26, 2012, at 9:41 PM, Nicolas Cellier wrote:
Compare this which is like old squeak selectionAsStream
SmalltalkEditor>>selectionForDoitAsStream    ^ ReadWriteStream        on: self string        from: self startIndex        to: self stopIndex - 1
and this which has been changed conforming to Cuis
TextEditor>>selectionAsStream    "Answer a ReadStream on the text in the paragraph that is currently    selected."
   ^ReadStream        on: (self string copyFrom: self startIndex to: self stopIndex - 1)
(or self selection asString readStream) The two differ. The former stream position starts at editor startIndex - 1, while the later stream position starts at 0. The stream position is used by the Compiler/Parser for notifying the SmalltalkEditor (or TextMorph) requestor.
Now look at SmalltalkEditor>>notify: aString at: anInteger in: aStream    "The compilation of text failed. The syntax error is noted as the argument,    aString. Insert it in the text at starting character position anInteger."
   | pos |    pos := self selectionInterval notEmpty        ifTrue: [self startIndex + anInteger - 1 ]        ifFalse: [anInteger].    self insertAndSelect: aString at: (pos max: 1)
You see that it adds (startIndex - 1) to the error message insertion position which is a Cuis change. (The old ParagraphEditor did not).
When #notify:at:in: is used for evaluating #selectionAsStream, all is OK, the (startIndex - 1) is added only once. But when it is used with #selectionForDoitAsStream, the offset is counted twice (because already in the stream location passed in anInteger parameter).
I guess the hack #selectionForDoitAsStream that undo the Cuis clean-up has been added because some other part of Parser/Encoder/Compiler is messing with the requestor selection (like highlighting some part of the source for user interaction in case of ambiguity). Tthis is not a valid workaround.
In Squeak, I did revert the Cuis clean-up just to keep some simple compatibility with ParagraphEditor, but Pharo does not care about that, so it should apply full simplification. I suggest removing #selectionForDoitAsStream, and replace senders with #selectionAsStream. Then if there are some other feature broken we'll fix'em. OK?
Sure!!!
Nicolas
There is also http://code.google.com/p/pharo/issues/detail?id=5404 in the same corner. Give you a pleasure, remove these 5 lines of code! Le 27 février 2012 00:58, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com> a écrit :
http://code.google.com/p/pharo/issues/detail?id=5406
SmalltalkEditor>>notify:at:in: is wrong and has been reverted in Cuis too.
I have a plan explained in above issue if we want to get rid of ReadStream on:from:to:
Nicolas
Le 26 février 2012 21:57, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com> a écrit :
One of the area of high interaction is Parser>>correctSelector:wordIntervals:exprInterval:ifAbort: which has to then to deal with (self startIndex - 1) offset. It will have to be fixed too.
Nicolas
Le 26 février 2012 21:54, Stéphane Ducasse <stephane.ducasse@inria.fr> a écrit :
On Feb 26, 2012, at 9:41 PM, Nicolas Cellier wrote:
Compare this which is like old squeak selectionAsStream
SmalltalkEditor>>selectionForDoitAsStream    ^ ReadWriteStream        on: self string        from: self startIndex        to: self stopIndex - 1
and this which has been changed conforming to Cuis
TextEditor>>selectionAsStream    "Answer a ReadStream on the text in the paragraph that is currently    selected."
   ^ReadStream        on: (self string copyFrom: self startIndex to: self stopIndex - 1)
(or self selection asString readStream) The two differ. The former stream position starts at editor startIndex - 1, while the later stream position starts at 0. The stream position is used by the Compiler/Parser for notifying the SmalltalkEditor (or TextMorph) requestor.
Now look at SmalltalkEditor>>notify: aString at: anInteger in: aStream    "The compilation of text failed. The syntax error is noted as the argument,    aString. Insert it in the text at starting character position anInteger."
   | pos |    pos := self selectionInterval notEmpty        ifTrue: [self startIndex + anInteger - 1 ]        ifFalse: [anInteger].    self insertAndSelect: aString at: (pos max: 1)
You see that it adds (startIndex - 1) to the error message insertion position which is a Cuis change. (The old ParagraphEditor did not).
When #notify:at:in: is used for evaluating #selectionAsStream, all is OK, the (startIndex - 1) is added only once. But when it is used with #selectionForDoitAsStream, the offset is counted twice (because already in the stream location passed in anInteger parameter).
I guess the hack #selectionForDoitAsStream that undo the Cuis clean-up has been added because some other part of Parser/Encoder/Compiler is messing with the requestor selection (like highlighting some part of the source for user interaction in case of ambiguity). Tthis is not a valid workaround.
In Squeak, I did revert the Cuis clean-up just to keep some simple compatibility with ParagraphEditor, but Pharo does not care about that, so it should apply full simplification. I suggest removing #selectionForDoitAsStream, and replace senders with #selectionAsStream. Then if there are some other feature broken we'll fix'em. OK?
Sure!!!
Nicolas
participants (2)
-
Nicolas Cellier -
Stéphane Ducasse