Fixing highlight/mark behavior #978 by cybermartini · Pull Request #980 · dail8859/NotepadNext

cybermartini · 2026-03-12T21:37:27Z

Fixing highlight/mark behavior #978

dail8859

Overall the changes seem reasonable and makes sense. Just a few comments/suggestions/tweaks.

dail8859 · 2026-03-12T23:12:52Z

src/decorators/MarkerAppDecorator.cpp

+    const QByteArray selText = editor->get_text_range(textStart, textEnd);
    Sci_TextToFind ttf {{0, (Sci_PositionCR)editor->length()}, selText.constData(), {-1, -1}};
-    const int flags = SCFIND_WHOLEWORD;
+    const int flags = (selectionStart != selectionEnd) ? SCFIND_MATCHCASE : SCFIND_WHOLEWORD;


I'm confused on this line. I'm not sure there's a need to enforce case matching if there was a selection.

I think maybe just:

const int flags = SCFIND_NONE;

Is sufficient.

Thoughts?

The intent was to have an exact match, so in the example when 'aa,b' is selected 'aa,B' would not match, but 'aa,b' would.

Yeah I agree with the match case now that I think about it.

For consistency this might be best:

const int flags = SCFIND_MATCHCASE;

The current logic would lead to different things getting marked depending on if the user selects a word, then marks it. Vs just having the cursor inside a word and marking it. I'd prefer consistency that can later be tweaked if it is even required.

dail8859 · 2026-03-12T23:16:39Z

src/decorators/SmartHighlighter.cpp

        return;
    }

+    const QByteArray selText = editor->get_text_range(selectionStart, selectionEnd);


If someone opens a large file and hits Ctrl+A it will create an entire copy of the file in this QByteArray. That's why get_text_range() is currently after the whole word checks.

Maybe some similar check as suggested above? Maybe they have to be on the same line? Or less than some length?

I didn't think of that, good point. Having it on the same line work be fine with me.

dail8859 · 2026-03-12T23:20:38Z

src/decorators/MarkerAppDecorator.cpp

-    const int wordEnd = editor->wordEndPosition(wordStart, true);
+    // If there is a selection, use it verbatim; otherwise fall back to the word at the cursor
+    int textStart, textEnd;
+    if (selectionStart != selectionEnd) {


Curious if someone selects multiple lines and then runs this. Not sure what should be the expected behavior. If the intent is to mark multiline strings I could potentially see that, or maybe that's not the intent of 'marking' things.

Just curious if this should do an extra check to see if selectionStart and selectionEnd are on the same line...or maybe see if they are < some predetermined number of characters apart.

If multiple lines are selected it does a check across multiple lines and there must be a match corresponding to the same number of matched lines selected. This will likely cause confusion.

aaa

aaa

aaa

aaa

If I select line 1 and 2, everything is selected, lines 3 and 4 also. If I select lines 1,2,3 it needs 3 lines in a row of 'aaa' so line 4 is not selected.

I'm thinking more of:

AAA BBB CCC AAA BBB CCC

If lines 1 and 2 are selected (e.g. AAA\nBBB) and marked, then it will also mark the second occurrence of it on lines 4 and 5. I think this is fine. For example, looking through a log file and wanting to find "chunks" of text.

Still might be good to put some arbitrary limit on the text (e.g 1KB or whatever). Luckily this section of code isn't something someone would be running at short intervals

dail8859 · 2026-03-12T23:27:05Z

src/decorators/SmartHighlighter.cpp


    Sci_TextToFind ttf {{0, (Sci_PositionCR)editor->length()}, selText.constData(), {-1, -1}};
-    const int flags = SCFIND_MATCHCASE | SCFIND_WHOLEWORD;
+    const int flags = isWholeWord ? (SCFIND_MATCHCASE | SCFIND_WHOLEWORD) : SCFIND_MATCHCASE;


I can understand the intent of SCFIND_WHOLEWORD here. If a whole word is selected, then only match whole words. I could see someone wanting different behavior but I'm fine with the way this is.

Now that I think about this more, I'm thinking of changing this to always use SCFIND_MATCHCASE only, and remove the isWholeWord check. It seems more intuitive to a user in my opinion.

Here's my fear with the current implementation. Every time a selection changes, it searches the whole file. If a user is selecting and dragging, then every time the cursor moves it is re-searching the entire file, which could be triggered veeeery rapidly.

This is why this original check was in place:

if (wordStart == wordEnd || wordStart != selectionStart || wordEnd != selectionEnd) { return; }

I'm thinking for now it would be best to treat the "SmartHighlighter" as purely a word highlighter. In theory this could always be exposed as a user option on how they want the smart highlighter to implement it, but I understand that's a good bit more work involved.

I know some editors just continually search for the selected text every time the selection is changed, but the way this code is implemented does not prevent any type of lag prevention or background threads to handle this on a non UI thread.

dail8859 · 2026-03-13T02:01:46Z

src/decorators/MarkerAppDecorator.cpp

-    const int wordEnd = editor->wordEndPosition(wordStart, true);
+    // If there is a selection, use it verbatim; otherwise fall back to the word at the cursor
+    int textStart, textEnd;
+    if (selectionStart != selectionEnd) {


I'm thinking more of:

AAA BBB CCC AAA BBB CCC

If lines 1 and 2 are selected (e.g. AAA\nBBB) and marked, then it will also mark the second occurrence of it on lines 4 and 5. I think this is fine. For example, looking through a log file and wanting to find "chunks" of text.

Still might be good to put some arbitrary limit on the text (e.g 1KB or whatever). Luckily this section of code isn't something someone would be running at short intervals

dail8859 · 2026-03-13T02:10:56Z

src/decorators/MarkerAppDecorator.cpp

+    const QByteArray selText = editor->get_text_range(textStart, textEnd);
    Sci_TextToFind ttf {{0, (Sci_PositionCR)editor->length()}, selText.constData(), {-1, -1}};
-    const int flags = SCFIND_WHOLEWORD;
+    const int flags = (selectionStart != selectionEnd) ? SCFIND_MATCHCASE : SCFIND_WHOLEWORD;


Yeah I agree with the match case now that I think about it.

For consistency this might be best:

const int flags = SCFIND_MATCHCASE;

The current logic would lead to different things getting marked depending on if the user selects a word, then marks it. Vs just having the cursor inside a word and marking it. I'd prefer consistency that can later be tweaked if it is even required.

dail8859 · 2026-03-13T02:23:19Z

src/decorators/SmartHighlighter.cpp


    Sci_TextToFind ttf {{0, (Sci_PositionCR)editor->length()}, selText.constData(), {-1, -1}};
-    const int flags = SCFIND_MATCHCASE | SCFIND_WHOLEWORD;
+    const int flags = isWholeWord ? (SCFIND_MATCHCASE | SCFIND_WHOLEWORD) : SCFIND_MATCHCASE;


Here's my fear with the current implementation. Every time a selection changes, it searches the whole file. If a user is selecting and dragging, then every time the cursor moves it is re-searching the entire file, which could be triggered veeeery rapidly.

This is why this original check was in place:

if (wordStart == wordEnd || wordStart != selectionStart || wordEnd != selectionEnd) { return; }

I'm thinking for now it would be best to treat the "SmartHighlighter" as purely a word highlighter. In theory this could always be exposed as a user option on how they want the smart highlighter to implement it, but I understand that's a good bit more work involved.

I know some editors just continually search for the selected text every time the selection is changed, but the way this code is implemented does not prevent any type of lag prevention or background threads to handle this on a non UI thread.

Fixing highlight/mark behavior dail8859#978

e43dd05

dail8859 requested changes Mar 12, 2026

View reviewed changes

dail8859 requested changes Mar 13, 2026

View reviewed changes

Uh oh!

Conversation

cybermartini commented Mar 12, 2026

Uh oh!

dail8859 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants