Thoughts about the IDE architecture #2739
Replies: 5 comments 18 replies
-
I think most decisions boil down to a few concepts:
What is really great about this whole discussion is that it's an architecture discussion :) React make it very explicit what you pass a props, so it's a pleasure to discuss: "should we put this prop here or here?" "Is this component responsibility really to do the renaming, or should we put it in a higher component?" Note that I agree some components are requiring too much stuff, like the |
Beta Was this translation helpful? Give feedback.
-
Hm I had an idea, but I am unsure if it is a good one. What if we used for interacting with the project a sort of decoupled system for handling the data, where we would have operations that the component can call that do all the changes to the project. Like, it could look like this mockup: // ProjectState.js
let project;
export const loadProject = () => {
project = gd.loadTheProject();
}
export const useProjectSettingsState = () => {
const forceUpdate = useForceUpdate();
const state = {
name: project.getName(),
author: project.getAuthor(),
};
const chageState = (setting, newValue) => {
switch (setting) {
case "name":
project.setName(newValue);
break;
case "author":
project.setAuthor(newValue);
}
forceUpdate();
}
return [state, changeState];
}
export const useExtensionFunctionListState = (extensionName, behavior) => {
const forceUpdate = useForceUpdate();
const extension = project.getEventsBasedExtension(extensionName);
const eventsFunctionsContainer = behavior ? extension.getBehavior(behavior) : extension;
const state = eventsFunctionsContainer.getAllNames().toJSArray();
const changeState = (setting, args) => {
switch (setting) {
case "rename":
// Do verification and stuff
eventsFunctionsContainer.set(args.newName, eventsFunctionsContainer.get(args.oldName);
eventsFunctionsContainer.delete(args.oldName);
break;
case "moveUp":
eventsFunctionsContainer.offsetIndex(eventsFunctionsContainer.get(args.functionName), -1);
}
forceUpdate();
}
return [state, changeState];
} // ExtensionFunctionsList.js
import { useExtensionFunctionsListState } from "ProjectState";
export const ExtensionFunctionsList = ({ extensionName, behavior }) => {
const [list, changeState] = useExtensionFunctionsListState(extensionName, behavior);
return list.map(function => (<>
<p>{function}<p>
<button onClick={() => changeState("rename", function, "new name")}>Rename</button>
</>));
}; // ProjectSettings.js
import { useProjectSettingsState } from "ProjectState";
export const ProjectSettings = () => {
const [{ name, author }, changeState] = useProjectSettingsState();
return (<>
<TextField value={name} placeholder="Project name" onCommit={(newValue) => changeState("name", newValue)} />
<TextField value={author} placeholder="Author" onCommit={(newValue) => changeState("author", newValue)} />
</>)
}; This is just a simplistic mockup, but if done well such a pattern would have multiple advantages i believe:
It also would not require any big refactor, as it could be adopted gradually (nothing stops us from still passing down project and other libGD objects down while having this alternative solution). |
Beta Was this translation helpful? Give feedback.
-
Sorry for still being insistent but I can't help but think of this each time I read From the lack of any answer, I infer that my points were unconvincing and not answering your concerns, so I will try to summarize my opinion on the matter again while trying to keep your points in mind.
If it concerns you too much, I think we can try to find a solution. For example, generating all boilerplate code automatically from
I think that overall the benefits in DX (no dealing with force update, having one centralized place for IDE validation instead of being scattered among components, no dealing manually with the undo stack, no dealing with modification in another component...) outweigh the costs (one more abstraction to deal with).
I'm not sure if we are talking about the same thing, I am talking about moving functions like
Most operations probably could be generated automatically, for example when a string, number, boolean, etc has a getter and setter, we can automatically generate code like this (pseudo-api): function setString(variable, newValue) {
const previous = variable.getString();
const undo = () => variable.setString(previous);
const redo = () => variable.setString(newValue);
UndoStack.push(undo, redo);
redo(newValue);
} The generation of this boilerplate could be done from the IDL files, just like how the types are currently generated. For more complex operations, for example, if the returned type is a complex class, we could also autogenerate something based on gd::Serializer. |
Beta Was this translation helpful? Give feedback.
-
So I thought a bit more about this, and indeed I realize now that adding another layer here is not a good idea, and not even necessary for what I want. We can actually do it all in C++. I was proposing two main things. I'll make a post for each to discuss them independently as they are rather different. Here is the first one: For any project mutation, if possible we should implement an undo operation. We could make it easier by having a global undo stack, and we could make macros for simple data structures like strings and numbers, for example (untested pseudocode to illustrate the idea): // UndoStack.cpp
struct Operation {
std::function<void()> do;
std::function<void()> undo;
Operator(std::function<void()> do_, std::function<void()> undo_) {
do = do_;
undo = undo_;
}
}
class UndoStackClass {
private:
std::vector<Operation> past;
std::vector<Operation> future;
public:
do(std::function<void()> do, std::function<void()> undo) {
Operation o(do, undo);
past.push_back(o);
o.do();
}
undo() {
if(past.empty()) return;
Operation& o = past.pop();
future.push_back(o);
o.undo();
}
redo() {
if(future.empty()) return;
Operation& o = future.pop();
past.push_back(o);
o.do();
}
};
UndoStackClass UndoStack; // Project.cpp
#include "UndoStack.cpp"
class Project {
GD_PROPERTY(gd::String, author)
GD_PROPERTY(gd::String, name)
// GD_PROPERTY expands to
private:
gd::String name;
public:
gd::String GetName() { return name; };
void SetName(gd::String name_) {
gd::String oldname = name;
UndoStack.do([this&](){
name = name_;
}, [this&, oldname]() {
name = oldname;
});
};
}; We could also make generic "Container" data structures integrating the undo stack: class Map<T> {
private:
std::map<T> map;
public:
T Get(const gd::String& key) {
return map.get(key);
};
void Set(const gd::String& key, T item) {
if(map.has(key)) {
gd::SerializerElement newItem;
item.SerializeTo(newItem);
gd::SerializerElement oldItem;
map.get(key).SerilaizeTo(oldItem);
UndoStack.do([this&, key, newItem](){
map.get(key).UnserializeFrom(newItem);
}, [this&, key, oldItem](){
map.get(key).UnserializeFrom(oldItem);
});
} else {
UndoStack.do([this&, key, item](){
map.set(key, item);
}, [this&, key](){
map.delete(key);
});
}
};
void Delete(gd::String key) {
if(!map,has(key)) return;
T item = map.get(key).Copy();
UndoStack.do([this&, key](){
map.delete(key);
}, [this&, key, item](){
map.set(key, item);
});
}
// etc
}; I think that this would require a bit of effort to add such an undo stack to the codebase, but once it is added it would be a minor effort to actually implement an undo function each time we add a new project mutation, and it would really pay off in user-friendliness by allowing to have very fine-grained undo-redo and make sure that anything you do can be undoable and redoable. It is a basic expectation from any user today from an editor to have a fully working undo-redo feature, and to be honest if there is any reason that might make me want to stop using GDevelop, that lack of usability of the undo-redo would be it. |
Beta Was this translation helpful? Give feedback.
-
Beta Was this translation helpful? Give feedback.
-
Every time I am working on the IDE, I get very frustrated by a few things. I thought I would share them here because I feel like if those issues were adressed everyone could be more productive, or I could improve my workflow if there is something I am missing.
Lack of global context
The most common example of this is the
gd::Project
instance, it is often needed pretty much everywhere in the IDE, but needs to be passed down the props, which can be very frustrating when working on a component that usesproject
with components that do not have it passed down as prop. It requires looking for every usage of the component, then looking for every time those components are used to passproject
down, etc which is very time consuming, especially when the IDE cannot scan for other components using the current component if the current component isn't a named export.A solution to that problem would be to make
project
global state, using for example the context API. I think I have seen this proposition at least once before, so I am wondering why it has been rejected."Overusage" of props.
I consider overusage of props is the splitting of logic of 1 action between 2 components for no apparent reason. Of course, maybe I am just overseeing the reason, but from my point of view, this is wasting much time. Here is an example. Let's say I want to look into renaming an EventsFunction. I may find this first:
GDevelop/newIDE/app/src/EventsFunctionsList/index.js
Lines 126 to 146 in 127a881
But, as you may see, the complete renaming function is not present here, part of it is handled by a prop
onRenameEventsFunction
. So now I need to search for all components using it, only to find out that there is only one other component using it, and pass the following prop:GDevelop/newIDE/app/src/EventsFunctionsExtensionEditor/index.js
Lines 233 to 266 in 127a881
I see no reason why this had to be passed as a prop, as EventsFunctionsList can do each operation done in that function passed as :
project
and the events based extension passed down as props and can therefore callgd.WholeProjectRefactorer.renameEventsFunction
gd.Project.validateName
andisExtensionLifecycleEventsFunction
as they do not require any special objects as parametersSo this splitting is only making the code more complex harder to read and modify, without any apparent positive impact.
"Overmodularizing" of some components
The other day, I was taking a look at events functions autocompletions and something that struck me was the following code:
https://github.com/4ian/GDevelop/blob/master/newIDE/app/src/EventsSheet/ParameterFields/GenericExpressionField/index.js#L513-L534
More specifically those props:
https://github.com/4ian/GDevelop/blob/master/newIDE/app/src/EventsSheet/ParameterFields/GenericExpressionField/index.js#L516-L524
I find this annoying because it requires that whoever uses that component separately imports
ExpressionAutocompletionsHandler
, and to call the methods manually when passing the props where the component could do it itself. It would be way more straightforward to just passautocompletions={this.state.autocompletions}
as props, which contains all the needed data, and let the component call the functions to adapt the list for rendering it via itself itself.One could argue "this is only a view, a Displayer, and it should not do the logic". I would not agree, as this logic is necessary for the component as it expects a trimmed list to function.
This is in my opinion again just making everything more complex when trying to work with that code, because I cannot see when looking just at the Displayer how the autocompletions are trimmed, but need to go look up in the components using it, even though they are trimmed exclusively for display inside it, and if I ever want to reuse that component I have to enter more props
instead of just giving it what I want it to display at once, and have to go import some other functions and call them myself etc. This is just a waste of time as far as I can tell.
Last words
Sorry for what turned out to be this big rant, again I may just be misunderstanding the workflow or structuring logic behind those choices, but the frustration about those points have been building up this last few months and I think that if I have issues with this workflow that other might have trouble understanding it too, and that it may really allow a better productivity and contributors count if the codebase is nice for everyone to work with.
Beta Was this translation helpful? Give feedback.
All reactions