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

Improve interface for changing local Config Path #4

Closed
svetogam opened this issue Sep 17, 2024 · 10 comments
Closed

Improve interface for changing local Config Path #4

svetogam opened this issue Sep 17, 2024 · 10 comments

Comments

@svetogam
Copy link
Contributor

The intended behavior is evidently for editing the text at Project Settings > Addons > Project Builder > Config Path to automatically move the file to the new location, but this is buggy and has lots of problems.

1
When the file is moved or renamed, it's not updated in the filesystem immediately, and accessing it gives an error that it could not be loaded. Making Godot lose and regain focus updates it.

2
When trying to move to a new folder that doesn't exist, the file is not moved and the new folder is not created. But the Config Path field shows the invalid path, and Project Builder then can't find any routines.

3
Pressing the revert icon next to the config path sets it to . It also removes Project Builder and this setting from the Project Settings so it can't be set back. Reloading the plugin from the Project Settings fixes this and sets it back to the default res://project_builds_config.txt.

4
Entering a path to an existing file overwrites that file. This is a little dangerous. Though it's perhaps unlikely that it would hurt anyone, users won't expect this automatic overwriting behavior, so their guard would be down. Fortunately, Godot doesn't allow dragging files to this field to overwrite them (at least for me). But how much can this behavior be counted on?

Pressing the file icon to select a file instead of writing the filepath gives a popup to browse the filesystem and select files. When a file is selected through here it gives the popup File "..." already exists. Do you want to overwrite it? It's good that it tells users exactly what would happen, but when would it be desired to overwrite a file here? This is basically file-shredding functionality. Though it is possible to navigate to the existing folder you want, type in your filename, and save.

5
Also, the same issue as #3 is present, aborting moving between partitions on some systems. (I personally don't mind this at all.)


There might be a way to make this functionality work, but I don't think that's a good plan. Most users aren't going to expect or try to use automatic renaming, and it opens safety concerns, so my suggestion is to drop that feature. I think it would be better to copy the functionality such as in Application > Config > Icon and Application > Run > Main Scene. These update the field in the Project Settings when the set file is moved to a new folder. But even if the user has to reselect the file manually through the Project Settings, I think it would be an improvement.

That would fix problems 1, 2, 4, and 5. My guess is the remaining problem 3 would be easy to fix by setting a hint somewhere to make the default filepath be the same as the one that is automatically generated when Project Builder first opens a project.

Another option could be to track this config path through the Project Builder's Config > Local Config settings instead of the Project Settings through the plugin. The plugin could then be kept as a limited, optional feature for adding an interface into the Godot Editor. And this feature could be valuable in the Project Builder apart from the plugin. But if I'm not mistaken that would be settings in the file for the file to find itself, so it might not be feasible. Perhaps something in global settings could map projects to their local config files? It's hard for me to tell how viable or good this idea would be. One issue is that this could make sharing files more complicated.

Another option would be to require the local config file to be at the root of the project and remove the feature of setting a path to it completely. I think this would be my preference, since I don't have any use for this feature and it would simplify things. But maybe someone else would want this feature.

@KoBeWi
Copy link
Owner

KoBeWi commented Sep 18, 2024

Reworked in 53a8d22
The project setting is now hidden and instead configurable from Local Config. It has explicit Apply button and does not allow overwriting files.

The setting will be visible if you don't have plugin installed (until godotengine/godot#97145 at least).
Also there is some new Windows bug that makes it impossible to change config path in 4.4: godotengine/godot#97147

@svetogam
Copy link
Contributor Author

Tested and I approve this new design. I like the implementation idea of storing the setting as a hidden variable in project.godot.

I once managed to enter a state in which I moved the local config path somewhere else while Project Builder didn't find it and created a new empty local config instead. But this wasn't dangerous and it was easy to fix when it happened. I think this was due to first updating the local config path from Project Builder and then updating the plugin. So I think this is a one-time upgrade inconvenience and not a problem worth fixing.

I noted some minor usability improvements. I don't consider any of them essential.

  1. The file dialog could look for and save as .txt instead of "All Files".
  2. The file dialog could set a default filename that is the current filename. This would make it easy to go to the desired directory and click Save without having to think about what to call the file. It already sets the default filename this way after using the file dialog to move the file once.
  3. Selecting the current config file in the file dialog gives the confirmation popup "File ... already exists. Do you want to overwrite it?" Everything works fine, but the confirmation popup doesn't reflect what is actually happening. It would be better if there was no confirmation popup here, since there's already safety against overwriting.
  4. It seems like it doesn't allow you to save the local config file outside of the Godot project folder. I don't know if this is intended, but I think it's a good feature. The problem is the result of trying to do this is it shows an absolute filepath and then gives a generic error. It would be better if it gave an error saying immediately that moving the file outside of the Godot project folder is not allowed.

@svetogam
Copy link
Contributor Author

I managed to again enter the state in which Project Builder can't find the local config file and so creates a new empty one instead. This happens whenever you move the local config file without using Project Builder's interface, so it's not just a one-time upgrade inconvenience.

In order to exit this state, you must exit Project Builder, delete the empty config file, then move and rename the non-empty config file to the place of where the empty config file was. If you do this without exiting Project Builder first, then it will overwrite the non-empty config file with the empty config file. (This is the worst part!)

My guess is it would be a minority (though a substantial one) of users who would try to move local config file outside of the Project Builder interface first. Apart from that, something like the same problem will be encountered by anyone moving a local config file from one project to another, which includes users sharing their local config files.

However, I doubt there is any way to substantially improve this without taking some noticeable usability hit somewhere else, given the nature of the problem. So I still consider this issue solved.

Thinking something like this might happen is why it was my preference to remove the feature completely. Standardizing the filename and location would make sharing and reusing files simpler. Is there a similarly valuable use-case for moving the local config file?

@KoBeWi
Copy link
Owner

KoBeWi commented Sep 19, 2024

The file dialog could look for and save as .txt instead of "All Files".

The config file can use any extension, but maybe providing default one is better, idk.

It would be better if there was no confirmation popup here, since there's already safety against overwriting.

That's a limitation of FileDialog. You can only select new files in Save mode, but it comes with the warning that can't be disabled.

Is there a similarly valuable use-case for moving the local config file?

It's valuable for project organization. I have all editor-related metadata stored in a single directory (except export_presets.cfg, which can't be moved yet).

@svetogam
Copy link
Contributor Author

It's valuable for project organization. I have all editor-related metadata stored in a single directory (except export_presets.cfg, which can't be moved yet).

Okay, that's a fine use-case. I just wanted to make sure there was some definite use for this feature.

It occurs to me that supporting your use-case could be done by standardizing the filename while only letting users select the folder to find it in. The logic would go:

If selected folder does not contain a local config file:
    Move current local config file there
    
Else if selected folder does contain a local config file:
    If current local config file is empty:
        Delete current local config file
        
    Else if current local config file is not empty:
        Do not delete current local config file
        
    Either way, begin to use local config file in selected folder

Benefits

  • Simpler interface. No Apply button or confirmations.
  • Easier maintainance (I think). No overwrite protections or hacks using the file dialog in an unintended way.
  • Users can move local config files outside the Project Builder interface without apparent bugs.
  • Users can reuse and share local config files with less hassle.

Downsides

  • No ability to change the filename.
  • (IDK, maybe there's some other problems with this I can't think of.)

So then I think the question is: Is there a valuable use-case for changing the filename? If there is none, then I think this could be a substantially better interface.

Though again, the way the interface is now is good enough if you don't want to change it anymore.

@KoBeWi
Copy link
Owner

KoBeWi commented Sep 20, 2024

2d2464b removes Apply button and makes the path non-editable. You can change it by picking a new folder, the file name is hard-coded.

@svetogam
Copy link
Contributor Author

I think it's an improvement, but it doesn't implement the full functionality I suggested. So it's missing these 2 benefits:

  • Users can move local config files outside the Project Builder interface without apparent bugs.
  • Users can reuse and share local config files with less hassle.

Maybe the checklist I made from testing will be helpful:

  • Creates and finds local config when past location is in root folder of project
  • Creates and finds local config when past location is in subfolder
  • Move to new folder using Project Builder interface
  • Move to same folder (do nothing) using Project Builder interface
  • Move without Project Builder interface and select new folder using Project Builder interface
  • Cannot move outside project folder
  • Select a different local config in a different folder
    • Delete old local config if it's empty
    • Do not delete old local config if it's not empty
  • Can delete local config in current location and add new local config over it to switch without Project Builder interface
  • Does not overwrite a local config when trying to select it

@KoBeWi
Copy link
Owner

KoBeWi commented Oct 15, 2024

Check after c587eb0

@svetogam
Copy link
Contributor Author

Looks good! Everything on the checklist now works.

Just 2 notes on slightly weird behavior:

  • Selecting the same local config with no change gives a message. I would expect it to do nothing like canceling.
  • Selecting either the same or a different local config when it was changed saves the changes since the last time the routine editor was exited and drops changes after it. This behavior is just hard to predict. You can't rely on either reloading the same file to revert changes, or to save the file when selecting a new one. But no one should be relying on these things anyway.

I'd say these don't matter and that this issue is complete.

@KoBeWi
Copy link
Owner

KoBeWi commented Oct 20, 2024

42d8765

@KoBeWi KoBeWi closed this as completed Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants