Rework styling by Quincunx271 · Pull Request #172 · paceholder/nodeeditor

Quincunx271 · 2018-05-11T23:05:36Z

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.

…ects

paceholder · 2018-05-14T20:06:02Z

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

I'll look through this PR

Dmitry

paceholder · 2018-05-14T20:18:19Z

include/nodes/internal/ConnectionStyle.hpp

+  void setNormalColor(QColor);
+  void setSelectedColor(QColor);
+  void setSelectedHaloColor(QColor);
+  void setHoveredColor(QColor);


Should we go further with the Qt-kung-fu and describe all the members in the class with Q_PROPERTY ?

As I understand, something like this

Q_PROPERTY(QColor color MEMBER m_color NOTIFY colorChanged)

would define a private member and generate setters and getters.

Not a bad idea. E.g. when we implement a QML frontend, that could come in handy. IIUC, Q_PROPERTY only has meaning inside something that inherits from QObject, which the FooStyles currently don't. QObjects can't be copied, so there are some options:

Pass around std::unique_ptr<FooStyle> instead of FooStyle directly.

Make FooStyle take a QObject* parent = Q_NULLPTR in its constructor.

Maybe this could work?: Write explicit copy constructors for FooStyles

paceholder · 2018-05-14T20:18:59Z

include/nodes/internal/Connection.hpp

  ConnectionState    _connectionState;
  ConnectionGeometry _connectionGeometry;

+  ConnectionStyle const *_connectionStyle;


Sorry, maybe a stupid question -- why pointer?

What other options were you thinking of?

ConnectionStyle _connectionStyle; This is expensive. We don't need a copy of the ConnectionStyle object for each Connection, especially when they're all going to be the same

std::shared_ptr<ConnectionStyle const> _connectionStyle; This is unnecessary. The ConnectionStyle outlives the connections anyway (correction: they can, but currently don't; that's something I will fix).

ConnectionStyle const &_connectionStyle; I don't use references as class members, as references can't be rebound (they are effectively const). It doesn't matter here, as Connection can't be copied anyway, but I used a pointer instead for consistency.

Here ConnectionStyle const & connectionStyle() const we convert this pointer back to a reference.

True. The member is stored as a pointer, but is semantically a reference (that's the style I use anyway).

I did not see any setter function

The copy constructor for Connection is deleted.

Therefore I suggested that this member shouldn't be rebound.

I'd simply use a const ref.

BTW, we should explicitly delete move constructor and move assignment operator for this class

paceholder · 2018-05-14T20:22:49Z

include/nodes/internal/FlowViewStyle.hpp

 #include <QtGui/QColor>

+#include <QString>
+#include <QByteArray>


Too much pedantry but I write full paths for Qt includes: #include <QtCore/QString>

paceholder · 2018-05-14T20:31:13Z

src/ConnectionStyle.cpp

+ConnectionStyle::
+setConstructionColor(QColor color)
+{
+  _constructionColor = std::move(color);


It is copied either way

Ah, I couldn't tell from the documentation.

TBH, I tend to use std::move on anything that isn't a primitive (int, long, etc). I forget whether it has a benefit for a lot of types, so just using std::move means I don't have to go look up the documentation.

I'll remove these std::moves

Sorry, I am not against this moves, it was just a note. No worries.

Sounds good

paceholder · 2018-05-14T21:00:56Z

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.

Quincunx271 · 2018-05-14T21:11:59Z

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.

paceholder · 2018-05-14T21:31:50Z

src/Node.cpp

+computeStyle(NodeStyle const *preferredStyle, NodeStyle const &backupStyle)
+{
+  return preferredStyle ? preferredStyle : &backupStyle;
+}


Oh, I see it now, thanks.

paceholder · 2018-05-14T21:32:11Z

src/Node.cpp

  , _nodeDataModel(std::move(dataModel))
  , _nodeState(_nodeDataModel)
-  , _nodeGeometry(_nodeDataModel)
+  , _nodeStyle(computeStyle(_nodeDataModel->nodeStyle(), style))


And this is maybe a reason to have a pointer.

If computeStyle returns a NodeStyle const&, then _nodeStyle could also be a NodeStyle const&:

NodeStyle const& computeStyle(NodeStyle const *preferredStyle, NodeStyle const &backupStyle) { return preferredStyle ? *preferredStyle : backupStyle; }

I'll change the style members from being const * to const &, unless you say otherwise

paceholder · 2018-05-14T21:34:51Z

Should I bump the library version to 3?

Quincunx271 · 2018-05-14T22:01:15Z

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 · 2018-05-14T22:25:53Z

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.

loganmcbroom · 2020-02-11T01:30:49Z

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.

Quincunx271 added 10 commits May 2, 2018 12:39

Add DI for FlowViewStyle

818b56a

DI styling

2bb3ff0

Fix bug in FlowViewStyle setting

8ad2d3a

Apply std::move where appropriate

da1f6c0

Most of the way there

77c4724

Finish removing styling singletons

397d98a

Merge branch 'master' into di-styling

a5c2430

small misc improvements

f88d00b

Use FooStyle objects directly; improve implementation of FooStyle obj…

008eea5

…ects

Add missing using declarations

9f09fad

paceholder reviewed May 14, 2018

View reviewed changes

Address concerns

8cb84ce

add missing #include <QtCore/QHash>

468101e

Conversation

Quincunx271 commented May 11, 2018

Uh oh!

paceholder commented May 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paceholder commented May 14, 2018

Uh oh!

Quincunx271 commented May 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paceholder commented May 14, 2018

Uh oh!

Quincunx271 commented May 14, 2018

Uh oh!

Quincunx271 commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loganmcbroom commented Feb 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Quincunx271 commented May 14, 2018 •

edited

Loading