Allow editing GDExtensions from inside project settings#115846
Allow editing GDExtensions from inside project settings#115846aaronfranke wants to merge 1 commit intogodotengine:masterfrom
Conversation
KoBeWi
left a comment
There was a problem hiding this comment.
I wonder if the dialog could be made less barebones. It only displays path and edit button for each extension. Although I suppose that's a problem with extensions, the default configuration does not even include name. Maybe using base folder name would be better at least? E.g.
There could be a column called "Name" and in this case it would be YAML. Extracting the name would make it more readable at least. Also maybe a column that displays whether extension is reloadable and whether its version is compatible with the current one.
I know this dialog was already reviewed and approved as part of #90979, but upon re-review I just noticed some problems 🙃
02fe6d4 to
29420d4
Compare
Actually, I think adding a Of course, any GDExtensions that exist now won't have one, though, so we still need a sane fallback if there is no name |
|
I think adding a name should be done in a follow-up PR. |
29420d4 to
d431933
Compare
|
Maybe I'm too much of a programmer in this, but I would prefer a highlighted text editor to edit The main reason is that there is information in the file that we cannot reasonably represent in an editor UI. For example, currently, you can edit the entry symbol etc., but you cannot edit the linked binaries. If you are making a file from scratch, you cannot use this editor. If you aren't making one from scratch, what use is there to edit it? In that case, you are likely opening a Listing GDExtensions in a tab makes sense to me. I currently provide an empty "plugin" file for my GDExtension because people have complained that my GDExtension didn't load because they couldn't see it in the "plugin" tab. I'm sure my users aren't the only ones, so listing loaded GDExtensions in a tab should help. Another question, from your screenshot it looks like you'd be editing |
|
I think we need to better clarify who this is for and what this is for. I think that if you already know how to edit everything directly from the text ffile this is not for you. I feel like this is better perceived as a part of the functionality to create everything from godot's editor. As such, this is more addressed to people who don't have the knowledge. I agree there should be a way to modify the binaries but in a less destructive way then just editing the names and people then complaining that it is not working anymore because they didn't understand that it is not enough and they should also modify the scons file. For example i could see the binaries being linked to the new name property cited before modifying it would also modify in the scons script the libname variable if it exist and modify in the .gdextensions the binaries. If we really want to have binaries being editable directly in the ui it should at least be hidden in a advanced section hidden by default similar to what we have for signal connection or editor settings |
d431933 to
e85ce02
Compare
|
@Ivorforce Users who would prefer to edit this file as text can just edit the file in their text editor of choice. That doesn't mean we shouldn't have this dialog. As for "you cannot edit the linked binaries", note that the dialogue was more capable in the past, but I was asked to remove the functionality.
The original context was that you can make a GDExtension from scratch. But aside from that, I think editing is useful. Or, at a minimum, opening the editor for the purposes of viewing what is there is itself useful.
That's a good idea. I pushed an update to the PR so that it will @ajreckof Yes, and that's precisely the context in which this was created, as part of a way to view a GDExtension that was just created in the editor UI, and the creation process absolutely should be given a UI. It is honestly getting tiring to go back and forth on this. The scope is either too wide or too narrow for each reviewer that looks at it. This editor was made with the context of creating GDExtensions, then I was asked to separate out the editing from the creation, and now I am being told that this separation results in a useless editor. I completely disagree with all such reviews, I think either of these PRs are useful on their own and I just think these should be merged. |
|
Personally, I do agree that this is a useful addition, and that it stands on its own without #90979. My main gripe is that I think it would work better as a text editor. This would allow you to edit the file entirely, without loss of data or information.
What does this do? Open it in the default text editor? The 'reload' functionality could probably be put in the |
I concur. It looks like there are different expectations on what the dialog should do, but after adding too much features at once will come a suggestion to split the PR into smaller parts and the cycle continues xd
You can already text edit a .gdextension file from FileSystem dock. |
I cannot; when I double click the
Then maybe we should merge the dialogue / tab without the file editor? In context this sounds most uncontroversial. I'd be hesitant to add a file editor I don't think is very useful, especially because I will likely continue advocating using a rich text editor instead of a lossy UI editor in the future. The settings tab can already serve some interesting functions, such as showing whether the |
Right-click and Open in External Editor. |
|
If the consensus is that editing via dialog is not a good idea (no strong opinion myself), then it would make sense to display in the UI, and provide explanatory tooltips, to properties that carry meaningful information to people using a GDExtension, not those developing one. The entry symbol and reloadable toggle aren't such, the compatibility versions are. So to me something like this makes sense:
|
|
I think this needs the approval of a @godotengine/Gdextension member to merge. |
dsnopek
left a comment
There was a problem hiding this comment.
Thanks for getting this started! We really need a UI in Project Settings that lists GDExtensions for some other much needed features too
However, this needs some work with regard to handling the "reloadable" flag - details below
| config->save(gdext_res_path); | ||
| _clear_fields(); | ||
| GDExtensionManager *gdext_man = GDExtensionManager::get_singleton(); | ||
| gdext_man->get_extension(gdext_res_path)->set_reloadable(is_reloadable); |
There was a problem hiding this comment.
A GDExtension can't be switched from non-reloadable to reloadable without restarting the Godot editor - if it's reloadable, there's bookkeeping that needs to start when it's first loaded.
So, at most, this should update the config, but it shouldn't actually mark the GDExtension object as reloadable or attempt to reload it, unless it was already reloadable when the editor first started. Otherwise, bad things (including crashing) could happen
In fact, I just tested this with the godot_openxr_vendors extension, and setting it to reloadable crashes the editor
There was a problem hiding this comment.
Changed to only attempt reloading if it's already reloadable and is still reloadable.
|
Switching the entry symbol would have similar problems, no? All you can really do is break it. |
If the GDExtension were reloadable (at editor startup), and you change the entry symbol to an invalid value, on reload it should unload the GDExtension but fail to load it again. That may make things work incorrectly, but it shouldn't crash the editor |
|
That's true, though i still don't understand what the use case of this is. |
|
once you can rebuild from the editor you could tab out change the name tab in rebuild and now it is loading again. Or on the other side you might already have modified the name and it didn't load specifically because of that you change it from the ui and it reloads fine |
7292aac to
2387e8d
Compare
99eb412 to
40b3dfa
Compare
dsnopek
left a comment
There was a problem hiding this comment.
Thanks! With the latest code changes, this looks good to me (I haven't re-tested it, though)
|
I still don't understand what the use-case of an editor for a file is where all you can do with the file is break it. |
|
We discussed this at today's GDExtension meeting and there were some really great points raised. This is definitely a feature that people have asked for repeatedly, usually in the context of "we had this in GDNative, and I want it for GDExtension too," but also new users saying that editing the .gdextension file is a pain point when creating their first extension. However, this feature only makes sense for people creating a GDExtension, and not developers only using GDExtensions, and this would make it very easy for those developers to inadvertently break things (and it was brought up that ~19% of folks still don't use version control per the last poll). This argument, in particular, has kind of won me over to this line of thinking. It was also brought up that this doesn't make all fields of the .gdextension file editable, so if you were creating a GDExtension, you'd still need to edit this file in a text editor anyway. We could commit to making all fields editable, but this is more work and on-going maintenance. On #90979 there was a lot of support for having the developer tools for creating a new GDExtension be an addon, and so this editing functionality could also be a part of that? That said, there was a lot of support for having the list of GDExtensions in the editor in the Project Settings dialog, and the ability to view some information from the .gdextension files - just not for editing it. |
|
I think it might make sense to move the interesting information out of the popup - min version, max version only I think - and just delete the popup for now. |
|
@dsnopek @Ivorforce Ok, but since that is considerably different, I've opened a new PR: #118063. After PR #118063 is merged I can rebase this one, we'll probably close this PR anyway but at least then the branch will be available for reference in a more up-to-date form. |
dsnopek
left a comment
There was a problem hiding this comment.
I'm not requesting any particular changes, just setting this status to make ensure it doesn't accidentally get merged per our discussion at the GDExtension meeting yesterday
On the counter part of that is you can already butcher a plugin easily through the file system by deleting the wrong file and the solution to that is simply to re-download the plugin, you don't really need version control to prevent those issue. I feel like people know they shouldn't mess too much with things that they have imprted and when they do so I would expect to at least know that if something is wrong they should just re-download it. |
40b3dfa to
949f691
Compare
Ivorforce
left a comment
There was a problem hiding this comment.
Since the precursor PR was merged, this PR now offers only functionality that I do not agree are useful.
I would suggest instead adding a text editor feature for editing .gdextension files, as that is more feature complete, less inviting to 'accidentally break the file', and should require less maintenance work in the future.
This PR adds a GUI to view and edit GDExtensions from inside the Project Settings dialog.
This code was originally in PR #90979, but I separate it out by request. This would need to be merged first before PR #90979 is ready to merge, because this is a subset of PR #90979.
See this proposal godotengine/godot-proposals#13518 note that in a previous version of PR #90979 I included the ability to edit libraries and icons as requested by this proposal but I was told to remove it because it had bad UX, this is something that can be added in later if someone figures out good UX for it.