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

PDFBridgeSumatra: Detect use of fork on runtime #263

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

swiftgeek
Copy link
Contributor

@swiftgeek swiftgeek commented Sep 5, 2022

Not even using windows, and I have not seen build of the forked sumatra yet.

But tested with official 32bit sumatra 3.4.6 under wine and win10, and PDF search works!


To get SDL log output/stdout I had use following steps when bulding, as currently /usr/x86_64-w64-mingw32/lib/cmake/SDL2/sdl2-config.cmake from my distro package includes -mwindows unconditionally, which has unfortunate effect of disabling all stdout

mkdir win_build
cd win_build
CFLAGS='-O3 -march=native' CXXFLAGS='-O3 -march=native' cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_TOOLCHAIN_FILE=../Toolchain-mingw64.cmake ../
sed -i 's/-mwindows//g' src/openboardview/CMakeFiles/openboardview.dir/link.txt
sed -i 's/-mwindows//g' src/openboardview/CMakeFiles/openboardview.dir/linklibs.rsp
make -j3

Copy link
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

I'm a bit rusty on C++, so I may say something silly. But I've had a quick once over and left some comments.

@@ -139,6 +139,38 @@ bool PDFBridgeSumatra::InitializeDDE() {
return true;
}

bool PDFBridgeSumatra::TestFork() {
if (this->ForkTested)
return true; // Already determined
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return true; // Already determined
return this->IsForked; // Already determined

Why can't this return this->IsForked? You don't currently use the return value when calling this method, but to me it makes sense to use this method to check if Sumatra is forked 🤔

e.g. if (this->TestFork()) { /* do stuff */ }

Should probably document what the return value is for if it's for something else (like indicating an error state)

Comment on lines +344 to +349
std::wstring ddeCmd;
this->TestFork();
if (this->IsForked)
ddeCmd = L"[Search(\"" + this->pdfPathString + L"\",\"" + wstr + L"\"," + (caseSensitive ? L"1" : L"0") + L")]";
else
ddeCmd = L"[Search(\"" + this->pdfPathString + L"\",\"" + wstr + L"\")]";
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be simplified to if (this->TestFork()); if the above suggestion is taken. Not sure how I feel about having the assignment nested in an if..else vs having an additional ternary in the assignment... I think split over two or three lines it's still fairly readable?

Suggested change
std::wstring ddeCmd;
this->TestFork();
if (this->IsForked)
ddeCmd = L"[Search(\"" + this->pdfPathString + L"\",\"" + wstr + L"\"," + (caseSensitive ? L"1" : L"0") + L")]";
else
ddeCmd = L"[Search(\"" + this->pdfPathString + L"\",\"" + wstr + L"\")]";
std::wstring ddeCmd = L"[Search(\"" + this->pdfPathString + L"\",\"" + wstr + L"\""
+ (this->TestFork() ? L"," + (caseSensitive ? L"1" : L"0") : L"") + L")]";

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

2 participants