Main project discussion thread

[nilaymaj]

No description provided.

[nilaymaj]

@4ian

I've added a basic UI component for the command palette - here's a screenshot (on Nord theme ❤️ )

I've added three commands for now - converting some Mainframe functions into callbacks in the process, and all three are working correctly - properly registering and deregistering (although the Close Project one might be updated more frequently than necessary - I'm not sure).

Added a new option to the debug toolbar dropdown for now, as you suggested. The "Command" box is for the search input (just a dummy for now, though).

Please try it and point out any changes or improvements :) In the meantime, I'll continue with adding more commands, I guess.

Also, please ignore the React warnings for now - I'll fix that in a while 😅

[nilaymaj]

I've added all the global commands from the Trello list in Mainframe. Everything seems to be working well, but five of the commands' handlers are updated on every Mainframe rerender - I've marked these on Trello (see the "Commands added" list). I'll start with fixing them now.

[4ian]

That's great progress! :D
I've not yet tried it but I read the code. :) As we already discussed the overall approach looks good.

I wrote tons of nitpicking, because I'm very picky (sorry ;)) but also because it's important to get the convention good now to avoid extra work later.

Overall this is a very good start. Looking forward to what is coming next 👍👍

[nilaymaj]

@4ian

I started with fixing the unnecessary handler updates, and turns out it will need a little bit more than just adding React.useCallbacks. For example, some functions use things that are defined after that function, and adding those things to dependency lists means I'll have to reorder the functions so that they're used after being declared. Also, every function that is somehow called in any of the command handlers will have to be wrapped in a useCallback to prevent unnecessary handler updates - so maybe around 10-15 (or 20) useCallbacks will be added.

Should I proceed with this? Otherwise, I can focus on finishing the UI part first, then move on to fix these issues.

[nilaymaj]

[nilaymaj]

I've added a very temporary keyboard shortcut for opening the command palette (see previous commit 07727fc). It's just a useEffect in Mainframe. The code looks pretty ugly - it's more like "yeah it works" than actual code.

The vanilla JS logic is a bit different than before. I realized that there was an issue with the original approach I mentioned. The problem was that clicking on the empty space in properties panel, etc led to focus somehow going to the body tag, which is outside div.main-frame. To fix that, now I also allow opening the palette if the active element is the body tag. Not completely pretty, but seems to work pretty well now.

[4ian]

I think it's fine for now! Would be great to move this to a custom hook provided newIDE/app/src/CommandPalette/ (I let you choose if you want to create a new file or use an existing one).

[nilaymaj]

Done. I've moved the code to a custom hook in .../CommandPalette/CommandHooks.js for now - the code is specific to the command palette anyway, due to the Ctrl+P keyboard shortcut being hardcoded into the hook.

I'll probably move it to a more appropriate location (and make it more flexible) when I start with shortcuts, since this piece will be reused then to enable and disable shortcuts according to whether an overlay is open.

[4ian]

I'll probably move it to a more appropriate location (and make it more flexible) when I start with shortcuts, since this piece will be reused then to enable and disable shortcuts according to whether an overlay is open.

Yes, we'll see how to handle customizable shortcuts later (we'll need them here, in commands, in KeyboardShortcuts.js and a few other places). It's fine to use a hardcoded shortcut for now because everything else is hardcoded.

Shortcuts will probably be stored in preferences, with maybe a context/hook to get the shortcut and use it in a command/KeyboardShortcuts.js (in the most appropriated format).

[nilaymaj]

I've made a few changes:

• Made changes to the palette UI so that a single Esc keypress now closes the palette.
• The Autocomplete is no longer wrapped in DialogTitle - this tweaks the UI in a pretty interesting way, actually:
  (left: old, right: new)
  
• There was an issue that I forgot to mention earlier - the dropdown appeared fraction of a second before the dialog (and input box) appear, and it didn't look very smooth. It was happening because the dialog had a fade transition by default, while the dropdown is rendered by portal and thus appears instantaneously. To fix this, I've removed the dialog fade transition (made transition duration 0s).

Next, I'll try to add a preference flag for the palette. I was thinking of checking for the preference flag inside the keyboard shortcut hook I added, and ignoring shortcut-press if flag is disabled - a drawback is that command handlers will be registered and deregistered in the background even if the command palette is disabled, but I can't find an easy way to disable the whole command system with a preference flag.

[nilaymaj]

Done. Moved the edit layer effects logic into scene editor. Split the scene editor commands into panel-specific parts as you said, and its certainly cleaner now!

[nilaymaj]

The next thing (and maybe last thing related to commands?) is to add a command to ToolbarIcon. Here's what I'm thinking:

• Every command needs a unique identifier, which is tough to get from the existing props of ToolbarIcon (i.e. icon src, tooltip string, accelerator string, disabled, onClick, onContextMenu).
• Some toolbar buttons' tooltips are a bit odd when shown in command palette as the command's display text (the preview button's tooltip has "Launch preview, right click for more", for example). Not sure what to do about that.
• Dropdown toolbar icons are a bit tricky - ToolbarIcon does not have access to the options so can't register a command with options on its own, and adding useCommandWithOptions to ElementWithMenu does not seem right to me (or is it?).

[4ian]

Right there are some limitations, so either we:

• refactor ToolbarIcon to make it "smarter" by adding stuff like a unique identifier, a list of option so that it's then using internally ElementWithMenu (or introduce an intermediate component doing all of that). And probably an optional prop to disable command generation.
• or we decide this is not worth the added complexity and simply add command hooks in the various Toolbar components.

While I like the idea of adding a button = automatically adding a command, which is nice on the paper, it's maybe not worth the complexity (especially for options => this would mean adding a new abstraction about menu/options... not a big fan). So let's go for just adding commands in the Toolbar components and call it a day :) It should still result in something that is very easy to maintain and understand, so it's actually a better solution when thinking about it.

[nilaymaj]

I've added commands for almost every toolbar button in scene editor and events editor, and it's working fine. For each toolbar, I've created another file ToolbarCommands.js in the same folder, that contains all the commands. There's a bit of redundancy in props and display text - although moving commands into the toolbar components themselves looks uglier to me.

Also, I realized a few commands are still left - the ones corresponding to items under "Game settings" in Project Manager (Game properties, global variables, etc). Adding them to ProjectManager won't work directly because it gets rerendered only when its opened so dialog-related state updates won't trigger opening the dialog. I can add return true to shouldComponentUpdate if any dialog state variable changes. Is that solution okay?

edit: Also, are there any other commands that are to be added now?

[nilaymaj]

@4ian I've fixed unnecessary updates for some commands, specifically the "Edit layer effects" command and three event sheet toolbar commands. With that, I don't think there are any more significant "update leaks" for any command.

Some commands haven't been added, see above message for that.

If there's nothing else left, should I clean up and create a PR on the main repository?

[4ian]

I think it's a good time for a PR! :)

specifically the "Edit layer effects" command and three event sheet toolbar commands. With that, I don't think there are any more significant "update leaks" for any command.

Sounds good, thanks for that!

For each toolbar, I've created another file ToolbarCommands.js in the same folder, that contains all the commands. There's a bit of redundancy in props and display text - although moving commands into the toolbar components themselves looks uglier to me.

I'm fine with both :)

I can add return true to shouldComponentUpdate if any dialog state variable changes. Is that solution okay?

I think that yes it's a good solution. Basically this means that the existing shouldComponentUpdate was incomplete in its implementation (checking the state is something that I did in some other components that have a shouldComponentUpdate).

edit: Also, are there any other commands that are to be added now?

If we have commands for all toolbars, for all stuff in the ProjectManager, I think we're in good shape! Let's open a PR and see if we miss commands and add them on the go :)

[nilaymaj]

well, turns out I used project variable already sweat_smile Fine to keep it then, but let's keep "global variables" in the user facing sentence

Didn't really understand - is the term "project variables" already used for something else in GDevelop?

[4ian]

is the term "project variables" already used for something else in GDevelop?

No, it's a proper term for what you've done. It's just that I chose "global variable" for the interface shown to the user and for the documentation (like here).
Project variable is a synonym for "global variables", and as it's already used in the codebase, it's fine to keep it.

[nilaymaj]

Closing this one. Opened #3 for work on shortcuts.