Re: [LyX/master] Fixup the fixup d0acc3e57044: use editable()/isActive()

[Jean-Marc Lasgouttes]

Le 20/07/2017 à 15:13, Jürgen Spitzmüller a écrit :

Am Donnerstag, den 20.07.2017, 14:57 +0200 schrieb Jean-Marc
Lasgouttes:

Technically, insets with a dialog are also "editable".


No, now we say that they "have settings". The notion of editable is
really related to entering in the inset with a cursor.


I see. Didn't know. This strikes me like a rather idiosyncratic use of
"editing". I would prefer if the function name would be more clear
about what that means.


I seem to remember that a long time ago we had editable and "highly 
editable". The problem is that an inset can be both (think box inset), 
so it is better to have two names. HasSettings is a good choice, since 
the UI does not say "edit" but "settings".


JMarc

Re: [LyX/master] Fixup the fixup d0acc3e57044: use editable()/isActive()

[Jürgen Spitzmüller]

Am Donnerstag, den 20.07.2017, 14:57 +0200 schrieb Jean-Marc
Lasgouttes:
> > Technically, insets with a dialog are also "editable".
> 
> No, now we say that they "have settings". The notion of editable is 
> really related to entering in the inset with a cursor. 

I see. Didn't know. This strikes me like a rather idiosyncratic use of
"editing". I would prefer if the function name would be more clear
about what that means.

> > isEditableInWorkarea()
> > 
> > (as opposed to: via dialog)
> 
> But editable, isActive and descendable are already about that.

Yes, I wasn't questioning the meaning, but the expression (name).

Jürgen

> 
> JMarc

signature.asc
Description: This is a digitally signed message part

Re: [LyX/master] Fixup the fixup d0acc3e57044: use editable()/isActive()

[Jean-Marc Lasgouttes]

Le 20/07/2017 à 14:08, Jürgen Spitzmüller a écrit :

Am Donnerstag, den 20.07.2017, 12:32 +0200 schrieb Jean-Marc
Lasgouttes:

Le 20/07/2017 à 11:52, Jürgen Spitzmüller a écrit :

The reason probably is that the naming of these functions is not
really
good.


Indeed.


As I understand your comments, shouldn't it be something like:

cursorCanEnter() (editable())


Well technically cursor can enter a closed InsetCollapsable (with
find)


Technically, insets with a dialog are also "editable".


No, now we say that they "have settings". The notion of editable is 
really related to entering in the inset with a cursor. The problem is 
that there are several ways to enter.



isEditableInWorkarea()

(as opposed to: via dialog)


But editable, isActive and descendable are already about that.

JMarc

Re: [LyX/master] Fixup the fixup d0acc3e57044: use editable()/isActive()

[Jürgen Spitzmüller]

Am Donnerstag, den 20.07.2017, 12:32 +0200 schrieb Jean-Marc
Lasgouttes:
> Le 20/07/2017 à 11:52, Jürgen Spitzmüller a écrit :
> > The reason probably is that the naming of these functions is not
> > really
> > good.
> 
> Indeed.
> 
> > As I understand your comments, shouldn't it be something like:
> > 
> > cursorCanEnter() (editable())
> 
> Well technically cursor can enter a closed InsetCollapsable (with
> find)

Technically, insets with a dialog are also "editable".

> 
> It is a pity that cursorable does not exist.
> 
> > isWorkareaEditable() (isActive())
> 
> The name WorkArea is already used.

isEditableInWorkarea()

(as opposed to: via dialog)

is actually what I meant.

Jürgen

> 
> And of course I did not mention descendable(BufferView*) yet :)
> 
> JMarc

signature.asc
Description: This is a digitally signed message part

Re: [LyX/master] Fixup the fixup d0acc3e57044: use editable()/isActive()

[Jean-Marc Lasgouttes]

Le 20/07/2017 à 11:52, Jürgen Spitzmüller a écrit :

Am Donnerstag, den 20.07.2017, 11:38 +0200 schrieb Jean-Marc
Lasgouttes:

Sigh. It turns out I understood the whole isActive/editable/nargs
issue
only vaguely.


The reason probably is that the naming of these functions is not really
good.


Here is a patch that simplifies the code. The only difference is that 
editable() is now true for inner math insets. I am not sure whether it 
can affect the Compare feature. The other uses should be OK.


I do not think it should go to master, but rather in 2.4 as an early 
cleanup action.


JMarc
From 6c549578b78d768b81f50d354d30c223ff32efb0 Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes 
Date: Thu, 20 Jul 2017 12:11:46 +0200
Subject: [PATCH] Cleanup editable and isActive

The only intended change of behavior in this patch is that on-toplevel
math inset will now return editable()==true when isActive() is true.

The situation is now the following:

Semantics:
* isActive: true if a cursor can be placed in this inset
* editable: true if the cursor movement operations will enter this inset.

Definition:
* default: isActive() == (nargs() > 0)
* override: isActive() == false for InsetMathRef and InsetInfo
* defaut: editable() == isActive()
* override: editable() == false for closed InsetCollasapable()
---
 src/BufferView.cpp  | 1 +
 src/Compare.cpp | 2 ++
 src/DocIterator.cpp | 5 +
 src/insets/Inset.cpp| 6 --
 src/insets/Inset.h  | 6 +++---
 src/insets/InsetInfo.h  | 2 --
 src/insets/InsetTabular.h   | 2 --
 src/insets/InsetText.h  | 2 --
 src/mathed/InsetMathHull.h  | 2 --
 src/mathed/InsetMathMacroTemplate.h | 2 --
 src/mathed/InsetMathNest.cpp| 6 --
 src/mathed/InsetMathNest.h  | 2 --
 12 files changed, 7 insertions(+), 31 deletions(-)

diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 2f25636..782f86b 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -792,6 +792,7 @@ bool BufferView::moveToPosition(pit_type bottom_pit, pos_type bottom_pos,
 			// shortened. Otherwise, setCursor may crash
 			// lyx when the cursor can not be set to these
 			// insets.
+			// FIXME: why can't we use the auto_open_ mechanism in this case?
 			size_t const n = dit.depth();
 			for (size_t i = 0; i < n; ++i)
 if (!dit[i].inset().editable()) {
diff --git a/src/Compare.cpp b/src/Compare.cpp
index 796eb19..684ba21 100644
--- a/src/Compare.cpp
+++ b/src/Compare.cpp
@@ -470,6 +470,7 @@ static bool equal(Inset const * i_o, Inset const * i_n)
 	// FIXME: This fails if the parameters of the insets differ.
 	// FIXME: We do not recurse into InsetTabulars.
 	// FIXME: We need methods inset->equivalent(inset).
+	// FIXME: shouldn't this use isActive() instead?
 	if (i_o->editable() && !i_o->asInsetMath()
 		  && i_o->asInsetText())
 		return true;
@@ -842,6 +843,7 @@ void Compare::Impl::processSnake(DocRangePair const & rp)
 	DocPair it = rp.from();
 	for (; it.o < rp.o.to; ++it) {
 		Inset * inset = it.o.text()->getPar(it.o.pit()).getInset(it.o.pos());
+		// FIXME: consider using isActive() instead
 		if (inset && inset->editable() && inset->asInsetText()) {
 			// Find the inset in the paragraph list that will be pasted into
 			// the final document. The contents of the inset will be replaced
diff --git a/src/DocIterator.cpp b/src/DocIterator.cpp
index 38f2393..6375142 100644
--- a/src/DocIterator.cpp
+++ b/src/DocIterator.cpp
@@ -381,10 +381,7 @@ void DocIterator::forwardPos()
 void DocIterator::forwardPosIgnoreCollapsed()
 {
 	Inset * const nextinset = nextInset();
-	// FIXME: the check for asInsetMath() shouldn't be necessary
-	// but math insets do not return a sensible editable() state yet.
-	if (nextinset && !nextinset->asInsetMath()
-	&& !nextinset->editable()) {
+	if (nextinset && !nextinset->editable()) {
 		++top().pos();
 		return;
 	}
diff --git a/src/insets/Inset.cpp b/src/insets/Inset.cpp
index b458c81..5ea0c2a 100644
--- a/src/insets/Inset.cpp
+++ b/src/insets/Inset.cpp
@@ -484,12 +484,6 @@ bool Inset::directWrite() const
 }
 
 
-bool Inset::editable() const
-{
-	return false;
-}
-
-
 bool Inset::hasSettings() const
 {
 	return false;
diff --git a/src/insets/Inset.h b/src/insets/Inset.h
index f10ab3f..758429e 100644
--- a/src/insets/Inset.h
+++ b/src/insets/Inset.h
@@ -348,14 +348,14 @@ public:
 	virtual bool isActive() const { return nargs() > 0; }
 	/// can the contents of the inset be edited on screen ?
 	// equivalent to isActive except for closed InsetCollapsable
-	virtual bool editable() const;
-	/// has the Inset settings that can be modified in a dialog ?
-	virtual bool hasSettings() const;
+	virtual bool editable() const { return isActive(); }
 	/// can we go further down on mouse click?
 	/// true for InsetCaption, InsetCollapsables (not ButtonOnly), InsetTabular
 	virtual bool descendable(BufferView const &) const { 

Re: [LyX/master] Fixup the fixup d0acc3e57044: use editable()/isActive()

[Jean-Marc Lasgouttes]

Le 20/07/2017 à 11:52, Jürgen Spitzmüller a écrit :

The reason probably is that the naming of these functions is not really
good.


Indeed.


As I understand your comments, shouldn't it be something like:

cursorCanEnter() (editable())


Well technically cursor can enter a closed InsetCollapsable (with find)

It is a pity that cursorable does not exist.


isWorkareaEditable() (isActive())


The name WorkArea is already used.

And of course I did not mention descendable(BufferView*) yet :)

JMarc

Re: [LyX/master] Fixup the fixup d0acc3e57044: use editable()/isActive()

[Jürgen Spitzmüller]

Am Donnerstag, den 20.07.2017, 11:38 +0200 schrieb Jean-Marc
Lasgouttes:
> Sigh. It turns out I understood the whole isActive/editable/nargs
> issue 
> only vaguely.

The reason probably is that the naming of these functions is not really
good.

As I understand your comments, shouldn't it be something like:

cursorCanEnter() (editable())
isWorkareaEditable() (isActive())

Jürgen



signature.asc
Description: This is a digitally signed message part

Re: [LyX/master] Fixup the fixup d0acc3e57044: use editable()/isActive()

[Jean-Marc Lasgouttes]

Le 20/07/2017 à 03:34, Scott Kostyshak a écrit :


Open the attached .lyx file. Make sure that the cursor is at the
beginning and that the footnote inset is not visible on the screen. Then
do a find for the word "find".

Expected: that Lyx opens the inset and highlights the word "find".
Actual: LyX places the cursor before the inset and leaves the inset
closed.


Sigh. It turns out I understood the whole isActive/editable/nargs issue 
only vaguely.


I fixed this in master now at fc7fb6a56  and tried to document what the 
functions actually do. I have to backport this to stable. Richard, OK? 
If you prefer, I can just revert the whole fixifbroken modifications 
from stable, which will unfix #10667. It might be safer, after all. I am 
confident about the fix, but it is the the fourth time that I am 
confident about what is the right thing to do.


JMarc

Re: [LyX/master] Fixup the fixup d0acc3e57044: use editable()/isActive()

[Scott Kostyshak]

On Tue, Jun 27, 2017 at 04:47:10PM +0200, Jean-Marc Lasgouttes wrote:
> commit 13c3c1485b68980c51658cef8fadf804982d75ee
> Author: Jean-Marc Lasgouttes 
> Date:   Fri Jun 23 20:32:32 2017 +0200
> 
> Fixup the fixup d0acc3e57044: use editable()/isActive()
> 
> While 522516d9 was too strong and broke mathed, d0acc3e57044 is too
> lenient and can accept insets (mathed/CommandInset, InsetInfo) that
> have a positive nargs() but are not editable (because they encapsulate
> something).
> 
> Therefore the best solution for now is to use editable() in text and
> isActive() in mathed, until those two things are merged.
> 
> Part of #10667.

Git bisect suggests this broke the following:

Open the attached .lyx file. Make sure that the cursor is at the
beginning and that the footnote inset is not visible on the screen. Then
do a find for the word "find".

Expected: that Lyx opens the inset and highlights the word "find".
Actual: LyX places the cursor before the inset and leaves the inset
closed.

Note that if the footnote inset is on the screen, then for some reason
it works as expected.

Scott


p.s. I am currently traveling so have not yet looked at the other
discussions.
#LyX 2.2 created this file. For more info see http://www.lyx.org/
\lyxformat 508
\begin_document
\begin_header
\save_transient_properties true
\origin unavailable
\textclass article
\use_default_options true
\maintain_unincluded_children false
\language english
\language_package default
\inputencoding auto
\fontencoding global
\font_roman "default" "default"
\font_sans "default" "default"
\font_typewriter "default" "default"
\font_math "auto" "auto"
\font_default_family default
\use_non_tex_fonts false
\font_sc false
\font_osf false
\font_sf_scale 100 100
\font_tt_scale 100 100
\graphics default
\default_output_format default
\output_sync 1
\bibtex_command default
\index_command default
\paperfontsize default
\spacing single
\use_hyperref false
\papersize default
\use_geometry false
\use_package amsmath 1
\use_package amssymb 1
\use_package cancel 1
\use_package esint 1
\use_package mathdots 1
\use_package mathtools 1
\use_package mhchem 1
\use_package stackrel 1
\use_package stmaryrd 1
\use_package undertilde 1
\cite_engine basic
\cite_engine_type default
\biblio_style plain
\use_bibtopic false
\use_indices false
\paperorientation portrait
\suppress_date false
\justification true
\use_refstyle 1
\index Index
\shortcut idx
\color #008000
\end_index
\secnumdepth 3
\tocdepth 3
\paragraph_separation indent
\paragraph_indentation default
\quotes_language english
\papercolumns 1
\papersides 1
\paperpagestyle default
\tracking_changes false
\output_changes false
\html_math_output 0
\html_css_as_file 0
\html_be_strict false
\end_header

\begin_body

\begin_layout Standard
Hello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to reproduce the regressionHello there.
 This text before the footnote must push the footnote off the screen to
 be able to