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

Show format within filename when exporting file #4389

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gui-marc
Copy link

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 method setOutputFilename split the provided path into m_outputPath and m_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:

  • Exposed path_formats defined in filename_formatter, modifying its type from vector to array for improved performance.
  • During the assignment of m_outputPath and m_outputFilename, dynamic placeholders such as {tag} are now considered part of m_outputFilename and excluded from m_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.

- 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
@gui-marc gui-marc requested a review from dacap as a code owner March 28, 2024 12:33
@gui-marc
Copy link
Author

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).

@dacap dacap self-assigned this May 22, 2024
@dacap
Copy link
Member

dacap commented May 22, 2024

Hi @gui-marc, I've just tested with an empty new sprite (that is not associated with a file on disk) and I got:

image

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 {path}+path elements to get to the output directory).

What I'm thinking is in something like this:

image

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 ../ elements if we are exporting in a parent dir, etc.). This doesn't require any of the changes in the file formatter, just calculating the correct output path/file comparing the selected output full path with the full input path.

@gui-marc
Copy link
Author

Hi @gui-marc, I've just tested with an empty new sprite (that is not associated with a file on disk) and I got:

image

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.

@gui-marc
Copy link
Author

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 ../ elements if we are exporting in a parent dir, etc.). This doesn't require any of the changes in the file formatter, just calculating the correct output path/file comparing the selected output full path with the full input path.

@dacap I don't understand what you mean by input sprite path and configured output path. Is it the path saved in the Document filename and the path set in the export file window? Also, I don't understand how to differenciate the path elements that goes to the output dir from the path dir itself without doing what I just did by finding the first path_format in the path string. My original PR just show the same output you suggested in the screenshot but only with text, not using these capsules.

image

@dacap
Copy link
Member

dacap commented May 24, 2024

@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:

  1. The current sprite we are working now is saved in C:\User\David\A.aseprite
  2. We select File > Export > Export As and choose C:\User\David\Output\A.png
  3. The file name field in the Export dialog could show Output\A.png

Example B:

  1. The current sprite is saved in C:\User\David\Assets\B.aseprite
  2. We select File > Export > Export As and choose C:\User\David\Output\B.png
  3. The file name field in the Export dialog could show ..\Output\B.png

Example C:

  1. The current sprite is saved in C:\User\David\Assets\B.aseprite
  2. We select File > Export > Export As and read C:\User\David\Assets\{tag}\{frame}\B.png from the preferences
  3. The file name field in the Export dialog could show {tag}\{frame}\B.png (the common path part is removed)

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 ([save_copy] filename) this relative path (instead of full output path).

@gui-marc
Copy link
Author

gui-marc commented May 24, 2024

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).

@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 file_formatter, I've just exposed the available path formats list, since it is not defined anywhere in the app, therefore I didn't bring any changes in the class behavior.

@dacap
Copy link
Member

dacap commented May 27, 2024

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.

That's right, that's a function that this PR should implement in case we want to add this behavior.

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 file_formatter, I've just exposed the available path formats list, since it is not defined anywhere in the app, therefore I didn't bring any changes in the class 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:

image

☝️ that's just creating a new sprite and trying to exporting it.

Other example: If we specify output/{tag}/test.png then the dialog will show {tag}/test.png without the output the next time we use it (because in this patch we try to think that only template delimiters like {tag} can form the extra part of the output).

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;
Copy link
Member

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).

@gui-marc
Copy link
Author

gui-marc commented May 27, 2024

☝️ that's just creating a new sprite and trying to exporting it.

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.

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).

Yes, it is not required if we just want to show the relative path.

Other example: If we specify output/{tag}/test.png then the dialog will show {tag}/test.png without the output the next time we use it (because in this patch we try to think that only template delimiters like {tag} can form the extra part of the output).

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.

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!

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

Successfully merging this pull request may close these issues.

{tag} as folder name in "Export As" creating nested folders after first Export
2 participants