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

Add custom nested Blockly categories support #4236

Merged
merged 15 commits into from
Sep 22, 2023

Conversation

Goldorion
Copy link
Contributor

@Goldorion Goldorion commented Sep 19, 2023

This PR adds the possibility to make nested categories for Blockly editors. Nested categories work and are created exactly like regular categories. However, 2 new parameters have been added to allow this:

  • parent_category: A String (ID) used inside a category to define what category will be its parent.
  • is_expanded: If true, the category will expand its children (not recursive)

Example with Block management and Secondary #4 expanded.

https://github.com/MCreator/MCreator/assets/38427877/c9d77745-a4ae-4015-a56f-97b99ca6b5e8

An example of the features in use can be seen in this commit

@Goldorion Goldorion added the new feature New feature or request label Sep 19, 2023
@Goldorion Goldorion changed the title Add nested Blockly categories Add custom nested Blockly categories support Sep 19, 2023
@KlemenDEV
Copy link
Contributor

Is there a realistic case where one would want the category nested and not nested simultaneously for this parameter to be needed?

@Goldorion
Copy link
Contributor Author

Is there a realistic case where one would want the category nested and not nested simultaneously for this parameter to be needed?

It was mainly to cover all possible cases and because it was possible and easy 🤷 However, this also helps to make the system smaller and simpler as I avoid a loop iterating over all toolboxes to check if it's not a nested category (making performances slower and a bad code).

@KlemenDEV
Copy link
Contributor

KlemenDEV commented Sep 19, 2023

I would ask you in this case to mark toolboxes that are children during runtime while loading them as this is neither bad code nor it will not impact performance(s) badly that much.

You can do that ExternalBlockLoader at line 112 (lines at your branch)

@KlemenDEV KlemenDEV marked this pull request as draft September 19, 2023 14:07
@Goldorion Goldorion marked this pull request as ready for review September 19, 2023 21:20
@KlemenDEV KlemenDEV added the community review Used by maintainers to mark issues that should get community reviews, tests, and feedback label Sep 20, 2023
@KlemenDEV
Copy link
Contributor

I was about to merge this PR but then realized it would be much better for the category to define its parent instead of nested categories.

This way a plugin could eg. add sub-category to existing category, eg. special category of "Block data" blocks.

All this needs is when assembling category, you would need to iterate through all categories and if any declares current one as parent, add XML of that category as "sub-category"

@KlemenDEV KlemenDEV marked this pull request as draft September 21, 2023 19:14
@Goldorion Goldorion marked this pull request as ready for review September 22, 2023 14:29
@KlemenDEV
Copy link
Contributor

Toolbox should only have one parent category.

In the previous system, toolbox could only have one parent category so this should stay the same as it was ok and logical

@KlemenDEV KlemenDEV marked this pull request as draft September 22, 2023 14:33
@Goldorion Goldorion marked this pull request as ready for review September 22, 2023 14:45
@Goldorion Goldorion marked this pull request as draft September 22, 2023 14:45
@Goldorion
Copy link
Contributor Author

Goldorion commented Sep 22, 2023

Toolbox should only have one parent category.

In the previous system, toolbox could only have one parent category so this should stay the same as it was ok and logical

No, they could already have multiple parents. The system is the same as before, except you define the parents inside the nested category instead of defining the children inside the parent categories. The example I pushed simply didn't show this case.

@Goldorion Goldorion marked this pull request as ready for review September 22, 2023 14:46
@KlemenDEV
Copy link
Contributor

You are correct, there could be multiple parents before too. But I don't think this is a good idea as it would allow cluttered toolboxes and also in most (all sensible) cases one would want just one, where JSON will require array

@Goldorion
Copy link
Contributor Author

You are correct, there could be multiple parents before too. But I don't think this is a good idea as it would allow cluttered toolboxes and also in most (all sensible) cases one would want just one, where JSON will require array

Ok, I'll change it. I made a list, so users wanting to put their categories in multiple parents would have been able to.

@KlemenDEV
Copy link
Contributor

I understand your reasoning for the decision of supporting multiple parents, as this offers more options.

What I have concern with multiple parents for one category is whether that would bring any real benefits or just allow to "dig" same category under two parents, which is a bit unnatural for usual "tree" structures which the toolbox aims to be

@Goldorion
Copy link
Contributor Author

It's now changed to a single parent only

@KlemenDEV KlemenDEV merged commit da92c5f into MCreator:master Sep 22, 2023
2 checks passed
@Goldorion Goldorion deleted the nested-blockly-categories branch September 22, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community review Used by maintainers to mark issues that should get community reviews, tests, and feedback new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants