-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Show format within filename when exporting file #4389
base: main
Are you sure you want to change the base?
Conversation
Hello @dacap, can you review this PR? I think it's a fast PR to review and it would be very useful for me. Sorry to be so insistent, but this bug was causing me some trouble in a project I'm developing, and it's a little hard for me to compile aseprite in that workspace (I use the steam version there). |
Hi @gui-marc, I've just tested with an empty new sprite (that is not associated with a file on disk) and I got: Anyway checking the code (and the problem to solve) I think we can approach this in a different way. In some way this is related to #4483, so we should find a way to display the output path, or the common path between the original sprite and the exported sprite (and add What I'm thinking is in something like this: Where little capsules are the {path}, {tag}, {frame} elements and we should be able to add these easily (writing "{something}" or with some kind of menu/right-click). Anyway as a first simple version that could be solved with this PR, we could just calculate the common path between the input sprite and the configured output path and remove it + adding all the path elements that goes to the output dir (that might include |
Strange, this seams to be a platform related problem. I couldn't reproduce it on Linux. But thanks for the suggestion! I'll implement the way you suggested and test the behavior on different environments and then I'll reach out to you again when it's finished. |
@dacap I don't understand what you mean by |
@gui-marc sorry for the confusion. For the moment you can ignore the screenshot with the capsules. What I said that a possible PR that could fix now is the possibility to show the relative output path from the input sprite path (if possible). Here some examples: Example A:
Example B:
Example C:
This means that the output file name could show a path relative to the path of the current file. Not sure if this makes sense or will be a lot easier to just show the full path. But it matches the current behavior when no path is specified (the output is done in the same directory of the current file). We then could save in preferences ( |
@dacap I've been looking through the code and didn't find any implemented method to, given two paths, return the relative path from one to another. Also, if we further want to implement the feature with the capsules #4483, the logic I've already implemented could be fully reused. Regarding the |
That's right, that's a function that this PR should implement in case we want to add this behavior.
I'll review your PR but it will not be accepted as it is because it needs more testing (and some code changes). E.g. Just create a new sprite and try to Ctrl+Shift+Alt+S you will get the assert I show you or this behavior on Linux: ☝️ that's just creating a new sprite and trying to exporting it. Other example: If we specify I'm not saying that the current version works better, but I'd prefer to merge a PR that fully fix this whole output path/file issue. |
Ah ok, I've already fixed this issue in a local branch, but I didn't understand that it was related to the print you shown.
Yes, it is not required if we just want to show the relative path.
Ok, i didn't get that this was also a problem related to this issue, but now I understand the problem. @dacap thanks for the patience explaining this to me. I guess the better way is to implement your proposal here with the relative path and then I could send another PR or comment the code snippet in #4483 with the method I've already implemented here with the fixes and code changes, so that work is not lost. I'll work on that. Thanks for the review! |
clang-tidy review says "All clean, LGTM! 👍" |
Hi @gui-marc, although using As a side note this PR could be simplified with just one commit (removing all initial unnecessary changes now) leaving the ones in |
This PR is not closed, I had to remove my commits due to a bug (i had to clone the project again on my pc so I could use the newer versions of the subrepositories). Then I removed all my commits of the branch and github closed the PR automatically... |
Yes @dacap you are right, I've just checked and macOS 10.9 really doesn't support |
That will be great @gui-marc, one possibility would be to add a new function in the laf base/fs.h for this (e.g. base::get_relative_path). The good part about this is that you can add some tests in base/fs_tests.cpp to check if the function works as expected before using it on Aseprite. |
- When exporting a file that was already saved, shows the relative path to the original path instead of only the filename. Resolves aseprite#4059
Bug Description
The
export_file_window
was displaying only the filename instead of the entire path, leading to confusion for users, especially when including dynamic placeholders such as {tag} in the file path.Cause of the Bug
Within the
ExportFileWindow
class, the methodsetOutputFilename
split the provided path intom_outputPath
andm_outputFilename
, causing the window to display only the filename. Consequently, dynamic placeholders like {tag} were omitted from the displayed path, leading to ambiguity for users.Impact
This behavior caused confusion and made it difficult for users to accurately discern the file's destination, particularly when using dynamic placeholders. It also difficulted clearing dynamic placeholders from the export path, as it is not shown in the window.
Solution
To address this issue, the following changes were implemented:
path_formats
defined infilename_formatter
, modifying its type fromvector
toarray
for improved performance.m_outputPath
andm_outputFilename
, dynamic placeholders such as {tag} are now considered part ofm_outputFilename
and excluded fromm_outputPath
.Example
With the new behavior, let's consider the export path:
/home/user/{tag}/test/{frame}/file.png
.m_outputPath
will be/home/user/
m_outputFilename
will be{tag}/test/{frame}/file.png
Now, the displayed path will include the entire dynamic placeholder structure, providing users with clear visibility of the file's destination and enabling the editing of these parameters.
Future Considerations
Maintainers should be aware of potential implications when modifying path-related functionalities in the future. Additionally, it's essential to maintain consistency in handling dynamic placeholders across the application to avoid similar issues.
Resolves #4059
I acknowledge that my contributions are licensed under the Individual Contributor License Agreement V4.0 ("CLA"), as stated in CLA.md.
I have signed the CLA following the steps provided here.