#1007 Selection enhancements (patch included)

Completed
closed
Neil Hodgson
None
5
2013-08-31
2013-08-09
Neomi
No

Hi,
I have made a few enhancements that I would love to see included. Basically I upgraded the rectangular selection mode so it can be used the same way as in Visual Studio which I am used to. It is now possible to switch to rectangular mode by pressing the Alt key when a selection is in progress (with the mouse, it already worked with the keyboard). Another enhancement is that rectangular mode is sticky (selection stays in rectangular mode even if Alt is being released). Since this one is up to personal preference and I don't want to break anything, I made it an option and left it off by default.

To allow the mode switch for mouse selections, I extended Editor::ButtonMove() to have the same parameters as ButtonDown(). I extended ButtonUp() as well for consistency, although I didn't use the other modifier keys in there. I implemented the necessary changes for all platforms, but could test only Win32 myself, so a quick functionality check on the other platforms is still needed.

There are now two new functions in Scintilla.iface to set and get the new option. I based them on the multiple selection option and gave them new ids following the previous largest 2xxx value, so it should be correct. But I didn't generate Scintilla.h (don't know how right now), instead I added them there manually. So a quick check if everything is correct is still needed.

While making and testing these changes, I came across a previously existing bug with switching to rectangular selection mode. To reproduce, copy this text into an editor and follow the steps...

xxAxxxxxxx
xxxxxxxxxx
xxxxxxxBxx

  • place the cursor before the A
  • press Shift and go to before B with stream selection
  • then press Alt (keep Shift pressed) and go up to switch to rectangular selection

The line containing the B was not updated, part of the old selection remains. It is just a drawing issue, the line didn't get invalidated for drawing because the +1 in Editor::InvalidateSelection() didn't reach the lower line. When moving the cursor into or below that line or resizing the window, it gets updated. Maybe that doesn't happen on all platforms, but for me (Windows 7 x64) it does. The fix for that problem is included in the patch, that is what the additional InvalidateSelection() in Editor::MovePositionTo() is for.

2 Attachments

Discussion

  • Neil Hodgson
    Neil Hodgson
    2013-08-09

    There are several other platform layers implemented independently for Scintilla (Tk, Fltk, wxWidgets, Other Qt...) as well as the included Cocoa platform and this patch breaks the interface contract between the platform dependent and platform independent code. While the patch includes implementations for 3 platform layers, it would avoid breakage if it was written in such a way that existing code would continue to work and platforms could move to the new behaviour as they have time. A previous change to key handling for better Mac functionality shows a way that preserves compatibility better using an additional method which also allows expanding the set of modifiers:
    https://sourceforge.net/p/scintilla/code/ci/63c8b927d4840ec823c0e5785d2332c643b62412/

     
  • Neil Hodgson
    Neil Hodgson
    2013-08-09

    AFAICT, the curTimeLast field is not contributing to behaviour and is only included to allow a uniform interface for auto-scrolling using ButtonMove where it isn't read.

     
  • Neil Hodgson
    Neil Hodgson
    2013-08-10

    Current behaviour with mouse is that selection mode stays rectangular after Alt is released. Perhaps you were looking at this after implementing Alt-turns-rectangular-after-start.

     
    • Neomi
      Neomi
      2013-08-10

      Before the patch Alt was sticky for mouse selection just because the mode couldn't change, with the patch and the option turned on it can change to rectangular but not back. That is not the same behavior as without the patch, although similar in a way. Sticky mode also works with keyboard input.

      As for the other platform layers: sorry for that, I was only aware of those three platforms I found in C++ form (I looked into the Scintilla code for the first time a few days ago when I was trying to exterminate some bugs in Notepad++). I will have a closer look at it tomorrow and resubmit the patch with platform compatibility restored.

       
      • Neil Hodgson
        Neil Hodgson
        2013-08-10

        For mouse selection, the patch makes it possible to be non-sticky and defaults to non-sticky. No one has asked for non-sticky rectangular mouse selection so it seems a mistake to change the default behaviour in this way.

        New features should be explained in the doc/ScintillaDoc.html file.

         
  • Neomi
    Neomi
    2013-08-10

    Thanks for the guidelines Neil, I made new patches and addressed all the problems you mentioned. Changes to the previous patches are:

    • Editor::ButtonMove() and Editor::ButtonUp() are as they were before, therefore the interface contract is still intact and no platform breaks. Now they just call the new methods Editor::ButtonMoveWithModifiers() and Editor::ButtonUpWithModifiers() with the modifier bit mask constructed from the last memorized modifiers (except for ctrl from ButtonUp). The memorized curTimeLast is gone again, it really didn't make much sense to deliver an outdated time stamp in Editor::Tick().

    • I changed the bool option for rectangular sticky selections into a selection behavior (named with "ou" to be consistent with existing interface constants) that can be configured individually for keyboard and mouse selections. Defaults are set so that the behavior is exactly like it is without the patch, non-sticky for keyboard and sticky in both directions for mouse.

    • Found another previously existing bug where the window wasn't redrawn when combining multiple selections with a lower multi line stream selection shrinking upwards. The fix is already in the patch.

    • Documentation is updated too. Both for Scintilla and for the new options in SciTE.

    The included Cocoa platform should be updated by someone who knows Cocoa, I don't and I didn't want to introduce a new bug, therefore I didn't touch it. But with the interface repaired again it will still work without an update, it just can't use the mouse selection improvements.

    Is there anything I still missed?

     
  • Neil Hodgson
    Neil Hodgson
    2013-08-12

    My earlier comments were an attempt to simplify the changes, not add yet more states. There are now 2 sets of 4 states (15 additional states) implemented by the patch. This will increase the difficulty of testing and maintaining this code for what appears to me to be little benefit.

    Unless there are benefits to some of these modes that I'm not seeing, it seems to me that the only worthwhile change is to include a single choice for mouse handling to allow Alt to switch to rectangular selection during move and no change for keyboard handling.

    With modifiers becoming a bit set, having 3 separate last flags is clutter. The name should include 'mouse' since modifiers are treated differently for keyboard and mouse input.

    The two selection redraw fixes have been committed as [80d79f].

     

    Related

    Commit: [80d79f]

    • Neomi
      Neomi
      2013-08-12

      The keyboard selection behavior was mainly to be able to emulate Microsoft Visual Studio behavior (it sticks to rectangular in keyboard selections too) and to be complete and consistent with mouse options, but I can do without it. I will remove it again in the next update. The important one for me was the mouse behavior.

      As for the mouse options, should I make it a boolean option (how would you name it?) or can I keep SCI_(S/G)ETSELECTIONBEHAVIOURMOUSE with only SCSB_STICKY and SCSB_RECTANGULARSTICKY? Or can I include SCSB_NONSTICKY as well? With a non-sticky behavior as an option, it would allow having the same behavior as keyboard selections have, but I won't insist if you think it shouldn't be there. SCSB_STREAMSTICKY was in there for completeness only, I wouldn't have used that option myself.

      The flags shiftLast, ctrlLast and altLast will be combined into mouseModifiersLast in the v3 patches. I will update them later today.

       
  • Neomi
    Neomi
    2013-08-12

    As promised, the new patches are done. Less clutter with mouseModifiersLast, unmodified keyboard selection (keyboard selection behavior instructions are gone as well) and the stream sticky behavior is gone too. These are the v3a patches.

    I think SCSB_NONSTICKY is a useful option to have, therefore I left it in v3a. If you really think it should be left out, that's what the v3b patches are for. They are reduced to only default sticky and rectangular sticky mouse selection behavior. Everything including documentation is updated accordingly in both versions of the patches.

    Thank you for your patience, I think the patches are finally getting there.

     
  • Neil Hodgson
    Neil Hodgson
    2013-08-14

    While the YAGNI principle can't be applied in all cases, this is one situation where I think its reasonable. There have been no other requests for this or related changes.
    http://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

    Attached is an attempt to minimize the patch down to the most basic implementation. A boolean setting to choose whether to allow switching to rectangular may be justified.

     
    Attachments
    • Neomi
      Neomi
      2013-08-14

      On one hand, this changes the default behavior when done without a previously activated option. On the other hand, pressing Alt while selecting with the mouse has done nothing so far, so changing this would break nothing. Unless of course somebody presses Alt for any reason and expects that nothing happens.

      Here is one more version of the patches, stripped down to your minimized approach plus the option (and documentation) in case you prefer to have it. I am happy with both versions.

       
  • Neil Hodgson
    Neil Hodgson
    2013-08-31

    • status: open --> closed
    • assigned_to: Neil Hodgson