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

Windows11.When changing the video cover, the path is incorrect. #109

Closed
TsaiRongFu opened this issue Aug 9, 2023 · 9 comments
Closed

Windows11.When changing the video cover, the path is incorrect. #109

TsaiRongFu opened this issue Aug 9, 2023 · 9 comments

Comments

@TsaiRongFu
Copy link

First of all, I want to express my sincere gratitude to the creator for developing such a convenient tool!

As the title suggests, when I try to change the video cover, there is an issue with the path. When the 'cover' parameter is set with an absolute path that includes another disk drive number, it results in a failure to successfully retrieve the updated image. The following image serves as an example:

I have tried enclosing the 'cover' parameter with both double and single quotation marks, but the error remains the same:

tageditor-cli.exe set cover="E:\test.jpg" --max-padding 429496729 -f "E:\test.mp4"
tageditor-cli.exe set cover='E:\test.jpg' --max-padding 429496729 -f "E:\test.mp4"

Error picture


The solution is to navigate to the location of the image file using the cd command and then specify it using a relative path. Here's an example:

tageditor-cli.exe set cover='.\test.jpg' --max-padding 429496729 -f "E:\test.mp4"

Success picture


One intriguing aspect is that the "-f" option allows specifying the file using an absolute path, while the "cover" parameter cannot be specified using an absolute path. I'm unsure whether this is a bug, but I am interested in understanding the underlying cause. If this issue could be resolved, it would greatly assist me in efficiently processing a large number of files in batch.

@Martchus
Copy link
Owner

Martchus commented Aug 9, 2023

I have never tested this feature under Windows. I suspect that the use of : in absolute paths on Windows collides with the syntax I implemented for the cover value to separate the path from the cover type and description (which also uses : as separator, see the example in the README). Since I almost always use MSYS2 under Windows I really haven't thought of that. Unfortunately I cannot change the separator to something else now because it would break backwards compatibility. Maybe I can allow escaping it but it would likely be a bit cumbersome to use.

So maybe I better add some additional logic so that a single letter plus : is treated as one peace. This has the disadvantage that if someone wants to refer to a single-letter file in the current directory and also specify a cover type one has to use e.g. cover=.\e:back-cover instead of just cover=e:back-cover but I guess we can live with that.

Until then: Besides the workaround you've already mentioned you can also use the MSYS2 terminal. I can recommend that environment and its packaging anyways.

Martchus added a commit that referenced this issue Aug 9, 2023
Interpret a single letter plus colon as drive specification followed by the
actual path instead of splitting it and considering the path the cover
type.

See #109
@Martchus
Copy link
Owner

Martchus commented Aug 9, 2023

Implemented on master. Do you want a development build for Windows to test it out? So far I've only tested it under GNU/Linux (by creating a file called e.g. e:foo in the current working directory).

@TsaiRongFu
Copy link
Author

Sure! Thank you for your prompt fix. I'd appreciate it if you could provide a Windows development build for testing.

@TsaiRongFu
Copy link
Author

I have never tested this feature under Windows. I suspect that the use of : in absolute paths on Windows collides with the syntax I implemented for the cover value to separate the path from the cover type and description (which also uses : as separator, see the example in the README). Since I almost always use MSYS2 under Windows I really haven't thought of that. Unfortunately I cannot change the separator to something else now because it would break backwards compatibility. Maybe I can allow escaping it but it would likely be a bit cumbersome to use.

So maybe I better add some additional logic so that a single letter plus : is treated as one peace. This has the disadvantage that if someone wants to refer to a single-letter file in the current directory and also specify a cover type one has to use e.g. cover=.\e:back-cover instead of just cover=e:back-cover but I guess we can live with that.

Until then: Besides the workaround you've already mentioned you can also use the MSYS2 terminal. I can recommend that environment and its packaging anyways.

I would like to ask why the parameters for paths, cover types, and descriptions are combined into one, instead of being set as separate parameters like --cover-back, --cover, --cover-front. Perhaps this could be the reason you mentioned, "it could potentially break backward compatibility." I'd like to understand the rationale behind this approach.

I apologize for asking so many questions without actually contributing to the code modifications. However, due to my limited experience with C++ development, I might need some time in the future to try and assist with maintenance.

@Martchus
Copy link
Owner

Martchus commented Aug 10, 2023

I'd appreciate it if you could provide a Windows development build for testing.

Ok, I've just started a build.

I would like to ask why the parameters for paths, cover types, and descriptions are combined into one, instead of being set as separate parameters like --cover-back, --cover, --cover-front.

Because the arguments for all fields are just following the fieldname=value. They are actually just values to the --values argument and not own arguments. When I started hacking this seemed good enough. I haven't though that it would get more complicated. Then I added support for cover types and cover descriptions and kept the approach because having an extra argument just for the cover (and not for other fields) would also be inconsistent. Note that internally (in the ID3v2 tag) the cover type and description are also just part of the field value and not a separate field so this is also how the underlying tagparser library treats it and thus it also makes sense to treat it this way in the CLI.

Perhaps this could be the reason you mentioned, "it could potentially break backward compatibility.

I mean if I would now change it to use a different separator that would break compatibility because if someone has a script using : it would cease to work. Of course e58f8a1 is also a slight break but I doubt anybody will run into problems in practice.

@TsaiRongFu
Copy link
Author

Thank you very much for your explanation. This indeed seems to be the quickest and most effective solution for now.

I'm looking forward to the finalized build.

@Martchus
Copy link
Owner

Here's a development build: https://martchus.no-ip.biz/repo/arch/ownstuff-experimental/os/x86_64/mingw-w64-tageditor-qt6-3.9.0-1-any.pkg.tar.zst

You'll have to pick the static version from that archive.

@TsaiRongFu
Copy link
Author

After testing, I no longer have any issues. Thank you very much for your quick fix and frequent responses. I will close this issue for now!

fix down picture

@Martchus
Copy link
Owner

Also thanks for reporting. I'm currently in the process of improving the tag editor here and there (don't expect any big changes) anyways so this kind of input is useful.

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

No branches or pull requests

2 participants