nodeeditor icon indicating copy to clipboard operation
nodeeditor copied to clipboard

Rework styling

Open Quincunx271 opened this issue 7 years ago • 7 comments

Addresses #149

This PR makes a rather large amount of changes, as the singletons were used in many places.

Changes:

  • Convert styling singletons into dependency injected styles. FlowViewStyle becomes a property of FlowView, and ConnectionStyle and NodeStyle (the default one, at least) become a property of FlowScene.
    • For FlowView, the style can be passed as a parameter to the constructor, or via calling myFlowView->setStyle(...).
    • For FlowScene, I chose not to allow the styles to be passed as parameters to the constructor. I am not sure of this decision. The problem was that I couldn't think of any reason to have the styles passed in one order or the other. Styles can still be set via scene->setConnectionStyle(...) and scene->setNodeStyle(...)
  • The *Styles are reworked.
    • Style settings can be set as properties of the FooStyle object, rather than solely through parsing JSON
    • Implementation: no more macros for parsing. Instead, utility functions from a new StyleImport class are used.
  • NodeDataModel::setStyle is removed. NodeDataModel is not in charge of managing the style, but Node is. To use a custom style, one would override the new virtual NodeStyle const* nodeStyle() const; function.
    • One thought I had here was that maybe the Creator objects could somehow be in charge of managing this. Currently the Creators are std::functions, but if we made them our own NodeModelCreator, there could be a way to set the style on the creator.

Concerns:

  • I don't know what would happen if you change the style in the middle of execution. I believe that some things will update to use the new style, but some things won't. I think that the right path would be to always allow style changes at any time, but the system isn't set up to handle it.

Quincunx271 avatar May 11 '18 23:05 Quincunx271

I am very curious about what are you implementing with the Node Editor :-)

I'll look through this PR

Dmitry

paceholder avatar May 14 '18 20:05 paceholder

Maybe I am missing something. Before, it was possible to define a style per NodeDataModel. Each model could be effectively styled differently. Even two identical models could be styled differently based on some inherent model data.

Can we repeat the same trick now? There were some users that needed different looks for different models.

paceholder avatar May 14 '18 21:05 paceholder

I removed NodeDataModel::setStyle. I can't remember the exact reason (I'm currently working on a different branch), but it had something to do with that it was easier to implement and made more sense to me for the NodeDataModel to not be in charge of managing the style.

Instead, users that want to customize the style per model (or internal model data) should implement virtual NodeStyle const* nodeStyle() const; to return the appropriate style.

Quincunx271 avatar May 14 '18 21:05 Quincunx271

Should I bump the library version to 3?

paceholder avatar May 14 '18 21:05 paceholder

This would indeed be a breaking change.

However, it might be beneficial to instead create something like a version-3 branch and merge this into that (once I fix the changes), so that we can implement multiple breaking-change things together (rather than having to bump the major version each time, we could group several changes into some milestone which we call a new major version)

Quincunx271 avatar May 14 '18 22:05 Quincunx271

With this commit, note that I did not address the Q_PROPERTY for style objects. I'm not sure which way to go with it.

Quincunx271 avatar May 14 '18 22:05 Quincunx271

I haven't looked through the full usage of styles to see if this breaks something, but if these changes are being saved for a future breaking update, could the const be dropped from the StyleCollection getters in the meantime? In my local branch I dropped the consts and forwarded the getters through the NodeStyle/etc. classes. Things seem to be working normally and style properties can be set individually without using json strings. If I am understanding the current version, you can only set styles via json strings? This seems very awkward to me.

loganmcbroom avatar Feb 11 '20 01:02 loganmcbroom