Empty selection issues

[Scott Kostyshak]

Often when using LyX I come across empty selections. The easiest way
to see what I mean by an empty selection is to select one character to
the right, then select one character to the left (this will undo the
first). Such issues are annoying for a few reasons:

(1) some functions behave differently depending on whether there is a
selection. If you press delete, nothing happens (where I expect the
character or inset before the cusor to be deleted). If you toggle bold
or emphasize nothing happens (where if there is no selection the
entire word is toggled). There are other LyX functions that depend on
whether there is a selection or not. Further, I wonder if any part of
LyX's code assumes that if there is a selection it is non-empty.

(2) menu options are incorrectly set. For example, the scissors icon.

Here are some situations where empty selection occur(ed):

- when dragging in math mode (now fixed, see #9074)
- when spell checking (now fixed, see a117a512)

- when you click and drag slightly on a reference inset.
- when dragging inside an empty table cell.
- if you go select up and then select down inside an empty table cell.

- write some text, put your cursor in the middle of it, do shift
right, then shift left. Selection is set and delete does nothing.
- similar to previous, do the same in math mode. Also in text, do
ctrl+shift+right, then ctrl+shift+left.

- write a few words, then with the mouse click and drag upwards
(without moving horizontally). Note, however, that the selection is
removed on mouse release, but you can see that the scissors icon
became active.

I don't know how to fix these at the core, in Cursor::selHandle(bool
sel) or Cursor::setSelection(). I think the following comment in
Cursor::setSelection is relevant:

// A selection with no contents is not a selection
// FIXME: doesnt look ok
if (idx() == normalAnchor().idx() &&
pit() == normalAnchor().pit() &&
pos() == normalAnchor().pos())
setSelection(false);

Does someone have an idea to fix it there?

Attached are patches that fix the selection issues one by one. The
final patch adds an assertion that the selection is not empty. The
patches do not feel right for a few reasons. Mainly though I think it
should be fixed in Cursor::setSelection().

Any thoughts?

Scott

[Richard Heck]

Post by Scott Kostyshak
Often when using LyX I come across empty selections. The easiest way
to see what I mean by an empty selection is to select one character to
the right, then select one character to the left (this will undo the
(1) some functions behave differently depending on whether there is a
selection. If you press delete, nothing happens (where I expect the
character or inset before the cusor to be deleted). If you toggle bold
or emphasize nothing happens (where if there is no selection the
entire word is toggled). There are other LyX functions that depend on
whether there is a selection or not. Further, I wonder if any part of
LyX's code assumes that if there is a selection it is non-empty.
(2) menu options are incorrectly set. For example, the scissors icon.
- when dragging in math mode (now fixed, see #9074)
- when spell checking (now fixed, see a117a512)
- when you click and drag slightly on a reference inset.
- when dragging inside an empty table cell.
- if you go select up and then select down inside an empty table cell.
- write some text, put your cursor in the middle of it, do shift
right, then shift left. Selection is set and delete does nothing.
- similar to previous, do the same in math mode. Also in text, do
ctrl+shift+right, then ctrl+shift+left.
- write a few words, then with the mouse click and drag upwards
(without moving horizontally). Note, however, that the selection is
removed on mouse release, but you can see that the scissors icon
became active.
I don't know how to fix these at the core, in Cursor::selHandle(bool
sel) or Cursor::setSelection(). I think the following comment in
// A selection with no contents is not a selection
// FIXME: doesnt look ok
if (idx() == normalAnchor().idx() &&
pit() == normalAnchor().pit() &&
pos() == normalAnchor().pos())
setSelection(false);
Does someone have an idea to fix it there?
Attached are patches that fix the selection issues one by one. The
final patch adds an assertion that the selection is not empty. The
patches do not feel right for a few reasons. Mainly though I think it
should be fixed in Cursor::setSelection().
Any thoughts?

JMarc is the one who knows about selection, so he'll have more to say
than I do. But a
couple quick thoughts. The first is that I don't myself understand when
we should call
selHandle() and when we should call setSelection(). That makes me unsure
whether
the first part of your patch is correct.

That said, looking through InsetTabular::doDispatch, for example, I note
that we very
often call setSelection(true), which skips the test in the code you just
quoted. Simply
changing that to setSelection() is going to fix most of these problems.
Indeed, I kind of
suspect that calling setSelection(true) is always wrong, since it always
skips that test.

I'm not sure why that code is said not to look OK. The comment seems to
have been
there in some form since before 2001, as has the basic idea behind the
code, which has
since been updated. I expect the FIXME can be removed.

Post by Scott Kostyshak
0002-Fix-empty-selection-from-mouse-drag-in-empty-cell.patch
From f2abd76e2be7e9513e9de01c1cfcf114ec77e30a Mon Sep 17 00:00:00 2001
Date: Tue, 10 Jun 2014 01:19:09 -0400
Subject: [PATCH 2/5] Fix empty selection from mouse drag in empty cell
---
src/insets/InsetTabular.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/insets/InsetTabular.cpp b/src/insets/InsetTabular.cpp
index 9d87084..fbc5b9a 100644
--- a/src/insets/InsetTabular.cpp
+++ b/src/insets/InsetTabular.cpp
@@ -4031,6 +4031,8 @@ void InsetTabular::doDispatch(Cursor & cur, FuncRequest & cmd)
setCursorFromCoordinates(cur, cmd.x(), cmd.y());
bvcur.setCursor(cur);
bvcur.setSelection(true);
+ if (bvcur.selBegin() == bvcur.selEnd())
+ bvcur.selHandle(false);

Here's a good example of what I meant above: Why should we call
setSelection(true) if we may not have a selection? If we just called
setSelection(), wouldn't that do it? But again, I'm confused about when
we are meant to call selHandle().

Richard

[Jean-Marc Lasgouttes]

Post by Richard Heck
JMarc is the one who knows about selection, so he'll have more to say
than I do. But a
couple quick thoughts. The first is that I don't myself understand when
we should call
selHandle() and when we should call setSelection(). That makes me unsure
whether
the first part of your patch is correct.

selHandle is a helper function that allows to ensure that the selection
is in the correct state. It allows to make the persistent mark work, in
particular.

Post by Richard Heck
That said, looking through InsetTabular::doDispatch, for example, I note
that we very
often call setSelection(true), which skips the test in the code you just
quoted. Simply
changing that to setSelection() is going to fix most of these problems.
Indeed, I kind of
suspect that calling setSelection(true) is always wrong, since it always
skips that test.

Yes. I would propose to get rid of setSelection(bool) (replace it with
selection_ == bool in Cursor.cpp) and to use better functions elsewhere.

JMarc

[Jean-Marc Lasgouttes]

Post by Scott Kostyshak
Often when using LyX I come across empty selections. The easiest way
to see what I mean by an empty selection is to select one character to
the right, then select one character to the left (this will undo the
(1) some functions behave differently depending on whether there is a
selection. If you press delete, nothing happens (where I expect the
character or inset before the cusor to be deleted). If you toggle bold
or emphasize nothing happens (where if there is no selection the
entire word is toggled). There are other LyX functions that depend on
whether there is a selection or not. Further, I wonder if any part of
LyX's code assumes that if there is a selection it is non-empty.
(2) menu options are incorrectly set. For example, the scissors icon.

What about having Cursor::selection() return false if selection is empty?

Post by Scott Kostyshak
I don't know how to fix these at the core, in Cursor::selHandle(bool
sel) or Cursor::setSelection(). I think the following comment in
// A selection with no contents is not a selection
// FIXME: doesnt look ok
if (idx() == normalAnchor().idx() &&
pit() == normalAnchor().pit() &&
pos() == normalAnchor().pos())
setSelection(false);

I think like Richard that the comment is incorrect.

Post by Scott Kostyshak
Does someone have an idea to fix it there?

Get rid of setSelection(bool). Use selHandle whenever possible.

Set transient selection mode (M-x mark-on) and move with regular cursor
commands. It will show you places when selection is suddenly reset. This
is not good.

Post by Scott Kostyshak
Attached are patches that fix the selection issues one by one. The
final patch adds an assertion that the selection is not empty. The
patches do not feel right for a few reasons. Mainly though I think it
should be fixed in Cursor::setSelection().

The patches that add extra checks all over the code are not a good idea.
Our helper code should be good enough for what we want to do.

JMarc

[Scott Kostyshak]

On Fri, Jun 20, 2014 at 10:40 AM, Jean-Marc Lasgouttes

Post by Jean-Marc Lasgouttes
What about having Cursor::selection() return false if selection is empty?

Attached is my attempt to do this. I have no idea what this code does
though. Surprisingly the attempt seems to partially work (although I'm
sure it's still poorly written) if the selection is not started with
an inset. For example, if the cursor is just behind a note inset or a
table, selecting it will not work. If it is a math inset it crashes.

Post by Jean-Marc Lasgouttes

Post by Scott Kostyshak
I don't know how to fix these at the core, in Cursor::selHandle(bool
sel) or Cursor::setSelection(). I think the following comment in
// A selection with no contents is not a selection
// FIXME: doesnt look ok
if (idx() == normalAnchor().idx() &&
pit() == normalAnchor().pit() &&
pos() == normalAnchor().pos())
setSelection(false);

I think like Richard that the comment is incorrect.

Should we remove the comment then?

Post by Jean-Marc Lasgouttes

Post by Scott Kostyshak
Does someone have an idea to fix it there?

Get rid of setSelection(bool). Use selHandle whenever possible.
Set transient selection mode (M-x mark-on) and move with regular cursor
commands. It will show you places when selection is suddenly reset. This is
not good.

I have not looked at this yet.

Post by Jean-Marc Lasgouttes
The patches that add extra checks all over the code are not a good idea. Our
helper code should be good enough for what we want to do.

OK.

Scott

[Jean-Marc Lasgouttes]

Post by Scott Kostyshak

Post by Jean-Marc Lasgouttes
What about having Cursor::selection() return false if selection is empty?

Attached is my attempt to do this. I have no idea what this code does
though. Surprisingly the attempt seems to partially work (although I'm
sure it's still poorly written) if the selection is not started with
an inset. For example, if the cursor is just behind a note inset or a
table, selecting it will not work. If it is a math inset it crashes.

What about simplifying the test to top() == anchor_.top() ?

Post by Scott Kostyshak

Post by Jean-Marc Lasgouttes
I think like Richard that the comment is incorrect.

Should we remove the comment then?

Yes.

JMarc

[Scott Kostyshak]

On Mon, Jun 23, 2014 at 7:28 AM, Jean-Marc Lasgouttes

Post by Jean-Marc Lasgouttes

Post by Scott Kostyshak

Post by Jean-Marc Lasgouttes
What about having Cursor::selection() return false if selection is empty?

Attached is my attempt to do this. I have no idea what this code does
though. Surprisingly the attempt seems to partially work (although I'm
sure it's still poorly written) if the selection is not started with
an inset. For example, if the cursor is just behind a note inset or a
table, selecting it will not work. If it is a math inset it crashes.

What about simplifying the test to top() == anchor_.top() ?

Much cleaner. But same problems as above (selecting when an inset is
in front). Here is what I tested:

bool selection() const { return selection_ && top() != anchor_.top(); }

Post by Jean-Marc Lasgouttes

Post by Scott Kostyshak

Post by Jean-Marc Lasgouttes
I think like Richard that the comment is incorrect.

Should we remove the comment then?

Yes.

Done at c62a53b7.

Scott

[Jean-Marc Lasgouttes]

Post by Scott Kostyshak
On Fri, Jun 20, 2014 at 10:40 AM, Jean-Marc Lasgouttes

Post by Jean-Marc Lasgouttes
What about having Cursor::selection() return false if selection is empty?

Attached is my attempt to do this. I have no idea what this code does
though. Surprisingly the attempt seems to partially work (although I'm
sure it's still poorly written) if the selection is not started with
an inset. For example, if the cursor is just behind a note inset or a
table, selecting it will not work. If it is a math inset it crashes.

This is to be expected, since the cursor enters the math inset instead
of entering it. We'd have to study a bit the code to understand what is
the cleanest solution.

A possibility would be to call at the end of in BufferView::dispatch a
function (or some code) that resets the selection if it is empty.

Would that work?

JMarc

[Scott Kostyshak]

On Tue, Jun 24, 2014 at 10:54 AM, Jean-Marc Lasgouttes

Post by Jean-Marc Lasgouttes
A possibility would be to call at the end of in BufferView::dispatch a
function (or some code) that resets the selection if it is empty.
Would that work?

It works for most of the issues. Attached is a patch.
As for the empty selection issues it doesn't fix, perhaps it's because of the
FIXME: let GuiView take care of those.
in GuiWorkArea

Scott

[Jean-Marc Lasgouttes]

Post by Scott Kostyshak
It works for most of the issues. Attached is a patch.
As for the empty selection issues it doesn't fix, perhaps it's because of the
FIXME: let GuiView take care of those.
in GuiWorkArea

The patch looks good. What are the remaining problems?

JMarc

[Scott Kostyshak]

Post by Jean-Marc Lasgouttes

Post by Scott Kostyshak
It works for most of the issues. Attached is a patch.
As for the empty selection issues it doesn't fix, perhaps it's because of the
FIXME: let GuiView take care of those.
in GuiWorkArea

The patch looks good. What are the remaining problems?

The video below shows three empty selections (with the previous patch applied):
https://www.dropbox.com/s/v25ht58nu6lswmr/emptySelections.ogv

The first one is when clicking and slightly dragging on a label inset.
The second is when clicking and slightly dragging up in a table cell
(it can be empty or have text). The third is similar to the first and
is when slightly dragging on a reference inset.

We could do the same thing as the previous patch but also in
GuiWorkarea.cpp after dispatch [if I remember, we would have to do
something like if (cmd.action() != LFUN_MOUSE_MOTION)], or we can not
worry about these cases and just put the first patch in as it solves
most of the empty selection issues.

Scott

[Jean-Marc Lasgouttes]

Post by Scott Kostyshak
The first one is when clicking and slightly dragging on a label inset.
The second is when clicking and slightly dragging up in a table cell
(it can be empty or have text). The third is similar to the first and
is when slightly dragging on a reference inset.
We could do the same thing as the previous patch but also in
GuiWorkarea.cpp after dispatch [if I remember, we would have to do
something like if (cmd.action() != LFUN_MOUSE_MOTION)], or we can not
worry about these cases and just put the first patch in as it solves
most of the empty selection issues.

Did this patch eventually go in?

JMarc

[Scott Kostyshak]

On Mon, Jul 28, 2014 at 4:19 AM, Jean-Marc Lasgouttes

Post by Jean-Marc Lasgouttes

Post by Scott Kostyshak
The first one is when clicking and slightly dragging on a label inset.
The second is when clicking and slightly dragging up in a table cell
(it can be empty or have text). The third is similar to the first and
is when slightly dragging on a reference inset.
We could do the same thing as the previous patch but also in
GuiWorkarea.cpp after dispatch [if I remember, we would have to do
something like if (cmd.action() != LFUN_MOUSE_MOTION)], or we can not
worry about these cases and just put the first patch in as it solves
most of the empty selection issues.

Did this patch eventually go in?

No it did not. I wasn't sure if it should go in because it does not
fix the problem completely. It does fix "most" of the cases, I
believe. Should the patch go in and I can open a bug report for the
remaining problems? Or, should I do what I describe above, which is
patch GuiWorkarea.cpp in a similar way after dispatch, conditioning on
cmd.action() != LFUN_MOUSE_MOTION ?

Scott

[Jean-Marc Lasgouttes]

Post by Scott Kostyshak
No it did not. I wasn't sure if it should go in because it does not
fix the problem completely. It does fix "most" of the cases, I
believe. Should the patch go in and I can open a bug report for the
remaining problems? Or, should I do what I describe above, which is
patch GuiWorkarea.cpp in a similar way after dispatch, conditioning on
cmd.action() != LFUN_MOUSE_MOTION ?

I think that it is the clean solution to one half of the problem. You
can commit it and solve the other half later. Unless you can move this
code in an even higher-level place?

JMarc

[Scott Kostyshak]

On Mon, Jul 28, 2014 at 10:19 AM, Jean-Marc Lasgouttes

Post by Scott Kostyshak
No it did not. I wasn't sure if it should go in because it does not
fix the problem completely. It does fix "most" of the cases, I
believe. Should the patch go in and I can open a bug report for the
remaining problems? Or, should I do what I describe above, which is
patch GuiWorkarea.cpp in a similar way after dispatch, conditioning on
cmd.action() != LFUN_MOUSE_MOTION ?

I think that it is the clean solution to one half of the problem. You can
commit it and solve the other half later.

Sounds good. I committed at fb05011a.

Unless you can move this code in an even higher-level place?

I referenced this idea at #9222.

Scott