-
-
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
- Exposed path_formats defined in filename_formatter and changed type from vector to array increasing performance. - While setting the output filename in the export file window, considered path formats (e.g. {tag}, {frame}) to also be included in the filename. Resolves aseprite#4059
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. |
@@ -87,6 +88,9 @@ namespace app { | |||
bool get_frame_info_from_filename_format( | |||
const std::string& format, int* startFrom, int* width); | |||
|
|||
// Types of formats that can be used in the filename formatter. E.g. "{frame}" | |||
extern std::array<const char*, 12> path_formats; |
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.
We don't (try to not) define global variables. This could be something like get_templates_list()
in case is required (I think this is not required for a proper fix).
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 showed.
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! |
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.