Feature/qt6 compatibility #1315
Conversation
…ature/qt6_compatibility
…ature/qt6_compatibility
| ; filter_incl = Topics whitelist as regular expression (will be monitored only) | ||
| ; filter_log_con = info, warning, error, fatal Log messages logged to console (all, info, warning, error, fatal, debug1, debug2, debug3, debug4) | ||
| ; filter_log_file = Log messages to logged into file system | ||
| ; filter_log_udp = info, warning, error, fatal Log messages logged via udp network | ||
| ; -------------------------------------------------- | ||
| [monitoring] | ||
| timeout = 5000 | ||
| filter_excl = __.* | ||
| filter_excl = ^__.*$ |
There was a problem hiding this comment.
@rex-schilasky Please leave a comment whether you think that this change can have bad side-effects
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
app/mon/mon_gui/src/widgets/ecalmon_tree_widget/topic_widget.cpp
Outdated
Show resolved
Hide resolved
app/mon/mon_gui/src/widgets/ecalmon_tree_widget/topic_widget.cpp
Outdated
Show resolved
Hide resolved
app/mon/mon_gui/src/widgets/ecalmon_tree_widget/topic_widget.cpp
Outdated
Show resolved
Hide resolved
| @@ -224,7 +226,19 @@ void GroupTreeModel::removeItemFromGroups(QAbstractTreeItem* item, bool remove_e | |||
|
|
|||
| if (item_to_remove->parentItem() == root()) | |||
| { | |||
| group_map_.remove(group_map_.key((GroupTreeItem*)item_to_remove)); | |||
| auto it = std::find_if(std::begin(group_map_), std::end(group_map_), | |||
| [item_to_remove](auto& p) { return p.second == (GroupTreeItem*)item_to_remove; }); | |||
There was a problem hiding this comment.
warning: do not use C-style cast to downcast from a base to a derived class; use dynamic_cast instead [cppcoreguidelines-pro-type-cstyle-cast]
| [item_to_remove](auto& p) { return p.second == (GroupTreeItem*)item_to_remove; }); | |
| [item_to_remove](auto& p) { return p.second == dynamic_cast<GroupTreeItem*>(item_to_remove); }); |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| @@ -217,9 +229,64 @@ target_include_directories(${PROJECT_NAME} PRIVATE src) | |||
| target_include_directories(${PROJECT_NAME} PRIVATE $<TARGET_PROPERTY:eCAL::core,INCLUDE_DIRECTORIES>) | |||
|
|
|||
|
|
|||
| if(WIN32) | |||
| if ((WIN32 OR APPLE) AND (${QT_VERSION_MAJOR} GREATER_EQUAL 6)) | |||
There was a problem hiding this comment.
We have this (or similar) code in many places. Could you please make cmake macros / functions for it?
There was a problem hiding this comment.
These are just 3 commands (qt_generate_deploy_app_script, add_custom_command, install). The rest are comments to explain what is going on. Do you think that it is worth to make a script for that, discover the script file, include the script and then call it?
I feel like that would cause more code and be more difficult to understand, because you would have to go to that script to see what is going on. And one would have to implement some kind of argument parsing, if you intend to keep it flexible. It would also cause additional coupling of all eCAL Qt Application to some custom script that is stored somewhere. I was excited to remove custom deploy scripts for Qt6, so I am not sure if encapsulating 3 commands justify creating one, again.
There was a problem hiding this comment.
But you have the same code in at least 4 places (mon_gui, play_gui, rec_gui, sys_gui, ...)
It's like arguing that you're not creating another header file and not using functions, because it's too complicated.
It's 60 lines of code, being (as far as I can see), exactly identical in at least mon_gui, play_gui, rec_gui.
If you want to keep it with the apps, put it in the app subdirectory, or define the macro in app/CMakeLists.txt.
But duplicated code is duplicated code and should be put in a function (or cmake macro, because that is in the end text replacement, and does not introduce new scopes which you may not want).
There was a problem hiding this comment.
You could also make it part of ecal_add_app_qt, if it's something to be done for all ecal qt apps.
There was a problem hiding this comment.
Hm ok, so your intention is to put the legacy Qt5 install(CODE ... in that script as well? I agree that that script could go in its own file, but the scope of this PR was to just add Qt6. Also the Qt5 script wasn't written by me and I honestly was afraid to touch it, so I just added the new commands for Qt6.
It's like arguing that you're not creating another header file and not using functions, because it's too complicated.
I didn't write it would be too complicated and (for Qt6) the CMakeLists.txt does use functions. But yea, it kind of is similar to copying code from one to the next header file versus creating a separate project for it that you then have to reference to. It's always a tradeoff between coupling and code duplication.
I felt like Qt6 already encapsulated everything very well and those official qt functions are probably better understandable than a custom one. Thus I didn't want to hide those 3 calls with the downside of more coupling.
You could also make it part of ecal_add_app_qt, if it's something to be done for all ecal qt apps.
Sure, could be a solution for the apps, as those are coupled to those functions anyways. It would get rid of the flexbility, but maybe we are ok with a preconfigured variant of the scripts.
We would also maybe need a preconfigured variant for samples, monitor plugins and libraries.
Description
Made eCAL Compatible with Qt6
HAS_QT, but the legacyHAS_QT5option is still supportedecal.ini, so it doesn't need to be "fixed" in C++ code anymore (Added^and$to match entire string)Related issues
Cherry-pick to