Opened 7 years ago

Last modified 6 years ago

#124 needs_work defect

Selecting a text with mouse and pasting does not replace the text, it adds to it.

Reported by: patrik.svestka@… Owned by:
Priority: major Milestone: 8.1.0
Component: default Keywords: EditTextView
Cc: Also affects CVS HEAD (eXept version): no

Description

How to reproduce? (the issue is in build #2480 the previously build StX works correctly - don't know the version as the Smalltalk versionBuildNumber returns empty string)

1) Open a workspace
2) Enter any code you wish
3) copy some text
4) select the text with mouse
5) paste it with ctrl+v (on windows)

Change History (29)

comment:1 by jan vrany, 7 years ago

Status: newtesting

This one is funny.

The problem is (was) that EditTextView>>paste actually cleared the selection before pasting, for whatever reason.

Last change to that code was done 4 years ago - but noone noticed because noone pasted the text using menu pick ('Edit' -> 'Paste'). However, with commit ce68ee978f93/stx.libtool, pasting through shortcut started using menu pick handling code (as application-defined shortcuts have always precedence over widget-defined shortuts).

Should be fixed in f0f9120765af/stx.libwidg

comment:2 by patrik.svestka@…, 7 years ago

This patch solves the issue with pasting, but still is not 100% perfect.

When performing a paste and then undo, 5 characters are left from the pasted text (this works for specific text like the one below; sometimes it works flawlessly):

"Paste copybuffer (clipboard)"
 self pasteOrReplace

 	"Modified: / 12-03-2017 / 21:46:59 / Jan Vrany <jan.vrany@fit.cvut.cz>"

comment:3 by jan vrany, 7 years ago

Right, I was too quick pushing this, too sure it would fix the problem. My bad. So I reverted the commit (see da4954e52fd3/stx.libwidg) and will try again.

Let's start with some tests first. This is the first test, does it assert behavior you'd expect (hope you can read it)?

test_issue124_01

    textView contents: '1234'.
    textView setCursorCol: 3.
    textView selectToEndOfLine.
    self assert: textView selectionAsString = '34'.

    textView setClipboardText: 'Smalltalk'.
    textViewInteractor type: #Paste. "/ this simulates pressing Ctrl-V
    self assert: textView selectionAsString = 'Smalltalk'.
    self assert: textView contents asString = ('12Smalltalk' , Character cr).

    textViewInteractor type: #Undo. "/ This simulates pressing Ctrl-Z
    self assert: textView contents asString = ('1234' , Character cr).

    "Created: / 13-03-2017 / 20:51:44 / Jan Vrany <jan.vrany@fit.cvut.cz>"

If you have more examples, please elaboreate. Then let's turn them into tests. Then I can debug the code and - eventually - fix it.

comment:4 by jan vrany, 7 years ago

Milestone: 8.0.0

comment:5 by jan vrany, 7 years ago

Status: testingneeds_work

comment:6 by patrik.svestka@…, 7 years ago

The logic is correct. Your above test will pass without issues. For now I have found issue with the string I have written above (did not find any connection to the old ones). The test would then look like this:

test_issue124_02

    textView contents: '1234'.
    textView setCursorCol: 3.
    textView selectToEndOfLine.
    self assert: textView selectionAsString = '34'.

    textView setClipboardText: '"Paste copybuffer (clipboard)"
                                   self pasteOrReplace

 	"Modified: / 12-03-2017 / 21:46:59 / Jan Vrany <jan.vrany@fit.cvut.cz>"'.
    textViewInteractor type: #Paste. "/ this simulates pressing Ctrl-V
    self assert: textView selectionAsString = "Paste copybuffer (clipboard)"
 self pasteOrReplace

 	"Modified: / 12-03-2017 / 21:46:59 / Jan Vrany <jan.vrany@fit.cvut.cz>"''.
    self assert: textView contents asString = ('12"Paste copybuffer (clipboard)"
 self pasteOrReplace

 	"Modified: / 12-03-2017 / 21:46:59 / Jan Vrany <jan.vrany@fit.cvut.cz>"' , Character cr).

    textViewInteractor type: #Undo. "/ This simulates pressing Ctrl-Z
    self assert: textView contents asString = ('1234' , Character cr).

I don't know how to correctly test multi-line value. The above code is just approximation.

Last edited 7 years ago by patrik.svestka@… (previous) (diff)

comment:7 by patrik.svestka@…, 7 years ago

I have during time to find another string found another bug in the text section process and in subsequent typing. The issue here is if you have a text (select it) and then paste a multi-line block and immediately start typing the text typed will appear somewhere else than the displayed cursor :) (both issues also affects linux version).

I'll try to write a test (would be nice to have a tutorial about tests .], I know there is still unfinished GUI testing framework (TM) .])

test_issue124_03

    textView contents: '1234'.
    textView setCursorCol: 3.
    textView selectToEndOfLine.
    self assert: textView selectionAsString = '34'.

    textView setClipboardText: '"ahoj"
Bla
"/ / / test"'.
    textViewInteractor type: #Paste. "/ this simulates pressing Ctrl-V
    self assert: textView selectionAsString = '"ahoj"
Bla
"/ / / test"'.
    self assert: textView contents asString = ('12"ahoj"
Bla
"/ / / test"' , Character cr).

    textView setCursorCol: 2.
    textView selectUpToEnd.
    self assert: textView selectionAsString = '2"ahoj"
Bla
"/ / / test"'.
    textViewInteractor type: 'Simsalabim'.
    self assert: textView contents asString = ('1Simsalabim' , Character cr).

Again this is my approximation in testing multi-line strings.

comment:8 by jan vrany, 7 years ago

As for tests in general, it's all SUnit, a test framework included in pretty much every Smalltalk with slight modifications (as everyone think he knows how to write test frameworks and noone's [*] interested in merging it).

So you may have a quick look at Pharo by Example, Chapter 9 SUnit. Just ignore the word Pharo (think of
Smalltalk instead) and keep in mind that UI for running tests is little different (better, indeed :-) I believe you can work out how to run tests from Smalltalk/X IDE, if not then it means the UI is badly designed...

Help on Still Unfinished UI Testing Framework (TM) is a different story, though.

comment:9 by jan vrany, 7 years ago

Huh, you just have opened Pandora's box. It looks to me that there are at least three independen problems:

  • event processing does not invoke same code as using API ('Ctrl-V' vs. #paste)
  • undo mechanism is broken and / or not behaving as expected. Namely undo transaction / action merging is not working as expected. See UndoSupport >> closeTransactionAndAddTo:
  • selection and cursor position invariant IS broken (not always true)

It will take a lot of effort to get it right, I'm afraid.

comment:10 by jan vrany, 7 years ago

As for the bug related to leftovers after undo, I have added some tests (see patch).

The problem is tab/space expansin and namely in method

withoutRedrawInsertStringWithoutCRs:aString atLine:lineNr col:colNr
    "insert aString (which has no crs) at lineNr/colNr"

    self basicWithoutRedrawInsertStringWithoutCRs:aString atLine:lineNr col:colNr.
    self addUndo:(DeleteRange line1:lineNr col1:colNr line2:lineNr col2:colNr+aString size-1 info:'insert').

The #basicWithoutRedraw... method expands tabs with spaces and insets expanded string. This indeed changes the size of inserted string, so end col computation when adding undo is wrong - aString size is not the size of the string actually pasted. Sigh.

comment:11 by jan vrany, 7 years ago

Regarding leftovers after undo adter paste with tabs, patch bb1f1b7dccbc (see above) contains some tests for this case (as well as all other tests written so fat) and path bb1f1b7dccbc should fix this particular problem.

comment:12 by patrik.svestka@…, 7 years ago

Great work. I'll test it later.

I have found one more issue with the editor.

If you have a source code like:

contents := String new: 4 * 1024.
contents replaceAll: Character space with: $X.

top := StandardSystemView new.
top extent:300@200.

textView := EditTextView new.
textView origin:0.0 @ 0.0 corner:1.0 @ 1.0.
top addSubView:textView.

textView contents: contents.

top open. 

and you want to indent part of it it behaves unexpectedly.

Steps to reproduce:

  1. Select a block of code
  2. Press TAB
  3. It should indent all the code selected

in reply to:  12 comment:13 by jan vrany, 7 years ago

...
and you want to indent part of it it behaves unexpectedly.

Steps to reproduce:

  1. Select a block of code
  2. Press TAB
  3. It should indent all the code selected

Well, this is not a bug, but a missing feature. There should be a macro to indent a block bound to F10. Likewise, a macro to undent a block should be bound to F9.

I believe it should be possible to bind a modification of it to Tab. But please, file in a separate bug/todo for documenting this and eventually including it to build. This bug is already big enough (although it looked innocent at first glance :-)

in reply to:  9 comment:14 by patrik.svestka@…, 7 years ago

Replying to jan vrany:

Huh, you just have opened Pandora's box. It looks to me that there are at least three independen problems:

  • event processing does not invoke same code as using API ('Ctrl-V' vs. #paste)
  • undo mechanism is broken and / or not behaving as expected. Namely undo transaction / action merging is not working as expected. See UndoSupport >> closeTransactionAndAddTo:
  • selection and cursor position invariant IS broken (not always true)

It will take a lot of effort to get it right, I'm afraid.

Ehm, just found out that my comment was not recorded. You would not like only easy bugs.

  • event processing does not invoke same code as using API ('Ctrl-V' vs. #paste)

This is weird. How could that happen? (historical reasons?)

  • undo mechanism is broken and / or not behaving as expected. Namely undo transaction / action merging is not working as expected. See UndoSupport >> closeTransactionAndAddTo:

You probably already found the reason - unsynced methods

  • selection and cursor position invariant IS broken (not always true)

This is also kind of weird to me.

Replying to jan vrany:

...
and you want to indent part of it it behaves unexpectedly.

Steps to reproduce:

  1. Select a block of code
  2. Press TAB
  3. It should indent all the code selected

Well, this is not a bug, but a missing feature. There should be a macro to indent a block bound to F10. Likewise, a macro to undent a block should be bound to F9.

I believe it should be possible to bind a modification of it to Tab. But please, file in a separate bug/todo for documenting this and eventually including it to build. This bug is already big enough (although it looked innocent at first glance :-)

I was considering it as it is borderline case. I'll create a separate todo for better tracking.

comment:15 by jan vrany, 7 years ago

Since this bug covers many different cases, let's review each case and corresponding fix one-by-one and push them once reviewed. Forget all patches attached before. So:

Case 0

Assertion failure when extending a selection word-wise after a word-select.

Steps to reproduce:

  1. Open an empty workspace and type three words as in:
    AAA BBB CCC
    
  2. Position cursor in the middle of word BBB and do select word (press Alt-w). Word BBB should be selected afterwards.
  3. Try to extend the selection word-wise to the left by pressing Ctrl-Shift-Left

Patch libwidg_fix_1_of_1_rev_ef289d40b266_Issue__124__case_0__Fixed_long_standing_bug_with_inconsistent_cursor_position_after_select_word.patch should fix it.

Please, review it, test it and let me know if it fixes the issue. If yes, I'll push and move on another case.

in reply to:  15 comment:16 by patrik.svestka@…, 7 years ago

Replying to jan vrany:

Since this bug covers many different cases, let's review each case and corresponding fix one-by-one and push them once reviewed. Forget all patches attached before. So:

Case 0

Assertion failure when extending a selection word-wise after a word-select.

Steps to reproduce:

  1. Open an empty workspace and type three words as in:
    AAA BBB CCC
    
  2. Position cursor in the middle of word BBB and do select word (press Alt-w). Word BBB should be selected afterwards.
  3. Try to extend the selection word-wise to the left by pressing Ctrl-Shift-Left

Patch libwidg_fix_1_of_1_rev_ef289d40b266_Issue__124__case_0__Fixed_long_standing_bug_with_inconsistent_cursor_position_after_select_word.patch should fix it.

Please, review it, test it and let me know if it fixes the issue. If yes, I'll push and move on another case.

  • EDITED to make more sense + correct some English mistakes .].

I have tried this patch, part works part not.

The part that works is pressing the alt+w (ad 2) ). That correctly selects a word.

The part that does not work, in my opinion, is ad 3). There, if you press ctrl+shift+left it deselects the BBB part of the text. In subsequent press it selects first the space between AAA BBB, then it selects also AAA, but the BBB is deselected.

I have also tested a combination ctrl+shift+right which partially works. It correctly selects space between BBB CCC. However, when I try to select CCC with additional ctrl+shift+right key-press, I get also 10 empty lines selected with CCC (till the EOF). The correct behavior, in my eyes, would be just to select CCC till the EOL instead of EOF.

Last edited 7 years ago by patrik.svestka@… (previous) (diff)

comment:17 by jan vrany, 7 years ago

Case 0

I have tried this patch, part works part not.

I understand your comment and agree that the behavior is not what one might expect. However, the problem the patch should fix is that you get assertion failure (therefore a debugger) not that it misbehaves. It seems that you did not see any assertion failures anymore.

So I decided

  1. to push this change and consider Case 0 fixed so we can move on another case in this issue.
  2. I also opened a new issue to fix the incorrect / unexpected behavior of Shift-Ctrl-Left/Right w.r.t. word-selection - see #135. Let's continue to work on this particular issue there.

Sorry about it, but I just don't want to throw more and more code into this issue as it's already big and text editor is clearly endless source of issues.

in reply to:  17 comment:18 by patrik.svestka@…, 7 years ago

Replying to jan vrany:

Case 0

I have tried this patch, part works part not.

I understand your comment and agree that the behavior is not what one might expect. However, the problem the patch should fix is that you get assertion failure (therefore a debugger) not that it misbehaves. It seems that you did not see any assertion failures anymore.

So I decided

  1. to push this change and consider Case 0 fixed so we can move on another case in this issue.
  2. I also opened a new issue to fix the incorrect / unexpected behavior of Shift-Ctrl-Left/Right w.r.t. word-selection - see #135. Let's continue to work on this particular issue there.

Sorry about it, but I just don't want to throw more and more code into this issue as it's already big and text editor is clearly endless source of issues.

I don't see any assertion failures. You are right to create separate #135 ticket, otherwise we would get lost in this loooong ticket.

comment:19 by jan vrany, 7 years ago

Case 1

Single edit action may generate 2 undo actions

Steps to reproduce:

  1. Open a fresh text view by evaluating following code:
    top := StandardSystemView new.
    top extent:300@200.
    textView := EditTextView new.
    textView origin:0.0 @ 0.0 corner:1.0 @ 1.0.
    top addSubView:textView.
    textView contssents: '1234'.
    textView setClipboardText:'Smalltalk'.            
    top open.
    
    Please note that it's important to create editor this way as it's essential to have some text already pre-set.
    1. Select text 34 in the editor - either by mouse or by keyboard, does not matter.
    2. Press Ctrl-V to paste text (Smalltalk in this case). This should replace selection.
    3. Press Ctrl-Z to undo

Edit text view contains only 12, but should contain the original text, i.e., 1234.

This should be fixed by:

in reply to:  19 ; comment:20 by patrik.svestka@…, 7 years ago

Replying to jan vrany:

Case 1

Single edit action may generate 2 undo actions

Steps to reproduce:

There is a typo in the Smalltalk code, I'm fixing it here:

  1. Open a fresh text view by evaluating following code:
 top := StandardSystemView new.
 top extent:300@200.
 textView := EditTextView new.
 textView origin:0.0 @ 0.0 corner:1.0 @ 1.0.
 top addSubView:textView.
 textView contents: '1234'.
 textView setClipboardText:'Smalltalk'.            
 top open.

Please note that it's important to create editor this way as it's essential to have some text already pre-set.

  1. Select text 34 in the editor - either by mouse or by keyboard, does not matter.
  2. Press Ctrl-V to paste text (Smalltalk in this case). This should replace selection.
  3. Press Ctrl-Z to undo

Edit text view contains only 12, but should contain the original text, i.e., 1234.

This should be fixed by:

This now behaves much better than before. These steps work as you suggested. 34 is correctly replaced by Smalltalk string in the clipboard and is correctly undo(ed).

I tired to test it little bit deeper. I tried to paste more Smalltalk string and undo it afterwards. The undo process is flawless - it correctly returns to the original state.

The pasting is a different case thou. It does not behave as is now considered common sense. When you paste additional Smalltalk strings the original Smalltalk string remains selected. In my eyes that should not be the case.

The rectification is matter of personal view, best would be to follow what is now considered a standard.

Maybe better solution would be selecting new pasting OR unselect the original pasting and just move the cursor behind the last pasting (that is the "normal" behavior of today's text editors).

in reply to:  20 comment:21 by jan vrany, 7 years ago

Replying to patrik.svestka@…:

This now behaves much better than before. These steps work as you suggested. 34 is correctly replaced by Smalltalk string in the clipboard and is correctly undo(ed).

I tired to test it little bit deeper. I tried to paste more Smalltalk string and undo it afterwards. The undo process is flawless - it correctly returns to the original state.

OK, I consider Case 1 being fixed by those patches.

The pasting is a different case thou. It does not behave as is now considered common sense. When you paste additional Smalltalk strings the original Smalltalk string remains selected. In my eyes that should not be the case.

Well, this is another thing - I suggest you to open a new issue for this - this thread is long enough.

comment:22 by patrik.svestka@…, 7 years ago

I have added a new ticket #170 for the pasting issue. Yes, consider Case 1 as fixed.

comment:23 by jan vrany, 6 years ago

Keywords: EditTextView added
Milestone: 8.0.08.1.0

Fixes for Case 1 have been integrated (some time ago).

As this is part of greater effort to revise EditTextView's behavior, I'm postponing this to 8.1.0,

Note: See TracTickets for help on using tickets.