Change default animation export path to same directory as project fil…

[donarturo11]

Change default animation export path to same directory as project file #1656

[Jose-Moreno]

@donarturo11 Hi, I hadn't seen this recently, been a bit busy. Thank you for your contribution. I'll add this to the respective priority projects and we'll have to wait for other developers to assign themselves to this review according to their availability 👍

[donarturo11]

If my contribution waits to merge so I'll else implement adding extension. I would like make my code perfect.

[MrStevns]

Hi donarturo11

First and foremost, thanks for your contribution! While I don't see the PR currently being in a state where we can simply merge it, I will try to help you get there.

What you're trying to do here is to use the project path for all other exports, this might be okay in some cases but do you always want your movie files placed in the same location as your gifs or your project? If you want them organized, that may not be the case.

Even if that was the case, instead of explicitly setting the filetype (affecting all dialogs...), you would get the same result by removing the fileType from setLastSavePath and getLastSavePath altogether, since the path is currently stored per type. That however may still not be the best solution since it lacks some flexibility, it's just the more appropriate one when looking at your code.

A better solution may be to use the project path when applicable (eg. you've saved the project) as default path but afterwards rely on the filetype as we do now.

Something like this could do it.

FileDialog::getSaveFileName(....)

// existing code
.....
if (fileType == FileType::ANIMATION) {
        // When we save a new project, change default path for all other filetypes
        QList<FileType> fileTypes = { FileType::IMAGE, FileType::IMAGE_SEQUENCE, FileType::GIF, FileType::MOVIE, FileType::SOUND, FileType::PALETTE };
        for (FileType& fileType : fileTypes) {
            auto projectPath = QFileInfo(filePath).absolutePath();
            setLastSavePath(fileType, projectPath + "/" + defaultFileName(fileType));
        }
    }
....
// existing code
if (filePath.isEmpty()) { return QString(); }

Some explanation to the above implementation:
When a filetype of type ANIMATION is found (we saved the project for the first time), we set the same path for all other filetypes, thus whenever a new project is saved or its path changes to a new location, the same will follow suit for the other types. If you then change the location for another filetype, it will work as it does today.

The last bit is to remember to change the extension (and possibly name) too, therefore we get the absolute path of the animation we're about to save, use that as a base and apply default naming to the rest.

Although I haven't tested the code thoroughly, I think this is a better way to go about implementing what you want, without removing some of the benefits we get from the current functionality.

[donarturo11]

Sorry. These contributions are first in my life, so sometimes I'm making some mistakes.
I was asked to update this pull request instead create new. I don't know if I made correctly.

[donarturo11]

What do you think about replace function "QString FileDialog::addDefaultExtensionSuffix(const FileType fileType)" from private into public (filedialog.h). If possible, switch case loop inside "void ImportExportDialog::init() " will not necessary, because possible will be using only one line like this QString ext = FileDialog::addDefaultExtensionSuffix(mFileType); . Sure I know, that this code won't identical, but I'll simplify this code. I know that is no sense input the same code, which I submitted in switch-case, so the same functionality is already implemented in class FileDialog - only make one function public.

[scribblemaniac]

Hello there and welcome to open source! Don't worry about making mistakes, it's how we learn. Different projects will have different preferences when it comes to how you contribute as well. Just a warning that things move very slowly in this project because of our small, busy team, so do not be discouraged if you do not hear back from anyone right away.

I think that addDefaultExtensionSuffix could definitely be used for that section in ImportExportDialog::init that you added here. Rather than make it a public method, it might be better to move it out of FileDialog. We do already access some static methods of that class in ImportExportDialog, but that's not really good design. I would probably make a more general sounding function in fileformat.cpp as getDefaultExtension(filetype), but maybe someone else can weigh in on that idea as well.

My thoughts from looking (but not testing) at your current implementation are in agreement with what @candyface has already suggested. I'm not against making the default path for opening/saving things be the director where the current project is saved (when applicable). However, if the user opens/saves something to a different location, that should be the default location next time they go to open/save a file of the same type. It's something Pencil2D currently does and I think would be a downgrade to remove. Passing a fixed FileType as arguments to functions defeats the purpose of having that argument there in the first place.

I would recommend taking a second look at the code example suggested. The only thing I would change is this:

for (FileType& fileType : fileTypes) {
    auto projectPath = QFileInfo(filePath).absolutePath();
    setLastSavePath(fileType, projectPath + "/" + defaultFileName(fileType));
}

To this:

QDir projectPath = QFileInfo(filePath).absoluteDir();
for (FileType& fileType : fileTypes) {
    setLastSavePath(fileType, projectPath.absoluteFilePath(defaultFileName(fileType)));
}

The difference being that QFileInfo is only constructed once and it doesn't use a hardcoded path separator. Qt will handle "/" just fine on all systems, but I my opinion using the built-in methods helps avoid potential confusion on platforms that use other separators.

[MrStevns]

I've pushed the discussed changes to the @donarturo11 branch, so the PR is ready now. I removed the extension logic again because we already have it so not sure what the point was with adding it to the import/export dialog specifically, it should already happen regardless.

[MrStevns]

Current status: awaits review by @scribblemaniac

[scribblemaniac]

I have reviewed this and codewise everything is looking good. The concern I have is with the functionality. One thing I think would be useful is having the file name default to the same as the project, not just the file path, so I have implemented this. In addition to this there are a couple other things I think should be done but I have not done:

• When a user exports before saving for the first time, the export path should not be overwritten when subsequently saving. The user has already specified where they want the file to export to, and we should not modify that.
• When opening a project, the default export path should match that project, not the last newly saved project.

This is just how I think it makes the most sense, but I welcome any other ideas.

[donarturo11]

I reviewed @scribblemaniac's commits. I think, that this solution about default export path is the best. In my humble opinion, this commit could be merged.
Refactoring of FileDialog::dafaultFileName makes this function more flexible, so I think that merging @scribblemaniac's solution would be the best idea.

[donarturo11]

These changes are more precise than mine.

[J5lx]

I’ve implemented scribblemaniacs second suggestion. The first one involves more complexity than it is worth IMO. Users should save their projects early anyway. With that I believe this PR should be good to go.