-
Notifications
You must be signed in to change notification settings - Fork 551
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 for automatic relinking of missing media files during project open #2275
Conversation
Neato! Will test this out soon! [Glad to see your PR here, BTW] Thank you for the good work! 👍 |
No worries, and sorry it took me so long. I've had this sitting around for a few months already, I just needed to sort it out and bring it up to date. I'll submit the second part shortly, to allow relinking of individual files. I decided to keep it separate as the second part is a feature that can go wrong when the user relinks to a completely different file. More details in the description of part 2... |
@peanutbutterandcrackers - Do you have time to test this one? I'm away this weekend from my dev PC. |
@DylanC - Right on it, Cap'ain! @fschill - Sorry I couldn't get to this sooner. Test 1:File
Still, this is a LOT better than what we currently have. I strongly advise polishing this PR up, according to test results, and merging it in. Great work, good sir, @fschill ! 👍 Also pinging @ferdnyc and @N3WWN here for further testing. Next, I want try to test moving a |
I'm not sure if I fully understand the test done here, I went the other way: I created a new project, added a few media files, saved and closed. I then moved the media files to a new location without moving the project folder. It asks during open for the new location, and as long as the other media files are in the same place, it automatically relinks them (this is in both my pull requests). I think in your test with moving the project file, not much happens, as filenames are stored as absolute paths? So when you open the moved project, it still finds the original media files, and this is independent of my changes and should work in the previous version as well. It seems that thumbnails are stored relative to the project file and are not found in this scenario? I can't comment on the internals for this, these are just guesses based on my observations. Maybe test moving the media files as I described to see if that works for you. |
Oops. Sorry. My bad. Test 2:It works really well. Just point to a directory and it does all the re-linking by itself. Great! I did try to push it a little too much and did find one thing that would be great if it was fixed.
Sorry, I just realized that what I was thinking was pretty stupid. However, I am keeping that in here, in case it might give someone any new ideas. For instance, I am starting to think (and I might be being stupid again) that it'd be even easier if we could somehow let a user specify more than one directory at once. But I have no idea how that'll be implemented, esp. with the GUI. sigh Anyways, I think this PR's great! Thank you @fschill. You're awesome. 👍 💯 P. S: I have a question. I moved the entire project directory - that contained all the footages used, the .osp file, the thumbnails and all - to another place and OpenShot (not this branch) was still able to open it up. But the thumbnails still wouldn't show. It's the same with this branch too. Even after saving the project once it's open. However, the thumbnail directory still shows thumbnails in them. I wonder why that is. |
Thanks for the testing. Regarding the issues around moving project files and thumbnails, the discussion thread on the issue already went into that a bit. I think it was suggested that it would be better to store everything as relative paths to the project file (at the very least for any files that are in the same directory or a subdirectory of the project). That way everything could be moved without issues. That's a larger issue though which will have a bigger impact (e.g. handling existing projects after the change, etc.). Regarding thumbnails, I suppose the easiest would be to trigger a re-creation of everything if there are changes? I don't know how to do that for now, so if anyone wants to put that in that would be great. In the bigger picture, moving a folder with everything in it (project file, thumbnails, assets, media, etc.) should always work, so I think if it doesn't it should be treated as a (mild) bug. As thumbnails and caches are temporary files that openshot creates itself somewhere, it should be referenced with relative paths, and if it's not found should be regenerated automatically. |
@peanutbutterandcrackers - That project has such random videos. I love it! 🤣 @fschill - I had a look and it seems to work really nice. Of course I did notice the thumbnail issue but that is probably a separate PR. |
Wow, it'll be really great to have this feature working. I'm excited to take a look at it, if/when I get a chance. Just some quick notes:
Unless something's changed that I'm not aware of, this is exactly backwards. Currently, when the project file is saved, everything is converted to RELATIVE paths, compared to the project file. When it's read in, they're converted back to absolute, which is why if you're only working with the in-memory strings you'll only ever see absolute paths — but they're made relative before being written to the project file. (Which is horrible, and why you can't ever move a project file anywhere else, except in the very rare case that all of the media files it uses are stored in a lower directory tree, are moved with it, and all path elements keep the same names. Anything else, the relative paths start with Worse still, the thumbnail paths are also stored relatively, even though they all live under the If paths were stored absolutely, then you could move the project file and it wouldn't affect anything at all. And if you moved the media files, you could simply recover their (absolute) paths to the new location. And the thumbnail files would never change their location, so it wouldn't affect those regardless. (And that's not even touching on the transition paths, which are completely off-topic for this PR. Those are converted to aliased paths like |
Same relative-paths issue. Even when thumbnails are managed correctly by |
@peanutbutterandcrackers - I don't think we need much more testers on this one. I'm pretty happy with it and how it works currently and you said so too. @fschill - Thanks for this. I REALLY like this feature and would be happy to have it in by default. One of us can work on the thumbnail re-generation in the meantime. 💯 👍 |
At least one. As I recently noted in an update to #1923: Particularly when AppImages are involved, the history stack presents a severe crash potential...
All of which means it's very easy to Undo or Redo a saved history action which restores an invalid file path into the current project data, which will cause an immediate traceback and hard crash. IMHO the history should never be written to the project file at all, and should never be persisted across sessions. Too much of its data potentially has no lifetime beyond the current session. (Not storing history would also decrease the size of project files by 50% or more, in many cases. I have some rough back-of-the-envelope numbers on that in #1923.) There was a point where I was thinking, "Maybe the backup project data should still store the history, though, for recovery purposes", but that was before I understood how severe the AppImage-internal paths issue was. When running from an AppImage, history isn't ever safe to recover, not even after a crash — the internal file paths will have expired the moment the crash occurred. |
path = os.path.join(starting_folder, file_name_with_ext) | ||
# try to find clip with previous starting folder: | ||
if starting_folder and os.path.exists(os.path.join(starting_folder, file_name_with_ext)): | ||
# Update clip path | ||
file["path"] = path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a logic error here — the line that set path
to a new value (previous line 920) was deleted, so setting file["path"] = path
here is a no-op, since on (old) line 911 path is initially set as path = file["path"]
.
This should read file["path"] = os.path.join(starting_folder, file_name_with_ext)
, which is the path that was just tested for existence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitted #2311 with a quick-fix change for that, but actually maybe I'll try out the new suggest-changes feature:
file["path"] = path | |
path = os.path.join(starting_folder, file_name_with_ext) | |
file["path"] = path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Completely unrelated to this code, but as it's currently implemented the new GitHub suggest changes feature is very poor, IMHO. It feels extremely half-assed, and doesn't even give you any context for the line of code you're modifying. #NeedsWork)
Cool, thanks for merging! Happy to contribute. ~~@ferdnyc I'm not sure that the line is redundant. In the first pass of the loop, it is indeed the same value, but as the loop repeats (e.g. if the user specifies different folders after being asked), it would be new values. I don't remember in detail, but I think the idea is that for the next file, it will first try to apply the last specified folder to see if that works, so it won't have to ask the user for each file. ~~ Edit: I think I misunderstood and spoke to fast. Looking at it again, I think I can see now what you mean, it does indeed look weird. I'd have to test it again to make sure it still works, but I assume you've done that. Sorry about that! :) |
No worries, this change is great! And already inspired my PR #2312, to fix the thumbnail-recovery issue.
In my testing, without the path fix (above, or my PR #2311), if I saved a project file to |
(addresses part 1 of issue "Media relinking in file properties #1829" )
This pull request contains some minor changes in project_data.py, to automatically try and relink missing media files after the user has specified a directory. Previous behaviour: a dialog would open multiple times for each missing file, giving the impression that OpenShot is stuck in an infinite loop. New behaviour: After the user picks a directory for the first missing file, the same directory is tried for all other missing files first. Only if that fails, a dialog opens to ask for a new directory.