Menu

#2140 Text deleted unexpectedly using multiple selections

Bug
closed-fixed
5
2019-12-07
2019-11-20
No

To implement a 'swap selections' feature, a reasonable approach may look like:

text0 = SelectionNText(0)
text1 = SelectionNText(1)
SetSelectionNText(0, text1)
SetSelectionNText(1, text0)

This may fail when the selections are adjacent, with some of the selected text missing after the operation. For example, if the text is "ab" and there are two selections, one for "a" and one for "b", the resulting text may be just "a" instead of the expected "ba". This occurs when the selections are no longer distinct and instead overlap. A test case TestMultipleSelection.py is attached.

This is caused by insertions at the start of a selection being included in that selection leading to overlapping selections. The extra characters in the selection can be demonstrated with a simpler example where textOfSelection(0) is "1a" instead of "a" - the extension has grown to include the inserted "1".

    def textOfSelection(self, n):
        self.ed.TargetStart = self.ed.GetSelectionNStart(n)
        self.ed.TargetEnd = self.ed.GetSelectionNEnd(n)
        return bytes(self.ed.GetTargetText())

    def testInsertBefore(self):
        self.ed.ClearAll()
        t = b"a"
        self.ed.AddText(len(t), t)
        self.ed.SetSelection(0, 1)
        self.assertEquals(self.textOfSelection(0), b'a')

        self.ed.SetTargetRange(0, 0)
        self.ed.ReplaceTarget(1, b'1')
        self.assertEquals(self.ed.Contents(), b'1a')
        self.assertEquals(self.textOfSelection(0), b'a')

This was seen as benign in the past as insertion of text at a position is ambiguous - sometimes moving the position is better and sometimes leaving it stationary is better. However, the swapping example shows this is dangerous. The swapping example is based on [feature-requests:#1316].

Selections are automatically moved when text is inserted or deleted and this is currently done separately for the caret and anchor of each selection. To prevent selections overlapping other selections, a selection should continue to cover just the currently selected characters and not be extended to cover earlier text when an insertion is made at its start. This can be implemented by modifying SelectionPosition::MoveForInsertDelete to check whether it is operating on the start of a non-empty selection. This is implemented in the attached patch. The behaviour of empty selections may not be a problem so is left unchanged.

This change may cause existing code to change behaviour and possibly fail if depending on selections growing.

2 Attachments

Related

Bugs: #2393
Feature Requests: #1316

Discussion

  • Neil Hodgson

    Neil Hodgson - 2019-11-23
    • status: open --> open-fixed
     
  • Neil Hodgson

    Neil Hodgson - 2019-11-23

    Fix committed as [20f353].

     

    Related

    Commit: [20f353]

  • Neil Hodgson

    Neil Hodgson - 2019-11-25

    A regression in [20f353] caused positions to not move when virtual space was consumed by an insertion. Fixed with [9419bd].

    A further issue where anchor and caret differed only in amount of virtual space was fixed with [a9a0ed].

     

    Related

    Commit: [20f353]
    Commit: [9419bd]
    Commit: [a9a0ed]

  • Neil Hodgson

    Neil Hodgson - 2019-12-07
    • status: open-fixed --> closed-fixed
     

Log in to post a comment.