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

Keep registration order as order for context menu #163

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Quincunx271
Copy link
Contributor

@Quincunx271 Quincunx271 commented Apr 30, 2018

Implements #147

I kept _categories and _registeredModelsCategory as-is. Someone could've called those functions and stored the variables, and removing them, but especially replacing it with a new data-type, would break their code (replacing could silently break in some cases). My ideal would've been to just change the categories() function to return the std::vector and to change the registeredModelsCategoryAssociation to return a separate map type that preserves insertion order (similar to LinkedHashMap in Java; I would've just combined std::vector and std::unordered_map).

If removing / changing _categories or _registeredModelsCategory is desired, I'll make the change and update this PR.

Note: if we do remove categories(), such that _categories becomes inaccessible through any external functions, it'd be best to change its type from std::set to std::unordered_set

@@ -57,11 +60,16 @@ class NODE_EDITOR_PUBLIC DataModelRegistry

QString const name = uniqueModel->name();

if (_registeredModels.count(name) == 0)
if (!_registeredModels.count(name))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You might find this less readable, but I've seen people treating the count method on std::map or std::set as "contains", which is why I wrote is as !_registeredModels.count(name) here. It might not be desirable, if so, it's easy enough to change this back

@Quincunx271
Copy link
Contributor Author

examples/calculator

This is the registration code (edited to make this comment smaller):

  auto ret = std::make_shared<DataModelRegistry>();

  ret->registerModel<NumberSourceDataModel>("Sources");
  ret->registerModel<NumberDisplayDataModel>("Displays");
  ret->registerModel<AdditionModel>("Operators");
  ret->registerModel<SubtractionModel>("Operators");
  ret->registerModel<MultiplicationModel>("Operators");
  ret->registerModel<DivisionModel>("Operators");
  ret->registerModel<ModuloModel>("Operators");

Before this PR:

image

After this PR:

image

The categories can still be sorted after this PR by calling ret->sortCategories()

@charlietsao
Copy link

Just found that version 2.1.3 still not support manually context menu settings.

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.

None yet

2 participants