[REGRESSION] "Open Selected PathName(s)" command not working

[donho]

Is there an existing issue for this?

• I have searched the existing issues

Description of the Issue

"Open Selected PathName(s)" command does not open all files while selection all Search results.

Steps To Reproduce

1. Search (Find in Files) "1660" (without quotes) in notepad-plus-plus\PowerEditor\installer\nativeLang\
2. There will be : (87 hits in 87 files of 94 searched) [Normal: Word]
3. Don't scroll down
4. Ctrl-A to select all Search results
5. Run "Open Selected PathName(s)" command

Current Behavior

Less 87 files are opened.

Expected Behavior

All 87 files are opened.

Debug Information

Notepad++ v8.7.4   (64-bit)
Build time : Dec  4 2024 - 23:50:05
Path : C:\Program Files\Notepad++\Notepad++.exe
Command Line : 
Admin mode : OFF
Local Conf mode : OFF
Cloud Config : OFF
Periodic Backup : ON
Placeholders : OFF
DirectWrite : ON
Multi-instance Mode : monoInst
File Status Auto-Detection : cdEnabledNew (for current file/tab only)
Dark Mode : OFF
OS Name : Windows 11 Home (64-bit)
OS Version : 23H2
OS Build : 22631.4602
Current ANSI codepage : 1252
Plugins : 
    mimeTools (3.1)
    NppConverter (4.6)
    NppExport (0.4)

Anything else?

If scroll down the search result to the end then all 87 files can be opened.

The regression could be introduced by:
b822480
4671826

[donho]

@alankilborn
Could you check it?

[alankilborn]

Yes, I will, in the next 24 hours (traveling now).

[donho]

@alankilborn
Bon voyage then!

[alankilborn]

I can reproduce the problem: I created a scenario where I expected 396 tabs to be opened; I get only 5 opened!

Please grant me some additional time to continue looking into this, as my time behind the keyboard is limited at the moment.

[alankilborn]

@donho

Regarding b822480

If you'll recall, you immediately replaced the code of that commit with some code that "softmgr" supplied; see #15741 (comment)
So, THAT is the code that is currently in place (basically), rather than my original version per the commit cited.

---

But... I did look into the problem, regardless of who authored it, and on a quick patch basis (still traveling, screen time is limited for thorough testing), I might suggest you try the following replacement for the getResultFilePaths() function:

vector<wstring> Finder::getResultFilePaths(bool onlyInSelectedText) const
{
	size_t fromLine = 0, toLine = 0;

	if (onlyInSelectedText)
	{
		const pair<size_t, size_t> lineRange = _scintView.getSelectionLinesRange();
		fromLine = lineRange.first;
		toLine = lineRange.second;
	}
	else
	{
		toLine = _scintView.execute(SCI_GETLINECOUNT) - 1;
	}

	size_t len = _pMainFoundInfos->size();
	vector<wstring> paths;

	for (size_t line = fromLine; line <= toLine; ++line)
	{
		const int lineFoldLevel = _scintView.execute(SCI_GETFOLDLEVEL, line) & SC_FOLDLEVELNUMBERMASK;
		if (lineFoldLevel == fileHeaderLevel)
		{
			// fileHeaderLevel lines don't have path info; have to look into the NEXT line for it,
			// but only need to do something special here if we are on the LAST line of the selection
			if (line == toLine)
			{
				++line;
			}
		}

		if (line < len)
		{
			wstring& path2add = (*_pMainFoundInfos)[line]._fullPath;
			if (!path2add.empty())
			{
				// make sure that path is not already in
				if (std::find(paths.begin(), paths.end(), path2add) == paths.end())
				{
					paths.push_back(path2add);
				}
			}
		}
	}

	return paths;
}

Notes:

• I hadn't really looked at the FoundInfo structure before (my original commit didn't access it), and, when I looked at its contents during debug, I was a bit shocked to see that, if a file has e.g. 1000 hits, that the pathname string is repeated 1000 times in the structure. It seems redundant and wasteful. Perhaps I will suggest a refactoring in a separate "issue"?
• Aside from suggesting a bug fix, I didn't change the algorithm here for how Copy/Open Selected Pathname(s) works. I still plan to do that via [Feature request] Copy/Open Selected Pathname(s) should work like Copy Selected Line(s) #15948 .
• As I said, the above suggested code is "quick and dirty"; even a quick glance at it now suggests improvements: (1) the fold-level check could be moved ahead of the for loop and (2) the path most-recently added to paths could be remembered and then when another path is about to be added it could be checked against the saved value (avoids extra std::find searching).

[donho]

@alankilborn
It seems const int lineFoldLevel = _scintView.execute(SCI_GETFOLDLEVEL, line) & SC_FOLDLEVELNUMBERMASK; doesn't work well while the line is not visible - it could be the scintilla control of finder is not initialized correctly.

It seems redundant and wasteful. Perhaps I will suggest a refactoring in a separate "issue"

No, please keep it as it is - the change of structure could draw a lot of regressions.

if (!path2add.empty()) is indeed a workaround solution. Well played.
You can do a PR to fix it, if you are on time. Otherwise I will integrate it for the next release.

[alankilborn]

if you are on time. Otherwise I will integrate it for the next release.

Let's wait as I want to look further into this:

It seems const int lineFoldLevel = _scintView.execute(SCI_GETFOLDLEVEL, line) & SC_FOLDLEVELNUMBERMASK; doesn't work well while the line is not visible - it could be the scintilla control of finder is not initialized correctly.

"while the line is not visible" did not come into play when I was replicating your REGRESSION from this issue -- I was not trying to open files using collapsed text from Search results -- and I could still replicate, so...

[donho]

Let's wait as I want to look further into this:

OK, you can always create PR if you have a better solution.
But since the new release is about to be out, and it's a regression (Yes MY regression - Sorry for the false accusation) - Open all pathnames faillure - I'll include the current solution in the v8.7.5

[alankilborn]

you can always create PR if you have a better solution.
But ... I'll include the current solution in the v8.7.5

If the "current solution" has no bug (I hope), then it probably isn't worth seeking that "better solution" now.

Yes MY regression - Sorry for the false accusation

Was it an "accusation" or did you just have confidence in me that I would find a fix? :-)
Thank you for accepting my code THIS time. :-)

[donho]

If the "current solution" has no bug (I hope), then it probably isn't worth seeking that "better solution" now.

I have tested it quite a lot - so far so good. So if there's no more bug, I think we will keep it.

Was it an "accusation" or did you just have confidence in me that I would find a fix? :-)

I don't blame to you but it's due to the change of behaviour (Open All files -> Open Selected PathNames) from you.
(Now you know why I don't like usually to change the behaviour)
OTOH, if you're not capable I won't to ping you.

[alankilborn]

Now you know why I don't like usually to change the behaviour

But change is worth the risk to bring better features, don't you think?
For me, the older open behavior was unusable, or annoying, because I would have to ClearAll first, then repeat a search.

---

if you're not capable I won't to ping you.

Well then I hope to remain capable. :-)