Update KJots to use KTextEdit #88

KEdit is deprecated and KTextEdit is one of the replacements. KTextEdit doesn't provide the find & replace functionality, so we have to re-implement that in KJotsEdit.

I assume you meant KEdit is deprecated.
Regarding the missing functionality, are you going to do more work for it or is this the final version of the PR?

My mistake, I did mean KEdit (I'll update the commit later). And yes, I'll be updating the PR to add the missing functionality.

Update: The search and repeatSearch are almost working, there is a small issue with repeatSearch where the data isn't correctly reset when the current entry is changed. Should be an easy fix.

Still need to implement the replace function.

I will have a look at this on Thursday.

I did a test. The new "standard" find/replace dialogs are nice, compared to the smaller original ones. Most things seem to work fine, but I found a problem with regex search.
I tried something like ^.*search_text.*$ and the whole text was selected instead of a single line. I tried the same search in Kate, on the same text and only a single like was selected instead. Do you see the same issue?

Another little issue: search for a word (even without regex). Continue the search till the end of the document. Press F3 again and KJots will ask to start searching from the beginning of the document again. Say yes, but notice how nothing is searched. It is necessary to press F3 again to actually get a search from the beginning of the document.
This does not happen in the original KJots, not it happens in Kate. In both cases after confirming the "search from the beginning of the document", the first match get found and selected.

I tried something like ^.*search_text.*$ and the whole text was selected instead of a single line. I tried the same search in Kate, on the same text and only a single like was selected instead. Do you see the same issue?

Yep, happens for me as well. I'll take a look at this. It could be an issue with how the highlighting is performed, or the conversion between index and paragraph/index. (TQTextEdit uses paragraph/index-from-paragraph-start while KFind and KReplace use index-from-text-start.)

It seems that Kate doesn't use the KFind and KReplace classes that I opted for here, instead opting for a custom implementation. So it could be an issue with those classes, although this is unlikely.

Another little issue: search for a word (even without regex). Continue the search till the end of the document. Press F3 again and KJots will ask to start searching from the beginning of the document again. Say yes, but notice how nothing is searched. It is necessary to press F3 again to actually get a search from the beginning of the document.

Good point! I agree that it should automatically start the search again.

I tried something like ^.*search_text.*$ and the whole text was selected instead of a single line.

After looking into it a bit more, this seems to be a limitation of TQRegExp which doesn't support multi-line matching.

We need ^ (caret) which when it is the first character in the regexp means that the regexp must match from the beginning of the string. And we also need $ (dollar) which when it is the last character in the regexp means that the regexp must match until the end of the string.

[...]

TQRegExp does not have an equivalent to Perl's /m option, but this can be emulated in various ways for example by splitting the input into lines or by looping with a regexp that searches for newlines.

[TQRegExp Documentation]

Not sure I have the right understanding of "looping with a regexp that searches for newlines", but I'll take a look at splitting the text into lines if the regex option is selected.

As an aside: I wonder if adding an option to change the behaviour of ^ and $ should be added to TQRegExp? I think most people nowadays would expect they match the start/end of a line, not the entire string.

Made some changes so find/replace now operates on paragraphs, rather than the whole text. This allows the regex ^.*some_text.*$ to work as expected. I've also fixed clicking "Continue" from the final "Find Next" dialog so it will perform a search automatically.

Made some changes so find/replace now operates on paragraphs, rather than the whole text. This allows the regex ^.some_text.$ to work as expected. I've also fixed clicking "Continue" from the final "Find Next" dialog so it will perform a search automatically.

Great, will do a new test and feedback, probably tomorrow.

I wonder if adding an option to change the behaviour of ^ and $ should be added to TQRegExp? I think most people nowadays would expect they match the start/end of a line, not the entire string.

If I understand correctly from your comments, the issue may not be in TQRegExp but the fact that KJots search treats the whole book as a single string? Haven't looked into the code, nor into how you fixed that, so I am jsut asking out of curiosity.

Great, will do a new test and feedback, probably tomorrow.

No worries, take your time :-)

I wonder if adding an option to change the behaviour of ^ and $ should be added to TQRegExp? I think most people nowadays would expect they match the start/end of a line, not the entire string.

If I understand correctly from your comments, the issue may not be in TQRegExp but the fact that KJots search treats the whole book as a single string? Haven't looked into the code, nor into how you fixed that, so I am jsut asking out of curiosity.

Pretty much. KJots was passing the entire contents of the entry as a single string, which caused the issue you experienced with the regex, since ^ and $ in TQRegExp match the start and end of the string respectively. The solution was to iterate through each paragraph (which are separated by newlines) and pass them to TQRegExp.

My comment was whether TQRegExp should support a multi-line mode, since it's a common modifier in other regex engines. It's not important for this PR, just a thought.

My comment was whether TQRegExp should support a multi-line mode, since it's a common modifier in other regex engines. It's not important for this PR, just a thought.

That could indeed be handy. We can also look at Qt4/5/6 and see if something like that has already been implemented. It may be as easy as a backport of some code.

I did a new test. The regex issue with ^ and $ and the issue with search from begin/end of file are now fixed. Well done.
A couple of more points I picked up during testing and that we can improve:

pressing F3 search again in the selected direction. I think users expect Shift+F3 to search in the opposite direction, like it does in Kate. In KJots Shift+F3 does not do anything
if the search text is not found, Kate will open a dialog saying "text not found" and stop searching. KJots asks to search from the beginning of the file and will continue to prompt the user till he presses "stop"
select some text before opening the search dialog. In Kate the "selected text" checkbox is enabled and the search only happens in the selected text if the checkbox is checked. In KJots the checkbox is grayed out and search is always happening on the complete text.

The first two points are probably easy fixes, the third one may need a bit more time. Once all are fixed, I think the functionality will be pretty good and we can look at a final review and merge, unless some other things pop up in the meanwhile.

Side note: I did not know this app, I was using KNotes or Kate to take simple notes. KJots fits in very nicely for that purpose :-)

pressing F3 search again in the selected direction. I think users expect Shift+F3 to search in the opposite direction, like it does in Kate. In KJots Shift+F3 does not do anything

This wasn't originally in KJots, but I've added it in the most recent push since it does make sense to support both forward and backwards searching.

There was an issue with the final count shown (when you reach the end of the document) as it counts the number of times the highlight() signal is emitted. Meaning if you swapped between searching forwards and backwards, you would likely end up with an incorrect number of matches.

This was "fixed" by not including the count in the final dialog, which I think works just as well. Just wanted to point this out in case you notice.

if the search text is not found, Kate will open a dialog saying "text not found" and stop searhcing. KJots asks to search from the beginning of the file and will continue to prompt the user till he presses "stop"

select some text before opening the search dialog. In Kate the "selected text" checkbox is enabled and the search only happens in the selected text if the checkbox is checked. In KJots the checkbox is grayed out and search is always happening on the complete text.

The first two points are probably easy fixes, the third one may need a bit more time.

I'll take a look at implementing these over the next couple of days. As you say, it's probably the last one that might take some extra time.

Side note: I did not know this app, I was using KNotes or Kate to take simple notes. KJots fits in very nicely for that purpose :-)

It is nice! I find it much easier to organize my notes, which were previously written on various little note pads laying around. (Especially since I never left enough blank pages between topics for when I revisited them 😅)

It is nice! I find it much easier to organize my notes, which were previously written on various little note pads laying around. (Especially since I never left enough blank pages between topics for when I revisited them 😅)

Same here, I pretty much started using it immediately and replaced all my KNotes that were flying around on my desktop ;-)

Ping me here when you are done with those remaining points, I will do a further test then.

Okay @MicheleC, should be good for another look now ( when you get a chance :-) )

Thanks @mio, I was actually wondering last night where we were up to with this one.
I have done a new test and here is the next round of feedback, with reference to the 3 items I highlighted here:

item 1 is working fine, well done
item 2 is working fine. I noticed a small difference between Kate and KJots. If a text is not found, both displays a messagebox. Pressing F3 again in Kate will try to search again for the same not found text. Pressing F3 in KJots brings up the search dialog instead. I would say Kate's behavior is consistent regardless of whether the string is found or not, while KJots behaves slightliy different in the two scenarios. It is not a big thing, what is your opinion about it?
search in selection works fine but there are a couple of minor changes required.

if some text is selected and we press Ctrl+F, the Selected text checkbox should be checked by default when the search dialog opens (see Kate's behavior)
when reaching the end of the selection, Kate says End of selection reached while KJots says End of document reached which is a bit misleading

Item 3's points should be fixed.
Item 2's point is debatable, so I am interested in your opinion on that. IMO we should fix it too, but I don't want to force it :-)

item 2 is working fine. I noticed a small difference between Kate and KJots. If a text is not found, both displays a messagebox. Pressing F3 again in Kate will try to search again for the same not found text. Pressing F3 in KJots brings up the search dialog instead. I would say Kate's behavior is consistent regardless of whether the string is found or not, while KJots behaves slightliy different in the two scenarios. It is not a big thing, what is your opinion about it?

That does make sense, considering the action is called "Find Again", one might expect it to perform the search again even if nothing was found the last run. Will fix.

search in selection works fine but there are a couple of minor changes required.

if some text is selected and we press Ctrl+F, the Selected text checkbox should be checked by default when the search dialog opens (see Kate's behavior)

when reaching the end of the selection, Kate says End of selection reached while KJots says End of document reached which is a bit misleading

I can implement the first point, no problem. The second one would probably be better addressed in the KFind class. (Kate uses its own dialog, whereas this patch uses the shouldRestart method provided by KFind.)

That does make sense... Will fix.

Great, thanks!

I can implement the first point, no problem.

👍

The second one would probably be better addressed in the KFind class

Sounds good to me. This way we can probably also simplify the Kate code afterwards.

Let's fix the first two items and then merge this PR. Subsequently you can fix the last point in KFind as you suggested.

Okay, should be good for another check now @MicheleC

I have done another tests and both item 1 and 2 are ok. Item 3 we will address on a separate PR.
I ran into one more bug during testing. Say you have the following text somewhere in kjots:

blah
first if second if third if forth if
first if second if third if forth if
first if second if third if forth if
blah

Select the three lines with ifs and search for if. Repeat a few searches and stop for example of the first if on the middle line. All works fine so far. Note that if is selected. Now press Ctrl+f to open the search dialog again and search for example for o: you would expect a failed search since the selection is now if, instead all the os in the middle line are found. Note the os on the other lines are correctly not found.
Try the same in Kate and you will see no os are found at all because the selection does not contain any o.
I think this is really the last bug, I ran into it by mistake actually 😰

I think this is really the last bug, I ran into it by mistake actually 😰

Nice! I'll take a look at fixing this today or tomorrow.

I ran into one more bug during testing [...]

The new changes should address this. Let me know what you think when you get a chance.

Just done a new test. The latest commit fixes the issue described before but introduce a new problem :-(

Select some text, search forward for something that exists in it a few times. When you reach the end of the selection, you get prompted to start again from the beginning, say yes and everything works fine.

Repeat but searching backward: this time after you get asked to start again from the end of the document, the text is not found. Please note this only happens when searching in a selection. If you do a backward search on the whole document, everything works just fine.

I know I am a bit picky in my testing, but I think it is important we commit good code after proper testing rather than rushing an incomplete/buggy solution. Please bear with me :-)

Repeat but searching backward: this time after you get asked to start again from the end of the document, the text is not found. Please note this only happens when searching in a selection. If you do a backward search on the whole document, everything works just fine.

Hmm, I don't experience this. Are you searching with the "Find backwards" option checked or by pressing SHIFT+F3? (or both?)

I know I am a bit picky in my testing, but I think it is important we commit good code after proper testing rather than rushing an incomplete/buggy solution. Please bear with me :-)

Don't worry about it! I appreciate you taking the time to test it so thoroughly :-)

Hmm, I don't experience this. Are you searching with the "Find backwards" option checked or by pressing SHIFT+F3? (or both?)

I checked the "Find backbard" checkbox in the Find dialog and then use F3 for repeated searches. I haven't tried Shift+F3 in this case, I can build again and do another test if you want.

Don't worry about it! I appreciate you taking the time to test it so thoroughly :-)

Thanks 🤝

I checked the "Find backbard" checkbox in the Find dialog and then use F3 for repeated searches. I haven't tried Shift+F3 in this case, I can build again and do another test if you want.

If you don't mind. Even when trying with both methods, I still don't have the same issue.

The latest version fixed an issue with searching in selected text where starting/ending a selection halfway through a line wasn't working correctly. (It would continue to search outside of the selection).

Not sure if this will fix your issue as well.

The last commit seems to fix the last issue, so we can finally proceed and merge this PR. Great job and thanks for all the adjustments along the way @mio.

We can also backport to R14.1.x.

Thanks for reviewing @MicheleC!

     : KTextEdit(parent, name)
     , m_entry(nullptr)
     , m_find(nullptr)
     , m_findDialog(nullptr)

Update KJots to use KTextEdit #88

Reviewers