-
Notifications
You must be signed in to change notification settings - Fork 901
Add qml #449
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
base: master
Are you sure you want to change the base?
Add qml #449
Conversation
|
||
project(QtNodesLibrary CXX) | ||
|
||
set(CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake" ${CMAKE_MODULE_PATH}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMAKE_MODULE_PATH is a list. Better use list(APPEND
. See https://stackoverflow.com/questions/52730397/how-can-i-set-cmake-module-path-for-doing-regular-and-out-of-source-builds-in-cm
) | ||
endif() | ||
|
||
if(QT_NODES_DEVELOPER_DEFAULTS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is not defined anywhere.
|
||
add_executable(calculator | ||
${CALC_SOURCE_FILES} | ||
${CALC_HEAEDR_FILES} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Miss spelling
CMakeLists.txt
Outdated
endif() | ||
|
||
find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS Core Widgets Gui OpenGL) | ||
find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS Core Widgets Gui OpenGL Quick) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QML needs Quick
@@ -0,0 +1,211 @@ | |||
#include "QmlWrapper.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapper stolen from musescore project
src/DataFlowGraphModel.cpp
Outdated
onOutPortDataUpdated(restoredNodeId, portIndex); | ||
}); | ||
|
||
model->load(internalDataJson); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember why but QtNodes crashes with _models[restoredNodeId]->load(internalDataJson);
.
set(ACQ_HEADER_FILES AcqData.hpp AcqModel.hpp) | ||
|
||
add_executable(acquisition_viewer ${ACQ_SOURCE_FILES} ${ACQ_HEADER_FILES} | ||
CMakeLists.txt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a good idea to add WINDEPLOYQT_EXECUTABLE for all examples ?
centerOn(sceneRect.center()); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve qml widget
cmake/QtNodesConfig.cmake.in
Outdated
@@ -1,4 +1,4 @@ | |||
get_filename_component(QtNodes_CMAKE_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH) | |||
@PACKAGE_INIT@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Config.cmake.in pattern from CMake documentation
examples/calculator_qml/main.qml
Outdated
height: 1000 | ||
visible: true | ||
|
||
ChartView { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace ChartView by Button. This component is only to avoid having WidgetView at position 0, 0.
@bansan85 This PR is too big for anyone to really vet it properly. There are a bunch of different strategies one could use to manage a change set this large. Stacked PRs is one that comes to mind (partially because they're sort of a trendy thing at the moment) Beyond that, I see a bunch of commits that don't seem to have anything to do with implementing QML support—various bug fixes, applying clang-format (on some lines that probably shouldn't have been touched to begin with), cmake packaging, deployment, silencing warnings, etc. All of these could actually be useful changes, but they don't belong together at all. If you'd like to see these changes merged, I'd be happy to walk through which pieces can be pulled out into their own smaller PRs. However, it looks like you may no longer be working on nodeeditor (See #429 (comment)). If that is the case, that is totally fine too; just please close the PR. |
You're right. Sometimes I think about this commit and know I should definitely clean it up... Vacation is coming. Please give me until September to remove everything not related to QML. Remember, zooming with the mouse is buggy, but this commit is a good start. |
All good! You can take as much time as you need/want. I only meant to check whether there was still any interest on this ticket. Let me know if you'd like any guidance on where/how to split these up. 😄 |
a17a33a
to
601a23c
Compare
Add calculator_test to test a QML form that integrate node editor
Branch has been cleaned. Feedback is welcome. You can read main.cpp from calculator_qml example to understand how to use Node Editor with qml. |
As I said in #429 it's hard to split every feature because I made (small) modification in lots of files so it's hard to have multiple PR that to conflict each others.
I think that it's better to convince you that every commit is fine :)
If you don't like some commits, I will remove them.
I wrote comments on almost every commit to add more context.
If you are fine with some commit, I can create another PR with these commits to reduce the size of the current PR.
I know this PR is huge :/ Don't hesitate to send me feedback or if you want I make some changes in this PR.