8000 Add a Create Folder Button to FileDialog by Schweini07 · Pull Request #192 · texus/TGUI · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add a Create Folder Button to FileDialog #192

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 11, 2023
Merged

Add a Create Folder Button to FileDialog #192

merged 4 commits into from
Apr 11, 2023

Conversation

Schweini07
Copy link
Contributor
@Schweini07 Schweini07 commented Mar 28, 2023 •
edited
Loading

This is a draft pull request to add a button to the FileDialog class which creates a folder with a given name in the current directory.

The functionality is pretty much complete, my only concern right now is a big memory leak that is created after the ChildWindow which handles the naming of the new folder is created, and then the application is closed.
This leaks were fixed now, I had to, of course, remove all widgets from the ChildWindow, before destroying.
There is still a memory leak, that occurs on closing the ChildWindow with the x-button, but I'll have to look into that later.

I would also be happy to receive feedback on how good the code confirms to the tgui code style (or SFML's, according to the CONTRIBUTING.md 🙂).

I am also a bit conflicted on how the ChildWindow is created. I feel it's most comfortable to let this just happen in a function, and not add members of it to the FileDialog class.
I'm also thinking about maybe putting the creation of the ChildWindow in a seperate function, for readability's sake, like this:

void FileDialog::createFolderButtonPressed()
{
    createFolderDialog();
}

image

@texus
Copy link
Owner
texus commented Apr 1, 2023

Thanks for contributing this.
The code itself looks good. Functionality wise, there are a two things that I would still change:

  • The create folder button shouldn't always be visible. The default for the file dialog is to load an existing file, so I would make the button invisible by default and only show it once a new function (e.g. setAllowCreateFolder(true)) is called.

  • The positioning of the create folder button is a bit weird. Maybe you can change the origin to (0,1) and set its position to (10, "100% - 10")?

Some other minor comments:

  • Instead of calling createFolderDialog from createFolderButtonPressed for readability, I think it would be better to just rename the createFolderButtonPressed function to createFolderDialog.

  • It probably shouldn't be possible to still press buttons on the file dialog while the create folder dialog is open, so you could put a semi-transparent black panel behind the file dialog (one that fills the entire file dialog) to prevent this, and to draw more attention to the create folder dialog.

  • While the removeAllWidgets() call fixes the memory leak, it is normally not required to remove child widgets before destroying the container (as this happens automatically). The reason the memory leak happens without the manual removal is because you are capturing the window in the lambda. The button::onPress signal thus stores a copy of the shared_ptr to the window. The window won't be destroyed until all pointers to it are gone, and the button won't be destroyed until the window itself is destroyed (as the window still holds a pointer to the button since it was added to the window). Removing the button from the window manually (which you did with removeAllWidgets) is one solution, but what I usually do is let the lambda capture a raw pointer instead of a shared_ptr:

cancel_button->onPress([wnd = create_folder_window.get()] {
    wnd->destroy();
} );

@Schweini07
Copy link
Contributor Author

Thanks for the feedback!
I've now implemented all your suggestions. I made quite a lot of changes to make the button optional, hopefully it's fine how I've done it.

I still have to iron out two bugs I found, where the new folder is not correctly created. But after that the PR should be good to go.

@Schweini07 Schweini07 marked this pull request as ready for review April 6, 2023 19:22
@Schweini07
Copy link
Contributor Author

The PR is now ready for review.
Everything should be all right, the only thing tha maybe could be done, is to use the create folder button in some file dialogs of the gui builder. Especially for the create form file dialog, which is the whole reason I made this PR 😄.

I would maybe open up another PR after this one is closed, where I implement this, but maybe you want to do it yourself, which could be a bit faster than going through the whole PR process again.
I actually implemented this for the gui builder while working on the PR. I just added another parameter to this function: void showLoadFileWindow(const tgui::String& title, const tgui::String& loadButtonCaption, bool allowCreateFolder, bool fileMustExist, const tgui::String& defaultFilename, const std::function<void(const tgui::String&)>& onLoad)
which is bool alllowCreateFolder. The parameter then gets passed on to the file dialog: auto fileDialog = tgui::FileDialog::create(title, loadButtonCaption, allowCreateFolder); inside the function.

@texus
Copy link
Owner
texus commented Apr 10, 2023

Everything looks good about the code.
For the gui builder change, maybe you can add it as another commit to your "create-folder" branch and then I'll just merge it together with this PR.

It is currently possible to create folders into other directories than the one you are in, by using slashes and dots in the filename. This should probably not be allowed. This is however something that can be fixed later and it shouldn't block this PR from being merged.

The native file dialog in my linux doesn't allow me to confirm when the name is either empty, ., .. or contains a /.
image

So eventually a function like this will need to exist somewhere, e.g. called in confirm_button->onPress to prevent the confirm button from doing anything if the folder name is invalid.

bool isValidFolderName(const tgui::String& name)
{
    if (name.empty() || (name == U".") || (name == U".."))
        return false;

#ifdef TGUI_SYSTEM_WINDOWS
    if (name.find_first_of(U"\\/:*?\"<>|") != tgui::String::npos)
#else
    if (name.contains(U'/'))
#endif
        return false;

    return true;
}

@Schweini07
Copy link
Contributor Author
Schweini07 commented Apr 10, 2023 •
edited
Loading

I implemented the Gui Builder adjustments now, so everything should be good to go.

Regarding your suggestion, I would probably create another PR, right after this one gets merged, where I implement this check. Would it be fine, if I'd take the code snippet from your comment as the check function?
And, should we display a text like in your file manager (I assume it's Nautilus) and disable the button when a name is invalid, or just solely disable the button as it is done for example in Thunar:
image

Although I just noticed, that Thunar has no check for folders named e.g. .., and it will ask if it should replace the parent directory 👀. So I'd trust Nautilus on this matter 😅.

@texus
Copy link
Owner
texus commented Apr 10, 2023

If you are going to add it immediately then we might as well just add it as another commit to this PR instead of starting a new one.

Would it be fine, if I'd take the code snippet from your comment as the check function?

Of course, that's mainly why I posted it.

And, should we display a text like in your file manager (I assume it's Nautilus) and disable the button when a name is invalid, or just solely disable the button as it is done for example in Thunar

I would just disable the button like in Thunar.
Adding the text not only makes it more complex (you wouldn't be able to use a simple boolean function as you need to know the reason to show in the error), but it would make customization (e.g. changing the language) more work too as the string needs to be changeable for every possible error case.

The file manager from my screenshot is Nautilus-based indeed, it's Nemo to be precise.

@Schweini07 Schweini07 force-pushed the create-folder branch 2 times, most recently from 31fb7a7 to e26e1f0 Compare April 10, 2023 19:45
@Schweini07
Copy link
Contributor Author

So, I finally got it done, after fighting a merge conflict for a while, but it works quite well.
I stumbled upon an unnecessary use of the tgui namespace from my myself (again), so I removed that in that commit too.
Hopefully everything's fine and ready to go now.

Regarding Nemo, I actually did not know about its existence until now, though I used Nautilus 'till a few months ago until they changed the UI so drastically that I switched over to Thunar. But I think, I am gonna give Nemo a try, it looks promising!

@texus texus merged commit 240d5f5 into texus:0.10 Apr 11, 2023
texus added a commit that referenced this pull request Apr 11, 2023
@texus
Minor code cleanup after PR #192
701ce83
@texus
Copy link
Owner
texus commented Apr 11, 2023

Thanks again, it seems to work great.
I did still make a few minor changes when merging this:

  • You removed one unnecessary "tgui::", but you added another in isValidFolderName when you copy-pasted my snippet 😄
  • The TGUI_INTERNAL widget names all have a fixed format. The name doesn't matter much, but for consistency it should be the the same everywhere and you used % instead of $ and without a # at the end.
  • While I didn't test it, I'm pretty certain you introduced a memory leak with folder_name_edit_box->onTextChange and confirm_button->onPress. The edit box callback refers to the edit box itself, while the edit box and button callbacks also referred to the other one. With this kind of issue being so easy to make, I'm really starting to wonder if using shared_ptr for widgets was such a good decision.
  • The createCreateFolderDialog function contained snake_case instead of camelCase for variables. If I didn't have to change the code for the memory leak then I would have completely missed this detail.

@Schweini07 Schweini07 deleted the create-folder branch August 9, 2023 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Schweini07 @texus
0