Just a discussion thread

[nilaymaj]

@4ian I've created a very barebones prototype for simple commands. The code is not very clean, as it's just a prototype for now. I'll add a prototype for Goto Anything commands in a few days.

• For now, I've created a separate context provider component in CommandPanel/context.js, and used it in both BrowserApp and LocalApp. Later, I guess we can move it into the Providers component.

• I've added two commands in Mainframe. Everything seems to work - the enabled property of commands is being updated properly and handlers are updated if the handling function changes (the sample Close Project command gets updated on each Mainframe rerender due to the point mentioned below).

• Some functions in Mainframe haven't been wrapped in a useCallback (for eg, askToCloseProject). These functions are also used in ElectronMainMenu, which means some IPC listeners are probably reset on every Mainframe render. Doesn't break anything (thanks to the custom hook in ElectronMainMenu) but still, that should be fixed, right?

This PR is just a discussion thread for the prototyping period. I guess we can shift to a proper draft PR in the main repo when the coding period starts, if that's okay.

[4ian]

For now, I've created a separate context provider component in CommandPanel/context.js, and used it in both BrowserApp and LocalApp. Later, I guess we can move it into the Providers component.

Yes, that's fine to start like this, you can add a TODO to remember that this should be moved to Providers which is indeed dedicated to holding all the common providers to avoid cluttering BrowserApp/LocalApp (which is dedicated to setting the different "bricks" specific to electron or the web-app).

I've added two commands in Mainframe. Everything seems to work - the enabled property of commands is being updated properly and handlers are updated if the handling function changes

That seems a good start! :)

Some functions in Mainframe haven't been wrapped in a useCallback (for eg, askToCloseProject). These functions are also used in ElectronMainMenu, which means some IPC listeners are probably reset on every Mainframe render. Doesn't break anything (thanks to the custom hook in ElectronMainMenu) but still, that should be fixed, right?

It's indeed:

• not breaking, because hooks are properly written
• but not optimal.

What I suggest is to make PRs on the MainFrame to add a few useCallbacks. Not necessarily on everything, just a few ones to avoid re-renders. It's a large component so we won't do everyhting in a single pass, but starting this could improve efficiency in the long run :)

[4ian]

I see a lot of find/findIndex - which can becomes costly when we'll have 200 commands. Do you think we could store instead commands in an object ({[string]: Command}), indexed by the command name? This means that access will be done in constant time, whatever the number of commands.

[nilaymaj]

Okay, so there is a possibly big issue. I just realized that the contents of tabs aren't really unmounted when the user switches to another tab - it just gets hidden. I know it's essential for instant tab switching, but now I somehow need to withdraw options when the user switches to another tab - I don't think I can know about that from inside of ObjectsList, can I? Info about whether a tab is open or hidden seems to be present at the TabContentContainer level.

One solution I see is to create a context at the TabContentContainer level, that provides info about whether the tab is open or closed. The command hooks can then use this context to manage handler overrides and withdraws. This will probably suit almost all goto commands, although seems a little less flexible than before. Is it okay, or is there a better approach?

By the way, there may be some goto commands that are odds amongst the rest - for example, the "Open recent project..." command. Unlike other goto commands, it has nothing to do with the current project at all, and has to be enabled regardless of whether a project is open or not. In fact, this command seems to be really different, since it doesn't seem to be related to a component. What should I do for such commands?

[4ian]

I just realized that the contents of tabs aren't really unmounted when the user switches to another tab - it just gets hidden

Yep you're right.

I don't think I can know about that from inside of ObjectsList, can I?

Indeed you can't know it from ObjectsList (and you should not try to know it from here, it's not this component responsibility - the component must just expose the commands, it's the context responsibility to know what to do with these).

Info about whether a tab is open or hidden seems to be present at the TabContentContainer level.

Correct, MainFrame controls which tab is active, and pass it to TabContentContainer and to the "EditorContainer",
https://github.com/NilayMajorwar/GDevelop/blob/d9135636fe151425623d21968b18790d57c47ac2/newIDE/app/src/MainFrame/index.js#L1645-L1648

One solution I see is to create a context at the TabContentContainer level, that provides info about whether the tab is open or closed

I'm also thinking of this solution as a good one.

The command hooks can then use this context to manage handler overrides and withdraws. This will probably suit almost all goto commands, although seems a little less flexible than before.

It's not necessarily less flexible. We can have three stuff:

• The "top level" context, which is the CommandsContext. It is provided by a "top level" CommandsContextProvider (around MainFrame).
  • "Top level" is just a fancy word to say that there is a single provider/context that is storing everything.
  • This is what you already started :)
• The hooks, that uses CommandsContext to register or unregister commands.
  • This is what you already started :)
• A CommandsContextScopedProvider. "Scoped" means that this provider is a way to "limit" the commands. How does it work? You can see it as a "bridge", a "facade", a "proxy". It acts as a provider and at the same time use the context :D It remember the commands that were registered, by keeping a copy of them, but only register the commands in the parent context if its "active" prop is used

It looks something like this:

function CommandsContextScopedProvider(props) {
  const commandsManager = React.useContext(CommandsContext);
  const scopedCommands = React.useRef<{ [string]: Command }>({});
  const scopedCommandsManager = {
    register: (cmdName: string, cmd: Command) => {
      scopedCommands.current[cmdName] = cmd;
      if (props.active) {
        commandsManager.register(cmdName, cmd);
      }
    },
    unregister: (cmdName: string_ => {
      delete scopedCommands.current[cmdName];
      commandsManager.unregister(cmdName);
    },
  } 
  React.useEffect(() => {
    if (!props.active) { 
      // Logic to unregister all commands from `scopedCommands` from commandsManager.
    } else {
      // Logic to register all commands from `scopedCommands` into commandsManager.
    } 
  }, [props.active]);

  return <CommandsContext.Provider value={scopedCommandsManager}>
        {props.children}
    </CommandsContext.Provider>;
}

The important point is that it must expose the exact same interface as the top level context (see, it's a CommandsContext.Provider).
By doing this, the components will be totally agnostic about which provider is used. You can use it finally in MainFrame:

          <TabContentContainer key={editorTab.key} active={isCurrentTab}>
            <CommandsContextScopedProvider active={isCurrentTab}>
              <ErrorBoundary>
                {editorTab.renderEditorContainer({

With this, it's now the job of CommandsContextScopedProvider to dynamically register or unregister the commands that are stored inside.

I think it's both flexible and clean (there is a single context for the whole codebase, and just two different providers, the top levle one and the "scoped" one). If you properly write the scoped one, tabs will work out of the box.
By the way, even better (but this need more thinking): instead of "hiding" the commands (by not registering them), the CommandsContextScopedProvider could register them with an additional prefix (which you could call.... a scope ;)).

    register: (cmdName: string, cmd: Command) => {
      scopedCommands.current[cmdName] = cmd;
      if (props.active) {
        commandsManager.register(props.scopeName + ' > ' + cmdName, cmd); // Just an example, the scopeName could be passed differently.
      }
    },

scopeName can be the scene name, the external layout, external event or extension name.

In fact, this command seems to be really different, since it doesn't seem to be related to a component.

Well these commands seems very related to a component to me :)
=> ElectronMainMenu.
What do you think?
Though this won't work on the web-app.. but most web-apps equivalent (opening, closing a project) are in ProjectManager.

This could even be in MainFrame, in this case in a custom hook useCommonCommands.