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

Toggle Buttons in SceneEditor Toolbar #2215

Closed
wants to merge 19 commits into from
Closed

Toggle Buttons in SceneEditor Toolbar #2215

wants to merge 19 commits into from

Conversation

INNOVATIVEGAMER
Copy link
Contributor

1.Following buttons in toolbar provided with toggle functionality.
2.Buttons BG changes depending upon toggle state of the subEditor.

Current Problems :
1.Clicking on the close button in subEditor do not changes the subEditor state.

const defaultToolbarControls = [<CloseButton key="close" />];

(As close button mentioned only in the object-list editor as separate attribute and for other subEditors as a default.
So need some help how close button can reference state of sceneEditor)

Example of how scene editor looks now :
GDevelop 5 - example___platformer - Google Chrome 15-01-2021 20_17_03

Preview Video :
https://streamable.com/nykuzj

1.Following buttons in toolbar provided with toggle functionality.
2.Buttons BG changes depending upon toggle state of the subEditor.
@INNOVATIVEGAMER
Copy link
Contributor Author

@Bouh As we discussed on forum. I converted these buttons to toggle and added some styling depending upon the toggle state.
Now as you are saying in forum we can work on it further in this PR with discussions.

@Bouh
Copy link
Collaborator

Bouh commented Jan 15, 2021

I'am not sure at all for the background.
The toggling seems to work fine with the default layout, have you tried to toggle a panel that has been moved?

@INNOVATIVEGAMER
Copy link
Contributor Author

INNOVATIVEGAMER commented Jan 15, 2021

have you tried to toggle a panel that has been moved?

@Bouh After you mentioned this I tried changing the position of subEditors at different positions and it all worked fine in toggling. But the position of the subEditor changes to default layout position. (This functionality you are mentioning was never incorporated in GD though).
But I will try to work on this.
Do you have any solution for the problem I have mentioned ?

@Bouh
Copy link
Collaborator

Bouh commented Jan 15, 2021

const defaultToolbarControls = [<CloseButton key="close" />];

  • I think you can add a new props in as the dev recommand.
    And use the props in onClick ( where mosaicActions.remove(mosaicWindowActions.getPath()); is used)

You have twice <CloseButton> in the codebase, be sure to edit both.
In onClick event you must call a function for turn off the state of the panel.

But i don't know how know which editor it is.
There is no types for the editors/panels from GD to Mosaic.
There is only a title, but we shouldn't use it.

  • For the background color it is not conclusive and totally breaks the immersion.
    Let's remove the css code and keep the original visual style of the buttons.

@INNOVATIVEGAMER
Copy link
Contributor Author

INNOVATIVEGAMER commented Jan 16, 2021

For the background color it is not conclusive and totally breaks the immersion.
Let's remove the css code and keep the original visual style of the buttons.

Yes, I just placed the background for temporary with colors.
Do you have any suggestions for on/off state styling ?
Or shall I show you some examples and then we will discuss on them ?

@Bouh
Copy link
Collaborator

Bouh commented Jan 16, 2021

Or shall I show you some examples and then we will discuss on them ?

Material-ui have no recommendation for this kind of button with a big icon with a big filling.
To do add a background related to the state we need to remade the icons, which is not at all planned and would require the work of a real UI designer.

Do you have any suggestions for on/off state styling ?

Keep the original visual style of the buttons, remove all css added in this PR.

1.Fixed issue with editor toggling when close button clicked for closing the editor
2.Fixed issue with persisting editors states in scene editor on reload.
3.Commented the css styling of the toolbar icon for future reference.
@INNOVATIVEGAMER
Copy link
Contributor Author

But i don't know how know which editor it is.

I used a small trick here by passing a function as a prop to close button which changes the editor state. ;)

I have fixed another issue in the scene editor which I came across while using these toggles,
The issue is when we left some subEditors open/close, subEditors state did not get updated in state of the SceneEditor when SceneEditor mounts on the screen.
Previously the EditorMosiac handles this issue (by luck) with initialNodes. Because previously it cares only about the nodes it gets as a prop but now as SceneEditor state is responsible for rendering of various editors its state should be updated as per the initialNodes before it mounts.

Now SceneEditors state is truly responsible for the rendering of different subEditors on startup and in between.

@INNOVATIVEGAMER
Copy link
Contributor Author

INNOVATIVEGAMER commented Jan 16, 2021

<InfoBar
identifier="objects-panel-explanation"
message={
<Trans>
Objects panel is already opened: use it to add and edit objects.
</Trans>
}
show={!!this.state.showObjectsListInfoBar}
/>
<InfoBar
identifier="instance-properties-panel-explanation"
message={
<Trans>
Properties panel is already opened. After selecting an instance on
the scene, inspect and change its properties from this panel.
</Trans>
}
show={!!this.state.showPropertiesInfoBar}
/>
<InfoBar
identifier="layers-panel-explanation"
message={
<Trans>
Layers panel is already opened. You can add new layers and apply
effects on them from this panel.
</Trans>
}
show={!!this.state.showLayersInfoBar}
/>
<InfoBar
identifier="instances-panel-explanation"
message={
<Trans>
Instances panel is already opened. You can search instances in the
scene and click one to move the view to it.
</Trans>
}
show={!!this.state.showInstancesInfoBar}
/>

I think these messages saying "panel is already opened" are not useful anymore as no one is able to open a panel that is already opened. User either will open/close by clicking on toggle.

To do add a background related to the state we need to remade the icons, which is not at all planned and would require the work of a real UI designer.

I think this is a good plan to go forward on this feature. But please be sure to tell the users about toggle functionality or no one will ever notice it unless they accidently double click on any button.

!Warning!
Users need to wait for some milliseconds while toggling the editors , currently it is not superfast as react takes its time while changing the state. But I think it is not a measure issue for now :).

@INNOVATIVEGAMER
Copy link
Contributor Author

@4ian & @Bouh this PR is ready now. Can you Please review it, so I can make PR on other issues.
Also check my comment about removing those "panel is already open" messages.

@@ -11,6 +11,7 @@ type Props = {|
disabled?: boolean,
onClick?: () => void,
onContextMenu?: () => void,
showComponent?: boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was used for styles, as we don't want style remove this props everywhere.

Comment on lines 30 to 46
//We can use here (showComponent) value from props to set the UI texture of the button to ON/OFF state
//Following example shows how to use this in simple manner
// let buttonStyle = {};
// if (showComponent !== undefined) {
// buttonStyle = showComponent
// ? {
// border: '5px solid #80BE1F',
// backgroundColor: '#80BE1F',
// marginRight: '5px',
// }
// : {
// border: '5px solid #FF3232',
// backgroundColor: '#FF3232',
// marginRight: '5px',
// };
// }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this useless code please.

Comment on lines 40 to +43
<ToolbarCommands
openObjectsList={this.props.openObjectsList}
openObjectGroupsList={this.props.openObjectGroupsList}
openPropertiesPanel={this.props.openProperties}
toggleObjectsList={this.props.toggleObjectsList}
toggleObjectGroupsList={this.props.toggleObjectGroupsList}
togglePropertiesPanel={this.props.toggleProperties}
Copy link
Collaborator

@Bouh Bouh Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ToolbarCommands component is used for bind an action to shortcuts.
In your case this code isn't complete, because props in ToolbarCommands are not complete.

See if you press O for open the object editor the app should crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now toggle functionality linked to shortcut keys.

@@ -24,6 +28,7 @@ export default class CloseButton extends Component {
{({ mosaicWindowActions }) => (
<IconButton
onClick={() => {
if (this.props.closeActions) this.props.closeActions();
mosaicActions.remove(mosaicWindowActions.getPath());
Copy link
Collaborator

@Bouh Bouh Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See in newIDE\app\src\UI\EditorMosaic\index.js you have the current stuff for add a node in mosaic with addNode.
It would be better to add a similar for removeNode.
Same for nodeExist for checking if an editor living in the node tree of mosaic.
This way you have not to check or add a state for know if the panel is already open.
And all your stuff in componentWillMount in newIDE\app\src\SceneEditor\index.js will be useless.

Not sure of the best way to do it thought.

Copy link
Contributor Author

@INNOVATIVEGAMER INNOVATIVEGAMER Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React recommends toggling of components through react state. As you can see , Material-UI also follows this convention. This way we can use this state anywhere in component and other components.
mosiacAction.remove(path) is already doing the job of removal of node.
The code in componentWillMount I wrote only for the first render. That is when project loads for the first time.
By doing this we will set the sceneEditor state of the panels as they were before closing the project.

Another example of state use is as you can see here , we have used state to display message :

<InfoBar
identifier="objects-panel-explanation"
message={
<Trans>
Objects panel is already opened: use it to add and edit objects.
</Trans>
}
show={!!this.state.showObjectsListInfoBar}
/>

So I don't think removing state is a good idea in case of toggle components. But You gave a solid idea of creating this removeNode function. Now speed of toggling of panels can be increased by 200%.
Thank you for your hint. In the next commit I will apply these changes.

@Bouh
Copy link
Collaborator

Bouh commented Jan 21, 2021

The first part of info bar can be edited for explain that panels are toggleable.

1.Panels in the scene editor can be toggled with shortcut keys also.
2.Changed the text to toggle keys in preferances tab -> shortcut keys.
3.added removeNode and hasNode functions in EditorMosaic.
4.added closeEditor function in EditorMosaic.
@INNOVATIVEGAMER
Copy link
Contributor Author

INNOVATIVEGAMER commented Jan 21, 2021

React recommends toggling of components through react state.

So I didn't removed the code in the componentWillMount method. Although app works fine without including it but the state of SceneEditor should be according to the state of panels, so if we need the state of the panels somewhere else it will not create any conflicts.
The code now works with shortcuts also.
@Bouh Please guide me if some refactors or updates are needed ? ;)

@INNOVATIVEGAMER
Copy link
Contributor Author

@Bouh Can you please review this PR now ?
I have checked the checks failing in the semaphoreci and those are because this PR is older so it can be fixed by merging with master.

Copy link
Owner

@4ian 4ian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds a lot of complexity in SceneEditor, by lifting only part of the state of EditorMosaic inside SceneEditor, meaning that we have a mix of imperative functions (openEditor, closeEditor in EditorMosaic) and declarative ones (toggleObjectsList, etc... in SceneEditor).

The additional code in componentWillMount is also something I want to avoid at all costs - it should not be the responsibility of SceneEditor to deal with this.

Two solutions:

  • either keep EditorMosaic responsible for handling its state (I.e: don't lift the state), in which case you can call a toggleEditor function on it. Advantage: easier. Disadvantage: you can't know the state (opened or not) from SceneEditor.
  • or do a more important refactoring work where all the functions to handle the state of an EditorMosaic is put in a React hook, that can be then used in SceneEditor (and the state is then passed down to EditorMosaic).

Appreciate your efforts so far! But in the current situation I think it's adding too much complexity for just toggling stuff. We have to either follow the current architecture or change it. componentWillMount should almost never be used.

@INNOVATIVEGAMER
Copy link
Contributor Author

The additional code in componentWillMount is also something I want to avoid at all costs - it should not be the responsibility of SceneEditor to deal with this.

Ok, so you mean remove all the following states for panels from the SceneEditor :

showObjectsListInfoBar: boolean,
layoutVariablesDialogOpen: boolean,
showPropertiesInfoBar: boolean,
showLayersInfoBar: boolean,
showInstancesInfoBar: boolean,
showAdditionalWorkInfoBar: boolean,

And give total responsibility of EditorMosaic of its state to itself.
and then use react hooks to know and control the state of the EditorMosaic.
So toggling can be done with these hooks as OnToggle first check whether the panel is open or not and act accordingly.

Is I am correct ? 😕

@4ian
Copy link
Owner

4ian commented Feb 3, 2021

And give total responsibility of EditorMosaic of its state to itself.

That's a possibility yes. If a state is a component, then by definition this component has the responsibility of it.
Currently, we can see that class EditorMosaic has the state of the nodes:

  state = {
    mosaicNode: this.props.initialNodes,
  };

so either:

  1. we keep the state here, and manipulate it through imperative functions like openEditor, or through _onChange which is called by MosaicWithoutDragDropContext itself. Advantage: easier, no much refactoring. Disadvantage: you can't know the state (opened or not) from SceneEditor. Because the state is inside the component.
  2. so the best alternative, but requiring a lot more refactoring is to separate the handling of the mosaicNode inside a hook (useMosaicNode). This hook would return: mosaicNode, a set of functions to open/change/toggle editor: openEditor, toggleEditor, onChange etc... and finally maybe a function to know if an editor is opened isEditorOpened.
  • Now that you have made this, you can refactor SceneEditor to use the hook. It's now trivial to know if an editor is opened (isEditorOpened('objects-list')), and you're not "polluting" SceneEditor because it's just a hook to call. You pass onChange to the EditorMosaic class, and any function that it needs.

2 is better but needs refactoring also in other editors. 1 is a simpler change to toggle just the editor, but you won't be able to show in the toolbar if something is toggled or not. Might not be worth it anyway?

Also remember that on small screens, clicking on buttons will replace the currently opened editor :)

EDIT: I know it's a bit discouraging because you already did work on this, but we need to find a solution that is not a mix of responsibility between components. Otherwise we would add an antipattern and technical debt to the codebase.

1.Added following public functions in EditorMosiac :
1)isEditorOpen
2)toggleEditor
and one private function :
1)_isEditorExist

2.Removed panels/editor states from SceneEditor
3.Removed componentWillUnmount from SceneEditor
4.refacotred toggling of the editors in SceneEditor
5.removed InfoBars related to panels in SceneEditor
6.removed closeActions function from CloseButton
@INNOVATIVEGAMER
Copy link
Contributor Author

  1. we keep the state here, and manipulate it through imperative functions like openEditor, or through _onChange which is called by MosaicWithoutDragDropContext itself. Advantage: easier, no much refactoring. Disadvantage: you can't know the state (opened or not) from SceneEditor. Because the state is inside the component.

@4ian I went with this approach and created two additional imperative functions named toggleEditor and isEditorOpen.
toggleEditor checks if the editor is open or close and acts accordingly.
isEditorOpen will remove your mentioned disadvantage in major cases where we want to know the state of the the editor is open or not ;).

I also removed these unnecessary infobars as they are not useful now, since toggling is enabled.

<InfoBar
identifier="objects-panel-explanation"
message={
<Trans>
Objects panel is already opened: use it to add and edit objects.
</Trans>
}
show={!!this.state.showObjectsListInfoBar}
/>
<InfoBar
identifier="instance-properties-panel-explanation"
message={
<Trans>
Properties panel is already opened. After selecting an instance on
the scene, inspect and change its properties from this panel.
</Trans>
}
show={!!this.state.showPropertiesInfoBar}
/>
<InfoBar
identifier="layers-panel-explanation"
message={
<Trans>
Layers panel is already opened. You can add new layers and apply
effects on them from this panel.
</Trans>
}
show={!!this.state.showLayersInfoBar}
/>
<InfoBar
identifier="instances-panel-explanation"
message={
<Trans>
Instances panel is already opened. You can search instances in the
scene and click one to move the view to it.
</Trans>
}
show={!!this.state.showInstancesInfoBar}
/>

The toggling is working fine now :)

@Bouh
Copy link
Collaborator

Bouh commented Feb 22, 2021

I got crash when i go for toggle panels.

react-dom.development.js:289 Uncaught Error: Path [first, second, second] did not resolve to a node
    at Object.getAndAssertNodeAtPathExists (mosaicUtilities.ts:169)
    at createRemoveUpdate (mosaicUpdates.ts:62)
    at removeNode (index.js:92)
    at EditorMosaic._this2.closeEditor (index.js:283)
    at EditorMosaic._this2.toggleEditor (index.js:302)
    at SceneEditor._this.toggleObjectsList (index.js:258)
    at HTMLUnknownElement.callCallback (react-dom.development.js:149)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:199)
    at invokeGuardedCallback (react-dom.development.js:256)
    at invokeGuardedCallbackAndCatchFirstError (react-dom.development.js:270)
    at executeDispatch (react-dom.development.js:561)
    at executeDispatchesInOrder (react-dom.development.js:583)
    at executeDispatchesAndRelease (react-dom.development.js:680)
    at executeDispatchesAndReleaseTopLevel (react-dom.development.js:688)
    at forEachAccumulated (react-dom.development.js:662)
    at runEventsInBatch (react-dom.development.js:816)
    at runExtractedEventsInBatch (react-dom.development.js:824)
    at handleTopLevel (react-dom.development.js:4826)
    at batchedUpdates$1 (react-dom.development.js:20439)
    at batchedUpdates (react-dom.development.js:2151)
    at dispatchEvent (react-dom.development.js:4905)
    at react-dom.development.js:20490
    at Object.unstable_runWithPriority (scheduler.development.js:255)
    at interactiveUpdates$1 (react-dom.development.js:20489)
    at interactiveUpdates (react-dom.development.js:2170)
    at dispatchInteractiveEvent (react-dom.development.js:4882)

image

@INNOVATIVEGAMER
Copy link
Contributor Author

INNOVATIVEGAMER commented Mar 4, 2021

I got crash when i go for toggle panels.

@Bouh this happens when you try to toggle the editors at high speed like double clicking the toggle button or fast double press.
So , I added some defensive code for this issue.
Toggling takes its time. So now if user accidently double click the button there will be no effect as added defensive code do not changes the state.
Sorry for this late reply 😥

@INNOVATIVEGAMER INNOVATIVEGAMER marked this pull request as draft June 2, 2021 10:13
@4ian
Copy link
Owner

4ian commented Aug 25, 2021

Closing as old.

@4ian 4ian closed this Aug 25, 2021
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

Successfully merging this pull request may close these issues.

3 participants