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

[LUA] fix - Lua Scripts that change the cwd also affect Aegisub #167

Closed

Conversation

Totto16
Copy link

@Totto16 Totto16 commented Jul 23, 2022

Lua Scripts that change the cwd can also affect Aegisub, here I save the cwd before the registering of the Lua scripts and then restore it afterwards, for more see #166

@Totto16
Copy link
Author

Totto16 commented Jul 23, 2022

A Problem that I encountered is, that these scriptsrun asynchronus, so the exact time when a possible cwd change happens, is unknown, so a better way to solve this would to pass the saved start cwd to each component, or make it available in main, so that it can be guaranteed, that it is the one, aegisub got started in

@FichteFoll
Copy link
Contributor

Another problem with this approach is that automation scripts may change the working directory whenever they execute code, which is also when their macros are executed, making this approach not exactly stable.

A better approach would be not to use relative paths to the workding dir in the code at all or as little as possible, resolving relative paths explicitly to a known directory instead. Except for loading files from the command line, I don't think any of the current features rely on the working directory.

@Totto16
Copy link
Author

Totto16 commented Jul 25, 2022

The second approach saves the start cwd and makes it accessible to all those, who want it, that happens only once, so you are right, but scripts can modify the cwd only when they're run: 1: at startup, 2: at manual execution of the script in the automation tab. So the second approach is 100% secure and we "need" the cwd for relative path resolvinf, it's only needed for opening a .ass, but it's more conveninet to open it from the terminal like : aegisub ./test.ass rather than aegisub $(realpath ./test.ass) or aegisub /really/long/absolute/path/test.ass

@FichteFoll
Copy link
Contributor

FichteFoll commented Jul 26, 2022

I admit I haven't checked the PR contents (and notably the second commit) and only read the comments here in the PR. This approach is indeed what I described and what should be used. Ideally it would also use startCwd in all the places where relative paths are currently supported to explictly tie them together, but I know practically nothing about the codebase to make any more substantial comments currently.

@Totto16
Copy link
Author

Totto16 commented Jul 26, 2022

As far As I found and knew, thats the only place it's used explicitly, there may be other calls then current_path() that use the cwd, thats also out of my knowledge about that

@Totto16
Copy link
Author

Totto16 commented Aug 4, 2022

The CI fails are related to #162

@Totto16 Totto16 closed this Dec 1, 2023
@Totto16 Totto16 deleted the relative-files-lookup-fix branch December 1, 2023 12:41
@Totto16 Totto16 restored the relative-files-lookup-fix branch December 3, 2023 21:36
@Totto16 Totto16 deleted the relative-files-lookup-fix branch December 3, 2023 22:07
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