Re: Only show Accept/Reject Change options if relevant

[Scott Kostyshak]

On Thu, May 10, 2018 at 05:23:32PM +, Jean-Marc Lasgouttes wrote:
> Le 10/05/2018 à 19:05, Scott Kostyshak a écrit :
> > It would make it easier to read the code. For example, I would know that
> > methods such as forwardPar() could be used, but that no changes to the
> > document would be done. But perhaps it is not clear what a "change" is.
> 
> Knowing what a change is is in general no clear in our data structures.

That answers my question. Thanks.

Scott


signature.asc
Description: PGP signature

Re: Only show Accept/Reject Change options if relevant

[Jean-Marc Lasgouttes]

Le 10/05/2018 à 19:05, Scott Kostyshak a écrit :

It would make it easier to read the code. For example, I would know that
methods such as forwardPar() could be used, but that no changes to the
document would be done. But perhaps it is not clear what a "change" is.


Knowing what a change is is in general no clear in our data structures.

JMarc

Re: Only show Accept/Reject Change options if relevant

[Scott Kostyshak]

On Thu, May 10, 2018 at 11:26:41AM +, Jean-Marc Lasgouttes wrote:
> Le 08/05/2018 à 02:04, Scott Kostyshak a écrit :
> > The patch is attached. Any comments?
> 
> It looks good.

Thanks, it's in at 23de5e5e.

> > In addition, I have a couple of questions:
> > 
> > 1. Is there a reason that there is no const_DocIterator class?
> 
> No, what would it be good for?

It would make it easier to read the code. For example, I would know that
methods such as forwardPar() could be used, but that no changes to the
document would be done. But perhaps it is not clear what a "change" is.

> 
> > 2. If paragraph::id() did not exist, would it be reasonable to do
> > something like the following?
> > 
> >if (() == ().paragraph())
> >  ...
> 
> Yes, I think so.

Thanks,

Scott


signature.asc
Description: PGP signature

Re: Only show Accept/Reject Change options if relevant

[Jean-Marc Lasgouttes]

Le 08/05/2018 à 02:04, Scott Kostyshak a écrit :

The patch is attached. Any comments?


It looks good.


In addition, I have a couple of questions:

1. Is there a reason that there is no const_DocIterator class?


No, what would it be good for?


2. If paragraph::id() did not exist, would it be reasonable to do
something like the following?

   if (() == ().paragraph())
 ...


Yes, I think so.

JMarc

Re: Only show Accept/Reject Change options if relevant

[Scott Kostyshak]

On Sun, May 06, 2018 at 09:19:11PM +, Scott Kostyshak wrote:
> On Sun, May 06, 2018 at 07:29:03PM +, Jean-Marc Lasgouttes wrote:
> > Le 05/05/2018 à 21:53, Scott Kostyshak a écrit :
> > > I'm unsure what approach to take. One option would be to define
> > > "isChanged()" to return true if there is a change in the paragraph or in
> > > any paragraph within the paragraph. I think that currently, isChanged()
> > > checks only the top-level paragraph. I have no idea if this is a
> > > reasonable change to make.
> > 
> > Here is what I would do : you can go over all paragraphs using DocIterator
> > (forwardPar should do what you want). This will go inside insets.
> 
> Thanks, I'm working on a patch.

The patch is attached. Any comments?

In addition, I have a couple of questions:

1. Is there a reason that there is no const_DocIterator class?

2. If paragraph::id() did not exist, would it be reasonable to do
   something like the following?

  if (() == ().paragraph())
...

Thanks,

Scott
From bc6d744817197bea490dc537b9b1ebcaa6984192 Mon Sep 17 00:00:00 2001
From: Scott Kostyshak 
Date: Fri, 4 May 2018 18:21:54 -0400
Subject: [PATCH] Only show Accept/Reject Change options if relevant

In the context menu for a selection, we now only show the options
"Accept Change" and "Reject Change" if there is actually a change in
the selection.

This fixes #10338.
---
 src/Paragraph.h |  4 +++-
 src/Text3.cpp   | 32 +++-
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/src/Paragraph.h b/src/Paragraph.h
index 450dfe3..b818322 100644
--- a/src/Paragraph.h
+++ b/src/Paragraph.h
@@ -263,10 +263,12 @@ public:
 	/// look up change at given pos
 	Change const & lookupChange(pos_type pos) const;
 
-	/// is there a change within the given range ?
+	/// is there a change within the given range (does not
+	/// check contained paragraphs)
 	bool isChanged(pos_type start, pos_type end) const;
 	/// is there an unchanged char at the given pos ?
 	bool isChanged(pos_type pos) const;
+
 	/// is there an insertion at the given pos ?
 	bool isInserted(pos_type pos) const;
 	/// is there a deletion at the given pos ?
diff --git a/src/Text3.cpp b/src/Text3.cpp
index 9b0f50c..bd8f91c 100644
--- a/src/Text3.cpp
+++ b/src/Text3.cpp
@@ -3185,17 +3185,31 @@ bool Text::getStatus(Cursor & cur, FuncRequest const & cmd,
 
 	case LFUN_CHANGE_ACCEPT:
 	case LFUN_CHANGE_REJECT:
-		// In principle, these LFUNs should only be enabled if there
-		// is a change at the current position/in the current selection.
-		// However, without proper optimizations, this will inevitably
-		// result in unacceptable performance - just imagine a user who
-		// wants to select the complete content of a long document.
 		if (!cur.selection())
 			enable = cur.paragraph().isChanged(cur.pos());
-		else
-			// TODO: context-sensitive enabling of LFUN_CHANGE_ACCEPT/REJECT
-			// for selections.
-			enable = true;
+		else {
+			// will enable if there is a change in the selection
+			enable = false;
+
+			// cheap improvement for efficiency: using cached
+			// buffer variable, if there is no change in the
+			// document, no need to check further.
+			if (!cur.buffer()->areChangesPresent())
+break;
+
+			for (DocIterator it = cur.selectionBegin(); it < cur.selectionEnd(); it.forwardPar()) {
+pos_type end;
+if (it.paragraph().id() == cur.selectionEnd().paragraph().id())
+	end = cur.selectionEnd().pos();
+else
+	end = it.paragraph().size();
+pos_type const beg = it.pos();
+if (beg != end && it.paragraph().isChanged(beg, end)) {
+	enable = true;
+	break;
+}
+			}
+		}
 		break;
 
 	case LFUN_OUTLINE_UP:
-- 
2.7.4



signature.asc
Description: PGP signature

Re: Only show Accept/Reject Change options if relevant

[Scott Kostyshak]

On Sun, May 06, 2018 at 07:29:03PM +, Jean-Marc Lasgouttes wrote:
> Le 05/05/2018 à 21:53, Scott Kostyshak a écrit :
> > I'm unsure what approach to take. One option would be to define
> > "isChanged()" to return true if there is a change in the paragraph or in
> > any paragraph within the paragraph. I think that currently, isChanged()
> > checks only the top-level paragraph. I have no idea if this is a
> > reasonable change to make.
> 
> Here is what I would do : you can go over all paragraphs using DocIterator
> (forwardPar should do what you want). This will go inside insets.

Thanks, I'm working on a patch.

Scott


signature.asc
Description: PGP signature

Re: Only show Accept/Reject Change options if relevant

[Jean-Marc Lasgouttes]

Le 05/05/2018 à 21:53, Scott Kostyshak a écrit :

I'm unsure what approach to take. One option would be to define
"isChanged()" to return true if there is a change in the paragraph or in
any paragraph within the paragraph. I think that currently, isChanged()
checks only the top-level paragraph. I have no idea if this is a
reasonable change to make.


Here is what I would do : you can go over all paragraphs using 
DocIterator (forwardPar should do what you want). This will go inside 
insets.


JMarc

Re: Only show Accept/Reject Change options if relevant

[Richard Kimberly Heck]

On 05/06/2018 09:34 AM, Scott Kostyshak wrote:
> On Sun, May 06, 2018 at 02:05:45AM +, Richard Kimberly Heck wrote:
>
>> You could just define a new method that does this and leave the old one
>> as is.
> That is a good idea. Actually, it appears that this function definition
> is not used. But I think I will still not touch it and create a new
> function called "hasChange()".

Yes, only the single argument version seems to be used. I'd go ahead and
delete,
or at least comment out, the unused one.

Riki

Re: Only show Accept/Reject Change options if relevant

[Scott Kostyshak]

On Sun, May 06, 2018 at 02:05:45AM +, Richard Kimberly Heck wrote:

> You could just define a new method that does this and leave the old one
> as is.

That is a good idea. Actually, it appears that this function definition
is not used. But I think I will still not touch it and create a new
function called "hasChange()".

> > If that is not a correct approach, then I suppose that I need to check
> > each paragraph on all levels myself. In this case, is there a way to
> > iterate through all paragraphs and nested paragraphs of a selection? If
> > there is not currently a way, would this be useful as a new helper
> > method?
> 
> There must be a good way to do this, but I do not know what it is.

I think I can copy the approach in Paragraph::acceptChanges.

Thanks,

Scott


signature.asc
Description: PGP signature

Re: Only show Accept/Reject Change options if relevant

[Richard Kimberly Heck]

On 05/05/2018 03:53 PM, Scott Kostyshak wrote:
> On Sat, May 05, 2018 at 05:11:31PM +, Scott Kostyshak wrote:
>> On Sat, May 05, 2018 at 04:05:49PM +, Richard Kimberly Heck wrote:
>>> On 05/05/2018 11:56 AM, Scott Kostyshak wrote:
 The attached patch is an attempt to only show the "Accept Change" and
 "Reject Change" options in the context menu when there is a change in
 the selection.

 It works well except if the selection is inside an inset. For example,
 when in a LyX Note, ppos_beg and ppos_end refer to the position within
 the note. Does each inset have its own ParagraphList? If so, how do I
 access it?
>>> If it's an InsetText, then it has its own Text object, which has paragraphs:
>>>     ParagraphList const & pars = theInset.text().paragraphs();
>> Thanks, Riki. that's exactly what I needed. New patch attached.
>>
>> The next problem is that I need to check all paragraphs on all depths.
>> For example, if there is a change inside a LyX Note, and I select the
>> LyX Note from the outside of the inset, isChanged() is false because I
>> guess it checks whether the entire note was changed, not the contents of
>> the note.
>>
>> I'll look into this.
> I'm unsure what approach to take. One option would be to define
> "isChanged()" to return true if there is a change in the paragraph or in
> any paragraph within the paragraph. I think that currently, isChanged()
> checks only the top-level paragraph. I have no idea if this is a
> reasonable change to make.

You could just define a new method that does this and leave the old one
as is.

> If that is not a correct approach, then I suppose that I need to check
> each paragraph on all levels myself. In this case, is there a way to
> iterate through all paragraphs and nested paragraphs of a selection? If
> there is not currently a way, would this be useful as a new helper
> method?

There must be a good way to do this, but I do not know what it is.

Riki

Re: Only show Accept/Reject Change options if relevant

[Scott Kostyshak]

On Sat, May 05, 2018 at 05:11:31PM +, Scott Kostyshak wrote:
> On Sat, May 05, 2018 at 04:05:49PM +, Richard Kimberly Heck wrote:
> > On 05/05/2018 11:56 AM, Scott Kostyshak wrote:
> > > The attached patch is an attempt to only show the "Accept Change" and
> > > "Reject Change" options in the context menu when there is a change in
> > > the selection.
> > >
> > > It works well except if the selection is inside an inset. For example,
> > > when in a LyX Note, ppos_beg and ppos_end refer to the position within
> > > the note. Does each inset have its own ParagraphList? If so, how do I
> > > access it?
> > 
> > If it's an InsetText, then it has its own Text object, which has paragraphs:
> >     ParagraphList const & pars = theInset.text().paragraphs();
> 
> Thanks, Riki. that's exactly what I needed. New patch attached.
> 
> The next problem is that I need to check all paragraphs on all depths.
> For example, if there is a change inside a LyX Note, and I select the
> LyX Note from the outside of the inset, isChanged() is false because I
> guess it checks whether the entire note was changed, not the contents of
> the note.
> 
> I'll look into this.

I'm unsure what approach to take. One option would be to define
"isChanged()" to return true if there is a change in the paragraph or in
any paragraph within the paragraph. I think that currently, isChanged()
checks only the top-level paragraph. I have no idea if this is a
reasonable change to make.

If that is not a correct approach, then I suppose that I need to check
each paragraph on all levels myself. In this case, is there a way to
iterate through all paragraphs and nested paragraphs of a selection? If
there is not currently a way, would this be useful as a new helper
method?

Scott


signature.asc
Description: PGP signature

Re: Only show Accept/Reject Change options if relevant

[Scott Kostyshak]

On Sat, May 05, 2018 at 04:05:49PM +, Richard Kimberly Heck wrote:
> On 05/05/2018 11:56 AM, Scott Kostyshak wrote:
> > The attached patch is an attempt to only show the "Accept Change" and
> > "Reject Change" options in the context menu when there is a change in
> > the selection.
> >
> > It works well except if the selection is inside an inset. For example,
> > when in a LyX Note, ppos_beg and ppos_end refer to the position within
> > the note. Does each inset have its own ParagraphList? If so, how do I
> > access it?
> 
> If it's an InsetText, then it has its own Text object, which has paragraphs:
>     ParagraphList const & pars = theInset.text().paragraphs();

Thanks, Riki. that's exactly what I needed. New patch attached.

The next problem is that I need to check all paragraphs on all depths.
For example, if there is a change inside a LyX Note, and I select the
LyX Note from the outside of the inset, isChanged() is false because I
guess it checks whether the entire note was changed, not the contents of
the note.

I'll look into this.

Scott
From 335f461c26d7aa64c01f7e1f9722d4007e983ce2 Mon Sep 17 00:00:00 2001
From: Scott Kostyshak 
Date: Fri, 4 May 2018 18:21:54 -0400
Subject: [PATCH] Only show Accept/Reject Change options if relevant

In the context menu for a selection, we now only show the options
"Accept Change" and "Reject Change" if there are actually changes in
the selection.
---
 src/Text3.cpp | 33 -
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/src/Text3.cpp b/src/Text3.cpp
index 9b0f50c..f0d488b 100644
--- a/src/Text3.cpp
+++ b/src/Text3.cpp
@@ -3185,17 +3185,32 @@ bool Text::getStatus(Cursor & cur, FuncRequest const & cmd,
 
 	case LFUN_CHANGE_ACCEPT:
 	case LFUN_CHANGE_REJECT:
-		// In principle, these LFUNs should only be enabled if there
-		// is a change at the current position/in the current selection.
-		// However, without proper optimizations, this will inevitably
-		// result in unacceptable performance - just imagine a user who
-		// wants to select the complete content of a long document.
 		if (!cur.selection())
 			enable = cur.paragraph().isChanged(cur.pos());
-		else
-			// TODO: context-sensitive enabling of LFUN_CHANGE_ACCEPT/REJECT
-			// for selections.
-			enable = true;
+		else {
+			// enable if there is a change in the selection
+			enable = false;
+			ParagraphList const & pars = cur.text()->paragraphs();
+			pit_type const pars_begin = cur.selectionBegin().pit();
+			pit_type const pars_end = cur.selectionEnd().pit();
+			for (pit_type pit = pars_begin; pit <= pars_end; ++pit) {
+pos_type ppos_beg, ppos_end;
+if (pit == pars_begin)
+	ppos_beg = cur.selBegin().pos();
+else
+	ppos_beg = 0;
+if (pit == pars_end)
+	ppos_end = cur.selEnd().pos();
+else
+	ppos_end = pars[pit].size();
+
+if (ppos_beg != ppos_end &&
+pars[pit].isChanged(ppos_beg, ppos_end)) {
+	enable = true;
+	break;
+}
+			}
+		}
 		break;
 
 	case LFUN_OUTLINE_UP:
-- 
2.7.4



signature.asc
Description: PGP signature

Re: Only show Accept/Reject Change options if relevant

[Richard Kimberly Heck]

On 05/05/2018 11:56 AM, Scott Kostyshak wrote:
> The attached patch is an attempt to only show the "Accept Change" and
> "Reject Change" options in the context menu when there is a change in
> the selection.
>
> It works well except if the selection is inside an inset. For example,
> when in a LyX Note, ppos_beg and ppos_end refer to the position within
> the note. Does each inset have its own ParagraphList? If so, how do I
> access it?

If it's an InsetText, then it has its own Text object, which has paragraphs:
    ParagraphList const & pars = theInset.text().paragraphs();

Riki