X Tutup
The Wayback Machine - https://web.archive.org/web/20201220223705/https://github.com/paceholder/nodeeditor/pull/172
Skip to content

/ nodeeditor

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

Jump to bottom

Rework styling #172

Open
wants to merge 12 commits into
base: master
from
Open

Rework styling #172

wants to merge 12 commits into from

Conversation

@Quincunx271
Copy link
Contributor

@Quincunx271 Quincunx271 commented May 11, 2018

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 added 10 commits Apr 30, 2018
@Quincunx271
Add DI for FlowViewStyle
818b56a
@Quincunx271
DI styling
2bb3ff0
@Quincunx271
Fix bug in FlowViewStyle setting
8ad2d3a
@Quincunx271
Apply std::move where appropriate
da1f6c0
@Quincunx271
Most of the way there
77c4724
@Quincunx271
Finish removing styling singletons
397d98a
@Quincunx271
Merge branch 'master' into di-styling
a5c2430
@Quincunx271
small misc improvements
f88d00b
@Quincunx271
Use FooStyle objects directly; improve implementation of FooStyle obj…
Loading status checks…
008eea5 
…ects
@Quincunx271
Add missing using declarations
Loading status checks…
9f09fad
@paceholder
Copy link
Owner

@paceholder paceholder commented May 14, 2018

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

I'll look through this PR

Dmitry
@paceholder
paceholder reviewed May 14, 2018
View changes
include/nodes/internal/ConnectionStyle.hpp
void setNormalColor(QColor);
void setSelectedColor(QColor);
void setSelectedHaloColor(QColor);
void setHoveredColor(QColor);

This comment has been minimized.

Sign in to view
@paceholder

paceholder May 14, 2018
Owner

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.

This comment has been minimized.

Sign in to view
@Quincunx271

Quincunx271 May 14, 2018
Author Contributor

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
paceholder reviewed May 14, 2018
View changes
include/nodes/internal/Connection.hpp Outdated
ConnectionState _connectionState;
ConnectionGeometry _connectionGeometry;

ConnectionStyle const *_connectionStyle;

This comment has been minimized.

Sign in to view
@paceholder

paceholder May 14, 2018
Owner

Sorry, maybe a stupid question -- why pointer?

This comment has been minimized.

Sign in to view
@Quincunx271

Quincunx271 May 14, 2018
Author Contributor

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.

This comment has been minimized.

Sign in to view
@paceholder

paceholder May 14, 2018
Owner

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

This comment has been minimized.

Sign in to view
@Quincunx271

Quincunx271 May 14, 2018
Author Contributor

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

This comment has been minimized.

Sign in to view
@paceholder

paceholder May 14, 2018
Owner

  1. I did not see any setter function
  2. 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
paceholder reviewed May 14, 2018
View changes
include/nodes/internal/FlowViewStyle.hpp Outdated
#include <QtGui/QColor>

#include <QString>
#include <QByteArray>

This comment has been minimized.

Sign in to view
@paceholder

paceholder May 14, 2018
Owner

Too much pedantry but I write full paths for Qt includes: #include <QtCore/QString>
@paceholder
paceholder reviewed May 14, 2018
View changes
src/ConnectionStyle.cpp
ConnectionStyle::
setConstructionColor(QColor color)
{
_constructionColor = std::move(color);

This comment has been minimized.

Sign in to view
@paceholder

paceholder May 14, 2018
Owner

It is copied either way

This comment has been minimized.

Sign in to view
@Quincunx271

Quincunx271 May 14, 2018
Author Contributor

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

This comment has been minimized.

Sign in to view
@paceholder

paceholder May 14, 2018
Owner

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

This comment has been minimized.

Sign in to view
@Quincunx271

Quincunx271 May 14, 2018
Author Contributor

Sounds good
@paceholder
Copy link
Owner

@paceholder paceholder commented May 14, 2018

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
Copy link
Contributor Author

@Quincunx271 Quincunx271 commented May 14, 2018

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
paceholder reviewed May 14, 2018
View changes
src/Node.cpp Outdated
computeStyle(NodeStyle const *preferredStyle, NodeStyle const &backupStyle)
{
return preferredStyle ? preferredStyle : &backupStyle;
}

This comment has been minimized.

Sign in to view
@paceholder

paceholder May 14, 2018
Owner

Oh, I see it now, thanks.
@paceholder
paceholder reviewed May 14, 2018
View changes
src/Node.cpp
: _uid(QUuid::createUuid())
, _nodeDataModel(std::move(dataModel))
, _nodeState(_nodeDataModel)
, _nodeGeometry(_nodeDataModel)
, _nodeStyle(computeStyle(_nodeDataModel->nodeStyle(), style))

This comment has been minimized.

Sign in to view
@paceholder

paceholder May 14, 2018
Owner

And this is maybe a reason to have a pointer.

This comment has been minimized.

Sign in to view
@Quincunx271

Quincunx271 May 14, 2018
Author Contributor

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
Copy link
Owner

@paceholder paceholder commented May 14, 2018

Should I bump the library version to 3?
@Quincunx271
Copy link
Contributor Author

@Quincunx271 Quincunx271 commented May 14, 2018

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
Address concerns
Loading status checks…
8cb84ce
@Quincunx271
Copy link
Contributor Author

@Quincunx271 Quincunx271 commented May 14, 2018 •
edited

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
add missing #include <QtCore/QHash>
Loading status checks…
468101e
@loganmcbroom
Copy link

@loganmcbroom loganmcbroom commented Feb 11, 2020

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paceholder paceholder

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@Quincunx271 @paceholder @loganmcbroom
You can’t perform that action at this time.
X Tutup