bug: [4993] Typing cmd+L cancel all typing and we can't undo it [Terminated]

This is my first pharo bug fix so be gentle :D

https://pharo.fogbugz.com/f/cases/4993/Typing-cmd-L-cancel-all-typing-and-we-can-t-undo-it

As I explain in the link and in my commit log , I have added a confirm dialog so the user in case he accidentally pressed CMD+L will click no and his changes will not be lost. I have verified the bug fix with the latest pharo build.

kilon wrote: It looks fine.  Simple enough even for me to understand ;).  So I've
changed the status to 'Resolved - fix to include.'  Actually my first,
so just checking this is the correct next step in the workflow ?

On Oct 13, 2013, at 4:10 PM, [hidden email] wrote:
Normally the first step is "fix review needed". Then someone else needs to check if this is ok (not any small mistake, problem really solved), "fix to include" should be the state
when it is guaranteed that is can be included without check and thinking.

        Marcus

As I reviewed the Slice <Changes> related to Case 4993, I notice the
relevant part was only a single method, but also included were several
more unrelated modifications with zero-sum refactorings.  eg, from...
position: aPoint
    super position: aPoint.
    self viewBox ifNotNil: [self viewBox: (aPoint extent: self viewBox
extent)].

to...
position: aPoint
    super position: aPoint.
    self viewBox ifNotNil: [:viewBox | self viewBox: (aPoint extent:  
viewBox extent)].

which itself is cool, but my first thought was that these shouldn't have
been included in the Case 4993 Slice, but in a separate Slice for a
different Case that dealt just with those refactorings.  But thinking
further in general the image I use for testing could have moved on from
the image used to generate that Slice.  So maybe its not not such a
problem anyhow?

Also, if the zero-sum refactoring modifications were put in their own
separate Slice, then probably the specific modifications related to the
Case would build upon that, so ultimately you end up with the same
situation.

So in short my question is, how much do you typically separate out
Case-specific modifications from incidental zero-sum refactoring made
along the way.

cheers -ben

--------Original Message-----------
[hidden email] wrote:

how you do that ? I try to find in the slice the method I have changed and I see it has the whole morphic inside it. Very confusing.

I went to changes and again it does not show the method. Just a class declaration. No idea how to use it to find out the changed method.

I changed only a single method PluggableTextMorph>>cancel .

cancel
        (self confirm:
'This action will cancel your changes.
Is it OK to cancel changes?' translated) ifTrue: [self setText: self getText.
        self setSelection: self getSelection.]

Marcus Denker wrote:

On Oct 13, 2013, at 4:10 PM, [hidden email] wrote:

  

kilon wrote:
    

This is my first pharo bug fix so be gentle :D

https://pharo.fogbugz.com/f/cases/4993/Typing-cmd-L-cancel-all-typing-and-we-can-t-undo-it
<https://pharo.fogbugz.com/f/cases/4993/Typing-cmd-L-cancel-all-typing-and-we-can-t-undo-it>  
As I explain in the link and in my commit log , I have added a confirm
dialog so the user in case he accidentally pressed CMD+L will click no and
his changes will not be lost. I have verified the bug fix with the latest
pharo build. 


--
View this message in context: http://forum.world.st/bug-4993-Typing-cmd-L-cancel-all-typing-and-we-can-t-undo-it-Terminated-tp4714154.html
Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.


 
      

It looks fine.  Simple enough even for me to understand ;).  So I've changed the status to 'Resolved - fix to include.'  Actually my first, so just checking this is the correct next step in the workflow ?

    

Normally the first step is "fix review needed". Then someone else needs to check if this is ok (not any small mistake, problem really solved), "fix to include" should be the state
when it is guaranteed that is can be included without check and thinking.

	Marcus
  

After loading the slice, as well as some functional testing, would you typically run all tests in TestRunner, or just those associated with the modified package ? 

Also at which point does the monkey do its checking?

cheers -ben

On Oct 13, 2013, at 7:11 PM, [hidden email] wrote:
>>
>> Normally the first step is "fix review needed". Then someone else needs to check if this is ok (not any small mistake, problem really solved), "fix to include" should be the state
>> when it is guaranteed that is can be included without check and thinking.
>>
>> Marcus
>>  
>
> After loading the slice, as well as some functional testing, would you typically run all tests in TestRunner, or just those associated with the modified package ?  
>
running tests is slow, so in the end people (hum, that is, me ;-) forget to even run the tests of the package.

> Also at which point does the monkey do its checking?
>

Sadly the monkey is sick right now… we need to fix it.

        Marcus

On Oct 13, 2013, at 6:36 PM, [hidden email] wrote:
that nobody used in 20 years, can I remove it?" The answer always will be "Wow, this is cool, over my dead body!". If people would know what I removed over the years… ;-)

        Marcus

On Oct 13, 2013, at 7:00 PM, kilon <[hidden email]> wrote:

> how you do that ? I try to find in the slice the method I have changed and I
> see it has the whole morphic inside it. Very confusing.
>

Monticello always snapshots the whole package even for a one-character change.

        Marcus

On Oct 13, 2013, at 1:08 PM, kilon <[hidden email]> wrote:

> This is my first pharo bug fix so be gentle :D

Great!
We love contributions.

Stef

Marcus Denker wrote:

On Oct 13, 2013, at 7:00 PM, kilon [hidden email] wrote:

  

how you do that ? I try to find in the slice the method I have changed and I
see it has the whole morphic inside it. Very confusing. 

    

Did you use the <Browse> button or <Changes> button.  The <Changes> button compares to the current slice. So if you make the slice in a fresh image (eg #30444), then if you unzip a new #30444 from which you review the slice for <Changes> you should see only your change.  However someone else using another image (eg #30450) that happens to have changes in packages overlapping your slice, the they will see those as changes when reviewing the slice.  It is of course important to see ALL changes a slices is going to make to an image, but as a "change review task" it seems very useful to see "only" the changes from one package version to the next.  As I understand it this is not currently possible - although I'd be very pleased to be correct.

For individual packages the way you compare differences between particular versions is with the <History>.  For Slices this seems useless, since there is no history.  It might be useful for this to be changed to show the history of each dependent package of the Slice?

Alternatively, comparing Versions and Monticello, it seems that:
* Versions defaults to showing the "changes to immediately previous version" plus a <Compare to current> button. 
* Monticello <Changes> button effectively defaults to "compare to current" while "changes to immediately previous" is hard to access.
I wonder if for consistency the existing Monitcello <Changes> button could be changed to <Compare to current> and a new <Changes> button be added that focuses only on the changes made for a slice to revolve a particular Issue.

Monticello always snapshots the whole package even for a one-character change.
  

As far as my cursor understand of fit goes, it works similar.
-ben

	Marcus

  

Hi!

Thanks Kilon for working on the Contr-l key binding. I use this control key binding a lot, and actually, it is really rare when I do a mistake. Having to press OK whenever I do this sounds like a regression to me. Why not always inserting the text in the undo queue before replacing the text?

Alexandre

On Oct 14, 2013, at 2:05 PM, Alexandre Bergel <[hidden email]> wrote:

> Hi!
>
> Thanks Kilon for working on the Contr-l key binding. I use this control key binding a lot, and actually, it is really rare when I do a mistake.

I did not even know that it exist, but I wondered often when the code I wrote disappeared with just no way of getting it back.

> Having to press OK whenever I do this sounds like a regression to me. Why not always inserting the text in the undo queue before replacing the text?
>

This would be best…

Definetly not a regression. If you make a mistake your work is gone, bye bye, hasta la vista baby. Personally I dont use, or see myself using CMD+L on a daily basis. Its highly unlikely that I will code in several lines of code and will want to discard them completely. In 99/100 cases I just slightly change, edit the code. So we definitely have a different workflow on this one.

You are much better coders than me, so I understand you may use that feature heavily.

Undo was my first goal too, but trying to understand how undo is implemented felt like hammering my face on a wall. The amount of spaggetication in the code is beyond understanding for me, on the 1 hour I invested of finding a bug fix. So I spent like 50 minutes trying to understand undo, failed miserably, spent rest 10 implementing the dialog box. And yes I am not very smart or good coder.

I will try to take another look at undo today and have another go at this, but If I found no solution I will leave the fix as it is. Chances are I wont figure this out, so don't hold your breath.

Bottom line is that in case of proper software the user should not allowed to do things that are unrecoverable without a prompt. So not implementing a fix at all, at least for me is considered very bad. But this is just my personal opinion.

On Oct 14, 2013, at 3:06 PM, kilon <[hidden email]> wrote:
I am 100% with you on this :-)

        Marcus

Alexandre Bergel wrote:

Hi!

Thanks Kilon for working on the Contr-l key binding. I use this control key binding a lot, and actually, it is really rare when I do a mistake. Having to press OK whenever I do this sounds like a regression to me. Why not always inserting the text in the undo queue before replacing the text?

Alexandre
  

I originally had the same opinion, and I had a go getting undo to work but failed.  So I tested kilon's slice and it seemed to not adversely affect my workflow.  I wonder if a compromise might be the ability to acknowledge the alert dialog with <ctrl-l> - so you don't have to move your fingers from the shortcut, and still provides a guard against accidental reset.
-ben

Le 13-10-2013 à 8:08, kilon [hidden email] a écrit :

This is my first pharo bug fix so be gentle :D

https://pharo.fogbugz.com/f/cases/4993/Typing-cmd-L-cancel-all-typing-and-we-can-t-undo-it
<https://pharo.fogbugz.com/f/cases/4993/Typing-cmd-L-cancel-all-typing-and-we-can-t-undo-it>  

As I explain in the link and in my commit log , I have added a confirm
dialog so the user in case he accidentally pressed CMD+L will click no and
his changes will not be lost. I have verified the bug fix with the latest
pharo build. 



--
View this message in context: http://forum.world.st/bug-4993-Typing-cmd-L-cancel-all-typing-and-we-can-t-undo-it-Terminated-tp4714154.html
Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.

    

the dialog does not force you to use the mouse, you just do ctrl+l and then hit enter to confirm ("yes" is preselected). So essentially it only introduces one more keyboard key, enter. If you want to cancel the cmd+l is when you will need to use the mouse (so you choose "No" option). Apparently arrow keys dont work with the dialog.

So you can think of it as ctrl+l+enter . Saying that I will have another go on undo , I really want to understand how undo works not just for this fix but generally.  I agree that the fix is not ideal as it is but unfortunately my understanding of Pharo is very limited right now.

btc wrote

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">


 
 


Alexandre Bergel wrote:

  Hi!

Thanks Kilon for working on the Contr-l key binding. I use this control key binding a lot, and actually, it is really rare when I do a mistake. Having to press OK whenever I do this sounds like a regression to me. Why not always inserting the text in the undo queue before replacing the text?

Alexandre
 


I originally had the same opinion, and I had a go getting undo to work
but failed.  So I tested kilon's slice and it seemed to not adversely
affect my workflow.  I wonder if a compromise might be the ability to
acknowledge the alert dialog with <ctrl-l> - so you don't have to
move your fingers from the shortcut, and still provides a guard against
accidental reset.
-ben


 
 
 
    Le 13-10-2013 à 8:08, kilon <[hidden email]>  a écrit :

This is my first pharo bug fix so be gentle :D

https://pharo.fogbugz.com/f/cases/4993/Typing-cmd-L-cancel-all-typing-and-we-can-t-undo-it 
<https://pharo.fogbugz.com/f/cases/4993/Typing-cmd-L-cancel-all-typing-and-we-can-t-undo-it>   

As I explain in the link and in my commit log , I have added a confirm
dialog so the user in case he accidentally pressed CMD+L will click no and
his changes will not be lost. I have verified the bug fix with the latest
pharo build.



--
View this message in context: http://forum.world.st/bug-4993-Typing-cmd-L-cancel-all-typing-and-we-can-t-undo-it-Terminated-tp4714154.html 
Sent from the Pharo Smalltalk Developers mailing list archive at Nabble.com.

On Oct 14, 2013, at 5:13 PM, kilon <[hidden email]> wrote:
We should add this now and then the people who use ctrl-l a lot can do
a fix with putting the content in the undo.

        Marcus

Hi Kilon,

Sorry for the late reply. The Cmd-L is useful as soon as you evaluate code in your code browser. Consider the following scenario that I am very sure you am facing every single day:

1 - Define a method on a class:
-=-=--=-=--=-=--=-=-
foo
        ^ 1 + 2
-=-=--=-=--=-=--=-=-

2 - It often happen that you want to know the value of some expression in the method body. Select "1 + 2" and press Cmd-P. It prints you the value 3, great.

3 - Click on a different method or close the browser. And you get this stupid question "Code has been modified. What do you want to do". Isn't it obvious what I want to do? The browser is asking me whether I may want to accept the method, even if it is not compilable (i.e., the code "^ 1 + 2 3" is not proper smalltalk). So stupid it is. Even worse: if you press backspace to remove the 3, the code browser is still notifying you the source code has changed, even if the content is the same.

This is so frustrating. I am sure you are facing this questions many times a day. Cmd-L is here to help (but it poorly helps, I agree). After you have evaluated "1 + 2" and have "3" displayed, you can press Cmd-L to restore the original method content and makes the browser forget that I have modified the method body. No more question is asked when I delete the browser or select a different method.

Now, making Cmd-L raises a question would simply makes the bidding not useful at all. Igor showed me this bidding, which makes me suspect he uses it since he knows it. I think Lukas was using it as well. Making the browser ask yet another question such as "your content is about to be deleted, are you sure you want to continue" does not make the environment more intelligent in my opinion, but just more talkative.

As your original post suggest, the text editor needs improvement. For example, if Cmd-L is really disturbing you, then remove it, but make sure the browser does not complain when the text content is the same than the original method (or class definition). This would be great actually.

On a slightly different front, I have seen that the create temporary method is broken. Last time I've seen this was many years ago...

Alexandre


On Oct 14, 2013, at 10:05 AM, kilon <[hidden email]> wrote:

--
_,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:
Alexandre Bergel  http://www.bergel.eu
^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.

I use Cmd-L almost as often than Cmd-P. Making it asking an obvious question which require a obvious answer is counter productive.
If one is really afraid to loose the content, then use the Undo queue for this. So, Cmd-L should use the Undo queue, not asking more questions.

Alexandre


On Oct 14, 2013, at 10:41 AM, [hidden email] wrote:

--
_,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:
Alexandre Bergel  http://www.bergel.eu
^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.