Re: Only show Accept/Reject Change options if relevant

[Scott Kostyshak]

On Sun, May 06, 2018 at 09:19:11PM +0000, Scott Kostyshak wrote:
> On Sun, May 06, 2018 at 07:29:03PM +0000, 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 (&it.paragraph() == &cur.selectionEnd().paragraph())
    ...

Thanks,

Scott

From bc6d744817197bea490dc537b9b1ebcaa6984192 Mon Sep 17 00:00:00 2001
From: Scott Kostyshak <skost...@lyx.org>
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