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 error in file path #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

user1823
Copy link

Add missing backslashes

Add missing backslashes
@GitHubRulesOK
Copy link
Owner

GitHubRulesOK commented Mar 21, 2024

@user1823
the syntax should be correct as %~DP0% should be a folder c:\example\ thus adding an extra one will be c:\example\\mutool.exe it is an oddity that should "TODO" be addressed a different way the idea was to change these over to SumatraPDF+ but covid and other family issues have left me less time to develop that simpler approach. The idea was to become, add files that can be standalone OR add-in as a single ...SumatraPDF\plus\ folder with less complex folder subdivisions than here.

to see what happens at command line run echo echo %~dp0%>testdp.cmd&testdp

@user1823
Copy link
Author

user1823 commented Mar 21, 2024

the syntax should be correct as %~DP0% should be a folder c:\example\ thus adding an extra one will be c:\example\mutool.exe

But, trying to use the script without adding the extra backslashes gives me the error "Either MuTool.exe or this file are not in correct location".

The example in the file (see below) doesn't have a backslash at the end.

system-wide environment variable e.g. SET addins=D:\location of\SumatraPDF\addins

Also, when using the Browse Directory option in the Edit System Variable screen, Windows doesn't automatically add the backslash to the path.

So, I think that it is better to assume that the path contained in %addin% doesn't end in a backslash.

Also, it seems that having an extra backslash is not harmful.

Edit:

the syntax should be correct as %~DP0% should be a folder c:\example\

I now got what you were saying.

However, because of the following code, addins is not assigned the value of %~DP0% if I have already set the value of addins in my system variables (which you have advised in line 46).

if "%addins%"=="" set "addins=%~dp0"

@GitHubRulesOK
Copy link
Owner

updated the example to show a terminal \ so set addins should be better for most users but these scripts do need test or vary for your own cases and .exe version. As basically should have been replaced with better more universal variants in the other repository (which was initially AutoHotKey extras) but many features now included in main SumatraPDF so needs a full reboot of concepts.

@user1823
Copy link
Author

user1823 commented Mar 23, 2024

I tried changing it to %addins%\\mutool.exe in my local file and it still works as expected. So, I think that adding an extra backslash is the safer choice because there may be some users who don't have the terminal backslash in their system variables.

In short, I don't see any problem arising due to addition of these backslashes. So, I think that they should be added.

If you are still not convinced, feel free to close this PR.

@GitHubRulesOK
Copy link
Owner

GitHubRulesOK commented Mar 23, 2024

the reason I am holding back is I dont have the time to check all the permutations so if it works dont fix it as any old loose "%..%\mutool" would be attempting to open the root.
The mutool command needs to be standalone whatever %cd% and %cd% itself should be \ terminated but is not.
Thus %~dp#% is the current issue it realy needs last \ character removing and adding back as required.
I have just recompiled mutool 1.25x32 so intend to try that in Plus where the time testing will be sumatraPDF\plus\mupdf\mutool.exe

@user1823
Copy link
Author

The latest commit was just to share my latest local code with you. It completely avoids the problem of adding the mutool.exe in the same folder. Mutool is in my system path, which allows me to call it from anywhere.

If you don't have the time to test it, fine. But if you do have some time in the future, I think that this approach is more clean.

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.

2 participants