The Inbox: Morphic-ct.1606.mcz

Am 07.12.2019 16:53:39 schrieb [hidden email] <[hidden email]>:

A new version of Morphic was added to project The Inbox:
http://source.squeak.org/inbox/Morphic-ct.1606.mcz

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

Name: Morphic-ct.1606
Author: ct
Time: 7 December 2019, 4:53:17.106431 pm
UUID: 83ec1d59-dfa2-ca40-806d-809e76232100
Ancestors: Morphic-mt.1604

Restore original selection interval after any kind of do-it.

This is important if you print-it the following and skip the compiler hint:

| x |
x.
2

Before this commit, the answer was displayed just after x, not at the end of the snippet.

=============== Diff against Morphic-mt.1604 ===============

Item was changed:
----- Method: TextEditor>>evaluateSelectionAndDo: (in category 'do-its') -----
evaluateSelectionAndDo: aBlock
"Treat the current selection as an expression; evaluate it and invoke aBlock with the result."
+ | selectionInterval result rcvr ctxt |
- | result rcvr ctxt |
self lineSelectAndEmptyCheck: [^ nil].
+ selectionInterval := self selectionInterval.

(model respondsTo: #evaluateExpression:) ifTrue: [
^ aBlock value: (model perform: #evaluateExpression: with: self selection)].

(model respondsTo: #doItReceiver)
ifTrue: [ rcvr := model doItReceiver.
ctxt := model doItContext]
ifFalse: [rcvr := ctxt := nil].
result := [
rcvr class evaluatorClass new
evaluate: self selectionAsStream
in: ctxt
to: rcvr
environment: (model environment ifNil: [Smalltalk globals])
notifying: self
ifFail: [morph flash. ^ nil]
logged: true.
]
on: OutOfScopeNotification
do: [ :ex | ex resume: true].
+ self selectInterval: selectionInterval.
+
-
(model respondsTo: #expressionEvaluated:result:) ifTrue: [
model perform: #expressionEvaluated:result: with: self selection with: result].

^aBlock value: result!

Hi Marcel,

could you explain what you mean with "within-statement print-its"? This commit only ensures that after evaluating a section, the previous selection is restored. I could only think of the scenario that you turn off modal exclusivity of a dialog window and change something in the CodeHolder while the compiler is running. Do we want to support this scenario?

Best,

Christoph

Am 09.12.2019 05:55:09 schrieb tim Rowledge <[hidden email]>:

> On 2019-12-08, at 1:11 PM, Thiede, Christoph wrote:
>
> I think I see what you mean, but did you try loading the commit? :)


No; I was about to leave for my flying club AGM, so didn't have time to do that. I was working from the descriptions.

> #printIt calls #evaluateSelectionAndDo:, and in the specified do block, it prints out the result and adjusts the selection to the output. So this will keep working fine. Or am I misunderstanding you?

No, that is what should happen in the normal case. The issue is what happens when there is an undefined (autocorrect wanted 'undefiled', which seems somehow very appropriate) variable in the text being evaluated. Me personally, I'd prefer a debugger in most cases, but I suppose this is an attempt to be helpful.

>
> Further, you are suggesting that we should only change the selection if the user cancels the compilation on a warning.

Not quite; the selection *at the end* should only be changed if the user chooses to stop. At that point it is quite reasonable to have errant variable name selected. And logically I'd have to agree that it is reasonable to do the selection as the notifier is opened. I think I'd prefer even more a secondary error-selection in a different colour but that would involve all sorts of other UI changes that might be problematic, so let's leave that aside.

> (By the way, this selection is made in Parser >> #queryUndefined, for example. You can find out this quite easy by debugging the invocation of the dialog window and peeling back the stack.)

Yah. The number of times I've done that over the last 4 decades probably needs a LargePositiveInteger to express.

> So my point is that the selection should be changed whenever a parser notification is raised.
> Ergo, the selection might change at some point during parsing/compiling. If the user cancels the operation, this selection should stay so the user can fix the code directly. Otherwise, the warning is skipped and the temporary selection is no longer relevant to the user, so we should restore the previous selection.
>
> What am I missing? :-)

A clean way to do that. Personally I'd prefer to see the selection fiddling removed from the #queryUndefined method and have that left to a handling method that knows it is working with a UI. So at a wild, late on a Sunday, guess, consider -
TextEditor>evaluateSelectionAndDo: aBlock
{fiddle with model respondsTo: crap that shouldn't be there either}
result := [wibbley-wobbley-timey-wimey-compile-and-do]
on: OutOfScopeNotification
do:[ {stuff - but why just resume if something is out of scope?} ]
on: UndefinedVariable
do: [:ex|
origSelection := self selectionInterval.
self selectInterval: ex selectionInterval "obviously the signal needs to carry this up from #queryUndefined"
answer := ex pass. "to get the answer from UI"
answer ifTrue:[self selectionInterval: origSelectin "may have logic backwards here"].
ex resume: answer]
{more weird respondsTo: faffing}

Why like this? It gathers together all the UI stuff in one place where the UI is a major player. It relieves the low-level compiler code of UI connections. It only changes the selection if it actually needs to (and remember that changing bits on glass is still relatively expensive). And it use on:do:on:do: - how often do we get to do layered exception handling?

{And the #respondsTo: stuff? Yuck. Those searches up the class chain scanning every method dictionary are expensive. #isKindOf: is massively cheaper and removing *those* was one of the things that I did to make Scratch many, many, times faster for the Pi. It would be so much nicer to avoid that construct }

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Any Sufficiently Advanced Incompetence Is Indistinguishable From Malice

Am 09.12.2019 10:02:25 schrieb Marcel Taeumel <[hidden email]>:

Hi Christoph,

I understand now. The fix is fine. However, both your description and picture are misleading. :-)

1. The description: " ... after evaluating a section, the previous selection is restored ..."

What you actually mean is after evaluating a selection and *before* printing the result.

2. The picture: 

Why is the "2" on an extra row? It looks like a print-out, which usually goes in the same line. Well, the "x" is undefined, which makes that "2" totally unrelated to the example.

***

Here is what it does:

Best,

Marcel

Am 09.12.2019 05:55:09 schrieb tim Rowledge <[hidden email]>:

> On 2019-12-08, at 1:11 PM, Thiede, Christoph wrote:
>
> I think I see what you mean, but did you try loading the commit? :)


No; I was about to leave for my flying club AGM, so didn't have time to do that. I was working from the descriptions.

> #printIt calls #evaluateSelectionAndDo:, and in the specified do block, it prints out the result and adjusts the selection to the output. So this will keep working fine. Or am I misunderstanding you?

No, that is what should happen in the normal case. The issue is what happens when there is an undefined (autocorrect wanted 'undefiled', which seems somehow very appropriate) variable in the text being evaluated. Me personally, I'd prefer a debugger in most cases, but I suppose this is an attempt to be helpful.

>
> Further, you are suggesting that we should only change the selection if the user cancels the compilation on a warning.

Not quite; the selection *at the end* should only be changed if the user chooses to stop. At that point it is quite reasonable to have errant variable name selected. And logically I'd have to agree that it is reasonable to do the selection as the notifier is opened. I think I'd prefer even more a secondary error-selection in a different colour but that would involve all sorts of other UI changes that might be problematic, so let's leave that aside.

> (By the way, this selection is made in Parser >> #queryUndefined, for example. You can find out this quite easy by debugging the invocation of the dialog window and peeling back the stack.)

Yah. The number of times I've done that over the last 4 decades probably needs a LargePositiveInteger to express.

> So my point is that the selection should be changed whenever a parser notification is raised.
> Ergo, the selection might change at some point during parsing/compiling. If the user cancels the operation, this selection should stay so the user can fix the code directly. Otherwise, the warning is skipped and the temporary selection is no longer relevant to the user, so we should restore the previous selection.
>
> What am I missing? :-)

A clean way to do that. Personally I'd prefer to see the selection fiddling removed from the #queryUndefined method and have that left to a handling method that knows it is working with a UI. So at a wild, late on a Sunday, guess, consider -
TextEditor>evaluateSelectionAndDo: aBlock
{fiddle with model respondsTo: crap that shouldn't be there either}
result := [wibbley-wobbley-timey-wimey-compile-and-do]
on: OutOfScopeNotification
do:[ {stuff - but why just resume if something is out of scope?} ]
on: UndefinedVariable
do: [:ex|
origSelection := self selectionInterval.
self selectInterval: ex selectionInterval "obviously the signal needs to carry this up from #queryUndefined"
answer := ex pass. "to get the answer from UI"
answer ifTrue:[self selectionInterval: origSelectin "may have logic backwards here"].
ex resume: answer]
{more weird respondsTo: faffing}

Why like this? It gathers together all the UI stuff in one place where the UI is a major player. It relieves the low-level compiler code of UI connections. It only changes the selection if it actually needs to (and remember that changing bits on glass is still relatively expensive). And it use on:do:on:do: - how often do we get to do layered exception handling?

{And the #respondsTo: stuff? Yuck. Those searches up the class chain scanning every method dictionary are expensive. #isKindOf: is massively cheaper and removing *those* was one of the things that I did to make Scratch many, many, times faster for the Pi. It would be so much nicer to avoid that construct }

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Any Sufficiently Advanced Incompetence Is Indistinguishable From Malice