Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Panel.Select #691

Merged
merged 1 commit into from May 19, 2023
Merged

Fix Panel.Select #691

merged 1 commit into from May 19, 2023

Conversation

johnd0e
Copy link
Contributor

@johnd0e johnd0e commented May 18, 2023

@johnd0e
Copy link
Contributor Author

johnd0e commented May 18, 2023

@shmuz

@shmuz
Copy link
Contributor

shmuz commented May 18, 2023

Looks good but you need to build Far and test.
Here is my test (assuming TmpPanel with files as here is on the active panel):

local a={}
for i=2,APanel.ItemCount do
  a[#a+1] = Panel.Item(0, i)
end
for k=1,3 do
  local m=k==1 and 1 or k==2 and 0 or 2 -- select, deselect, invert
  Panel.Select(0, m, 2, table.concat(a,'\n'))
  far.Text()
  win.Sleep(2000)
end

@johnd0e johnd0e force-pushed the fixPanelSelect branch 2 times, most recently from 5422070 to c58a086 Compare May 19, 2023 00:32
@johnd0e johnd0e marked this pull request as ready for review May 19, 2023 00:32
@alabuzhev
Copy link
Contributor

This is a breaking change.
Currently Panel.Select(0, 1, 2, "file.ext") works on TmpPanel, after the change it won't, and the documentation explicitly allows omitting full paths.

We should fall back to the current logic if the item is not found, something like

-	const auto Pos = FindFile(PointToName(i), true);
+	auto Pos = FindFile(i, false);
 	if (Pos == -1)
-		continue;
+	{
+		Pos = FindFile(PointToName(i), true);
+		if (Pos == -1)
+			continue;
+	}

@shmuz
Copy link
Contributor

shmuz commented May 19, 2023

Currently Panel.Select(0, 1, 2, "file.ext") works on TmpPanel

I wouldn't call that "works" as in case there are several "file.ext" on TmpPanel only 1 of them is processed.

@shmuz
Copy link
Contributor

shmuz commented May 19, 2023

See my backward-compatibility fix: shmuz/far2m@4067c52

@shmuz
Copy link
Contributor

shmuz commented May 19, 2023

@johnd0e
Вместо LGOOD_SLASH поставьте L'\\'

far/filelist.cpp Outdated
@@ -982,7 +982,7 @@ long long FileList::VMProcess(int OpCode,void *vParam,long long iParam)
if (i.empty())
continue;

const auto Pos = FindFile(PointToName(i), true);
const auto Pos = FindFile(i, !wcschr(namePtr,LGOOD_SLASH));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't compile.
!wcschr(namePtr,LGOOD_SLASH) -> !i.contains(path::separator).

This is still a bit incompatible: previously it was possible to select names like "q:\non_existing_or_completely_unrelated_path\file.ext", as long as there is file.ext on the panel, but we can probably live with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we can probably live with that.

Hmm.. I rather see that as breaking change

far/filelist.cpp Outdated
@@ -982,7 +982,7 @@ long long FileList::VMProcess(int OpCode,void *vParam,long long iParam)
if (i.empty())
continue;

const auto Pos = FindFile(PointToName(i), true);
const auto Pos = FindFile(i, !i.contains(path::separator));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just realized this won't compile too (or at least not everywhere).

!i.contains(path::separator) -> !contains(i, path::separator)

@johnd0e
Copy link
Contributor Author

johnd0e commented May 19, 2023

This is still a bit incompatible: previously it was possible to select names like "q:\non_existing_or_completely_unrelated_path\file.ext", as long as there is file.ext on the panel, but we can probably live with that.

@alabuzhev
Could you please provide sample code illustrating that incompability?
Because for me that seems still working.

lua:Panel.Select(0,1,2,"file.ext")

@alabuzhev
Copy link
Contributor

Could you please provide sample code illustrating that incompability?

cd %farhome%
lua:Panel.Select(0, 1, 2, "banana\\far.exe")

I'm not saying it makes any sense though.
However, the documentation also mentions Panel.Select(0,1,2,clip(0)), so one might imagine a scenario like "select files that I copied into clipboard in some other directory".

@johnd0e
Copy link
Contributor Author

johnd0e commented May 19, 2023

Ah, I see now.
So in my opinion this new behavior is more correct (and corresponds to docs even more).

@shmuz ?

@shmuz
Copy link
Contributor

shmuz commented May 19, 2023

In my opinion the current solution is OK.

BTW, when I and zg were changing the macrosystem to Lua, zg removed all macroapi functions (he thought they were unnecessary as we had LuaFAR API). Later I put them back.

@alabuzhev alabuzhev merged commit 33131a6 into FarGroup:master May 19, 2023
25 of 26 checks passed
@johnd0e johnd0e deleted the fixPanelSelect branch May 19, 2023 23:32
@shmuz
Copy link
Contributor

shmuz commented May 20, 2023

I made another change (not committed yet; as I'm writing the test for Panel.Select).
See the snippet below and pay attention to the new variable RegularPanel:

auto SelectByList = [&] (ps_action act)
{
	Result=0;
	const wchar_t *name;
	bool RegularPanel = !GetPluginHandle();
	for(size_t I = 0; (name=itemsList.Get(I)) != nullptr; ++I)
	{
		int Pos;
		auto arg1 = RegularPanel ? PointToName(name) : name;
		auto arg2 = RegularPanel || !wcschr(name,LGOOD_SLASH);
		if ((Pos=FindFile(arg1,	arg2)) != -1)
		{
			Select(ListData[Pos], act==remove?FALSE : act==add?TRUE : !ListData[Pos]->Selected);
			Result++;
		}
	}
};

@shmuz
Copy link
Contributor

shmuz commented May 20, 2023

The commit is here. Also, a 90-line test function was added.

alabuzhev added a commit that referenced this pull request May 20, 2023
@alabuzhev
Copy link
Contributor

Thanks, synchronized.

@shmuz
Copy link
Contributor

shmuz commented May 20, 2023

Thanks, synchronized.

I'm a little confused. There are some API differences between Far3 and far2m and such a synchronization is a ... non-trivial task.
(I'm speaking about the tests).

@alabuzhev
Copy link
Contributor

I've picked what I could.

@shmuz
Copy link
Contributor

shmuz commented May 20, 2023

I didn't (yet) look at your commit but for example in the test added today:
far2m: panel.GetPanelInfo(1)
Far3: should be panel.GetPanelInfo(nil,1)

... But this may work for Far3 ...
I'll try to look at your commit.

@shmuz
Copy link
Contributor

shmuz commented May 20, 2023

@alabuzhev
Well done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants