Re: Bug #3812: Comments on patch
Michael Gerz wrote:
Maybe I was talking nonsense. I just looked at the patch (without the
surrounding code). It looked like you check for the first character and
then match(...) tries to match the complete string.
On a second thought, you might be right. The attached patch does what you
propose and works for me.
OK to apply?
Jürgen
Index: src/lyxfind.cpp
===
--- src/lyxfind.cpp (Revision 18720)
+++ src/lyxfind.cpp (Arbeitskopie)
@@ -62,7 +62,8 @@
{}
// returns true if the specified string is at the specified position
- bool operator()(Paragraph const par, pos_type pos) const
+ // del specifies whether deleted strings in ct mode will be considered
+ bool operator()(Paragraph const par, pos_type pos, bool del = true) const
{
docstring::size_type const size = str.length();
pos_type i = 0;
@@ -74,6 +75,8 @@
break;
if (!cs uppercase(str[i]) != uppercase(par.getChar(pos + i)))
break;
+ if (!del par.isDeleted(pos + i))
+break;
}
if (size != docstring::size_type(i))
@@ -101,20 +104,24 @@
};
-bool findForward(DocIterator cur, MatchString const match)
+bool findForward(DocIterator cur, MatchString const match,
+ bool find_del = true)
{
for (; cur; cur.forwardChar())
- if (cur.inTexted() match(cur.paragraph(), cur.pos()))
+ if (cur.inTexted()
+ match(cur.paragraph(), cur.pos(), find_del))
return true;
return false;
}
-bool findBackwards(DocIterator cur, MatchString const match)
+bool findBackwards(DocIterator cur, MatchString const match,
+ bool find_del = true)
{
while (cur) {
cur.backwardChar();
- if (cur.inTexted() match(cur.paragraph(), cur.pos()))
+ if (cur.inTexted()
+ match(cur.paragraph(), cur.pos(), find_del))
return true;
}
return false;
@@ -141,7 +148,8 @@
}
-bool find(BufferView * bv, docstring const searchstr, bool cs, bool mw, bool fw)
+bool find(BufferView * bv, docstring const searchstr, bool cs, bool mw, bool fw,
+ bool find_del = true)
{
if (!searchAllowed(bv, searchstr))
return false;
@@ -150,7 +158,8 @@
MatchString const match(searchstr, cs, mw);
- bool found = fw ? findForward(cur, match) : findBackwards(cur, match);
+ bool found = fw ? findForward(cur, match, find_del) :
+ findBackwards(cur, match, find_del);
if (found)
bv-putSelectionAt(cur, searchstr.length(), !fw);
@@ -177,7 +186,7 @@
int const ssize = searchstr.size();
DocIterator cur = doc_iterator_begin(buf.inset());
- while (findForward(cur, match)) {
+ while (findForward(cur, match, false)) {
pos_type pos = cur.pos();
Font const font
= cur.paragraph().getFontSettings(buf.params(), pos);
@@ -227,7 +236,7 @@
Cursor cur = bv-cursor();
cap::replaceSelectionWithString(cur, replacestr, fw);
bv-buffer()-markDirty();
- find(bv, searchstr, cs, mw, fw);
+ find(bv, searchstr, cs, mw, fw, false);
bv-update();
return 1;
Index: src/BufferView.cpp
===
--- src/BufferView.cpp (Revision 18720)
+++ src/BufferView.cpp (Arbeitskopie)
@@ -665,6 +665,8 @@
{
FuncStatus flag;
+ Cursor cur = cursor_;
+
switch (cmd.action) {
case LFUN_UNDO:
@@ -678,7 +680,7 @@
case LFUN_FILE_INSERT_PLAINTEXT:
case LFUN_BOOKMARK_SAVE:
// FIXME: Actually, these LFUNS should be moved to Text
- flag.enabled(cursor_.inTexted());
+ flag.enabled(cur.inTexted());
break;
case LFUN_FONT_STATE:
case LFUN_LABEL_INSERT:
@@ -691,7 +693,6 @@
case LFUN_NOTE_NEXT:
case LFUN_REFERENCE_NEXT:
case LFUN_WORD_FIND:
- case LFUN_WORD_REPLACE:
case LFUN_MARK_OFF:
case LFUN_MARK_ON:
case LFUN_MARK_TOGGLE:
@@ -703,9 +704,13 @@
flag.enabled(true);
break;
+ case LFUN_WORD_REPLACE:
+ flag.enabled(!cur.paragraph().isDeleted(cur.pos()));
+ break;
+
case LFUN_LABEL_GOTO: {
flag.enabled(!cmd.argument().empty()
- || getInsetByCodeInsetRef(cursor_, Inset::REF_CODE));
+ || getInsetByCodeInsetRef(cur, Inset::REF_CODE));
break;
}
@@ -1617,7 +1622,9 @@
FileDialog fileDlg(_(Select LyX document to insert),
LFUN_FILE_INSERT,
make_pair(_(Documents|#o#O), from_utf8(lyxrc.document_path)),
- make_pair(_(Examples|#E#e), from_utf8(addPath(package().system_support().absFilename(), examples;
+ make_pair(_(Examples|#E#e),
+from_utf8(addPath(package().system_support().absFilename(),
+examples;
FileDialog::Result result =
fileDlg.open(from_utf8(initpath),
Re: Bug #3812: Comments on patch
Jürgen Spitzmüller wrote: On a second thought, you might be right. The attached patch does what you propose and works for me. OK to apply? This patch also fixes bug 3160: http://bugzilla.lyx.org/show_bug.cgi?id=3160 Jürgen
Re: Bug #3812: Comments on patch
Jürgen Spitzmüller wrote: Jürgen Spitzmüller wrote: On a second thought, you might be right. The attached patch does what you propose and works for me. OK to apply? This patch also fixes bug 3160: http://bugzilla.lyx.org/show_bug.cgi?id=3160 This patch looks fine. Regards, A/
Re: Bug #3812: Comments on patch
Alfredo Braunstein wrote: This patch looks fine. committed now. Jürgen
Re: Bug #3812: Comments on patch
Michael Gerz wrote:
> Maybe I was talking nonsense. I just looked at the patch (without the
> surrounding code). It looked like you check for the first character and
> then match(...) tries to match the complete string.
On a second thought, you might be right. The attached patch does what you
propose and works for me.
OK to apply?
Jürgen
Index: src/lyxfind.cpp
===
--- src/lyxfind.cpp (Revision 18720)
+++ src/lyxfind.cpp (Arbeitskopie)
@@ -62,7 +62,8 @@
{}
// returns true if the specified string is at the specified position
- bool operator()(Paragraph const & par, pos_type pos) const
+ // del specifies whether deleted strings in ct mode will be considered
+ bool operator()(Paragraph const & par, pos_type pos, bool del = true) const
{
docstring::size_type const size = str.length();
pos_type i = 0;
@@ -74,6 +75,8 @@
break;
if (!cs && uppercase(str[i]) != uppercase(par.getChar(pos + i)))
break;
+ if (!del && par.isDeleted(pos + i))
+break;
}
if (size != docstring::size_type(i))
@@ -101,20 +104,24 @@
};
-bool findForward(DocIterator & cur, MatchString const & match)
+bool findForward(DocIterator & cur, MatchString const & match,
+ bool find_del = true)
{
for (; cur; cur.forwardChar())
- if (cur.inTexted() && match(cur.paragraph(), cur.pos()))
+ if (cur.inTexted() &&
+ match(cur.paragraph(), cur.pos(), find_del))
return true;
return false;
}
-bool findBackwards(DocIterator & cur, MatchString const & match)
+bool findBackwards(DocIterator & cur, MatchString const & match,
+ bool find_del = true)
{
while (cur) {
cur.backwardChar();
- if (cur.inTexted() && match(cur.paragraph(), cur.pos()))
+ if (cur.inTexted() &&
+ match(cur.paragraph(), cur.pos(), find_del))
return true;
}
return false;
@@ -141,7 +148,8 @@
}
-bool find(BufferView * bv, docstring const & searchstr, bool cs, bool mw, bool fw)
+bool find(BufferView * bv, docstring const & searchstr, bool cs, bool mw, bool fw,
+ bool find_del = true)
{
if (!searchAllowed(bv, searchstr))
return false;
@@ -150,7 +158,8 @@
MatchString const match(searchstr, cs, mw);
- bool found = fw ? findForward(cur, match) : findBackwards(cur, match);
+ bool found = fw ? findForward(cur, match, find_del) :
+ findBackwards(cur, match, find_del);
if (found)
bv->putSelectionAt(cur, searchstr.length(), !fw);
@@ -177,7 +186,7 @@
int const ssize = searchstr.size();
DocIterator cur = doc_iterator_begin(buf.inset());
- while (findForward(cur, match)) {
+ while (findForward(cur, match, false)) {
pos_type pos = cur.pos();
Font const font
= cur.paragraph().getFontSettings(buf.params(), pos);
@@ -227,7 +236,7 @@
Cursor & cur = bv->cursor();
cap::replaceSelectionWithString(cur, replacestr, fw);
bv->buffer()->markDirty();
- find(bv, searchstr, cs, mw, fw);
+ find(bv, searchstr, cs, mw, fw, false);
bv->update();
return 1;
Index: src/BufferView.cpp
===
--- src/BufferView.cpp (Revision 18720)
+++ src/BufferView.cpp (Arbeitskopie)
@@ -665,6 +665,8 @@
{
FuncStatus flag;
+ Cursor & cur = cursor_;
+
switch (cmd.action) {
case LFUN_UNDO:
@@ -678,7 +680,7 @@
case LFUN_FILE_INSERT_PLAINTEXT:
case LFUN_BOOKMARK_SAVE:
// FIXME: Actually, these LFUNS should be moved to Text
- flag.enabled(cursor_.inTexted());
+ flag.enabled(cur.inTexted());
break;
case LFUN_FONT_STATE:
case LFUN_LABEL_INSERT:
@@ -691,7 +693,6 @@
case LFUN_NOTE_NEXT:
case LFUN_REFERENCE_NEXT:
case LFUN_WORD_FIND:
- case LFUN_WORD_REPLACE:
case LFUN_MARK_OFF:
case LFUN_MARK_ON:
case LFUN_MARK_TOGGLE:
@@ -703,9 +704,13 @@
flag.enabled(true);
break;
+ case LFUN_WORD_REPLACE:
+ flag.enabled(!cur.paragraph().isDeleted(cur.pos()));
+ break;
+
case LFUN_LABEL_GOTO: {
flag.enabled(!cmd.argument().empty()
- || getInsetByCode(cursor_, Inset::REF_CODE));
+ || getInsetByCode(cur, Inset::REF_CODE));
break;
}
@@ -1617,7 +1622,9 @@
FileDialog fileDlg(_("Select LyX document to insert"),
LFUN_FILE_INSERT,
make_pair(_("Documents|#o#O"), from_utf8(lyxrc.document_path)),
- make_pair(_("Examples|#E#e"), from_utf8(addPath(package().system_support().absFilename(), "examples";
+ make_pair(_("Examples|#E#e"),
+from_utf8(addPath(package().system_support().absFilename(),
+"examples";
FileDialog::Result result =
fileDlg.open(from_utf8(initpath),
Re: Bug #3812: Comments on patch
Jürgen Spitzmüller wrote: > On a second thought, you might be right. The attached patch does what you > propose and works for me. > > OK to apply? This patch also fixes bug 3160: http://bugzilla.lyx.org/show_bug.cgi?id=3160 Jürgen
Re: Bug #3812: Comments on patch
Jürgen Spitzmüller wrote: > Jürgen Spitzmüller wrote: >> On a second thought, you might be right. The attached patch does what you >> propose and works for me. >> >> OK to apply? > > This patch also fixes bug 3160: > http://bugzilla.lyx.org/show_bug.cgi?id=3160 This patch looks fine. Regards, A/
Re: Bug #3812: Comments on patch
Alfredo Braunstein wrote: > This patch looks fine. committed now. Jürgen
Bug #3812: Comments on patch
Jürgen,
your patch looks nice in general. I can imagine that it even fixes #3160. Below
please find some additional comments.
Michael
Index: src/lyxfind.cpp
===
--- src/lyxfind.cpp (Revision 18711)
+++ src/lyxfind.cpp (Arbeitskopie)
@@ -101,20 +101,26 @@
};
-bool findForward(DocIterator cur, MatchString const match)
+bool findForward(DocIterator cur, MatchString const match,
+bool find_del = true)
{
for (; cur; cur.forwardChar())
- if (cur.inTexted() match(cur.paragraph(), cur.pos()))
+ if (cur.inTexted()
+ (find_del || !cur.paragraph().isDeleted(cur.pos()))
+ match(cur.paragraph(), cur.pos()))
return true;
return false;
}
AFAICS, you only check the deletion status of the first character. However, ALL matching characters shouldn't be marked as deleted.
-bool findBackwards(DocIterator cur, MatchString const match)
+bool findBackwards(DocIterator cur, MatchString const match,
+bool find_del = true)
{
while (cur) {
cur.backwardChar();
- if (cur.inTexted() match(cur.paragraph(), cur.pos()))
+ if (cur.inTexted()
+ (find_del || !cur.paragraph().isDeleted(cur.pos()))
+ match(cur.paragraph(), cur.pos()))
return true;
}
return false;
The same here. Maybe we should propagate find_del to match(...).
+ case LFUN_WORD_REPLACE:
+ flag.enabled(!cur.paragraph().isDeleted(cur.pos()));
+ break;
+
... and here (but I am willing to accept this minor bug) ...
*** The End ***
Re: Bug #3812: Comments on patch
Michael Gerz wrote: for (; cur; cur.forwardChar()) - if (cur.inTexted() match(cur.paragraph(), cur.pos())) + if (cur.inTexted() + (find_del || !cur.paragraph().isDeleted(cur.pos())) + match(cur.paragraph(), cur.pos())) return true; return false; } AFAICS, you only check the deletion status of the first character. However, ALL matching characters shouldn't be marked as deleted. I don't understand. I just assure that a character is skipped on find if it is marked deleted. Since the loop moves ahead, this test applies to all characters of a word. And nothing will be marked anyway. Or do I miss something? Jürgen
Re: Bug #3812: Comments on patch
Jürgen Spitzmüller schrieb: Michael Gerz wrote: for (; cur; cur.forwardChar()) - if (cur.inTexted() match(cur.paragraph(), cur.pos())) + if (cur.inTexted() + (find_del || !cur.paragraph().isDeleted(cur.pos())) + match(cur.paragraph(), cur.pos())) return true; return false; } AFAICS, you only check the deletion status of the first character. However, ALL matching characters shouldn't be marked as deleted. I don't understand. I just assure that a character is skipped on find if it is marked deleted. Since the loop moves ahead, this test applies to all characters of a word. And nothing will be marked anyway. Or do I miss something? Maybe I was talking nonsense. I just looked at the patch (without the surrounding code). It looked like you check for the first character and then match(...) tries to match the complete string. If this isn't true, please ingore my comment and ask for another OK. (I have to leave in a few seconds.) Michael
Bug #3812: Comments on patch
Jürgen,
your patch looks nice in general. I can imagine that it even fixes #3160. Below
please find some additional comments.
Michael
Index: src/lyxfind.cpp
===
--- src/lyxfind.cpp (Revision 18711)
+++ src/lyxfind.cpp (Arbeitskopie)
@@ -101,20 +101,26 @@
};
-bool findForward(DocIterator & cur, MatchString const & match)
+bool findForward(DocIterator & cur, MatchString const & match,
+bool find_del = true)
{
for (; cur; cur.forwardChar())
- if (cur.inTexted() && match(cur.paragraph(), cur.pos()))
+ if (cur.inTexted() &&
+ (find_del || !cur.paragraph().isDeleted(cur.pos())) &&
+ match(cur.paragraph(), cur.pos()))
return true;
return false;
}
AFAICS, you only check the deletion status of the first character. However, ALL matching characters shouldn't be marked as deleted.
-bool findBackwards(DocIterator & cur, MatchString const & match)
+bool findBackwards(DocIterator & cur, MatchString const & match,
+bool find_del = true)
{
while (cur) {
cur.backwardChar();
- if (cur.inTexted() && match(cur.paragraph(), cur.pos()))
+ if (cur.inTexted() &&
+ (find_del || !cur.paragraph().isDeleted(cur.pos())) &&
+ match(cur.paragraph(), cur.pos()))
return true;
}
return false;
The same here. Maybe we should propagate find_del to match(...).
+ case LFUN_WORD_REPLACE:
+ flag.enabled(!cur.paragraph().isDeleted(cur.pos()));
+ break;
+
... and here (but I am willing to accept this minor bug) ...
*** The End ***
Re: Bug #3812: Comments on patch
Michael Gerz wrote: > > for (; cur; cur.forwardChar()) > > - if (cur.inTexted() && match(cur.paragraph(), cur.pos())) > > + if (cur.inTexted() && > > + (find_del || !cur.paragraph().isDeleted(cur.pos())) && > > + match(cur.paragraph(), cur.pos())) > > return true; > > return false; > > } > > AFAICS, you only check the deletion status of the first character. However, > ALL matching characters shouldn't be marked as deleted. I don't understand. I just assure that a character is skipped on find if it is marked deleted. Since the loop moves ahead, this test applies to all characters of a word. And nothing will be "marked" anyway. Or do I miss something? Jürgen
Re: Bug #3812: Comments on patch
Jürgen Spitzmüller schrieb: Michael Gerz wrote: for (; cur; cur.forwardChar()) - if (cur.inTexted() && match(cur.paragraph(), cur.pos())) + if (cur.inTexted() && + (find_del || !cur.paragraph().isDeleted(cur.pos())) && + match(cur.paragraph(), cur.pos())) return true; return false; } AFAICS, you only check the deletion status of the first character. However, ALL matching characters shouldn't be marked as deleted. I don't understand. I just assure that a character is skipped on find if it is marked deleted. Since the loop moves ahead, this test applies to all characters of a word. And nothing will be "marked" anyway. Or do I miss something? Maybe I was talking nonsense. I just looked at the patch (without the surrounding code). It looked like you check for the first character and then match(...) tries to match the complete string. If this isn't true, please ingore my comment and ask for another OK. (I have to leave in a few seconds.) Michael
