Skip to content

Conversation

@firewave
Copy link
Collaborator

@firewave firewave commented Sep 18, 2025

@firewave firewave marked this pull request as draft September 18, 2025 12:43
@firewave firewave force-pushed the qt-wrap-ui branch 2 times, most recently from a38d9e1 to fca9982 Compare September 23, 2025 12:37
@sonarqubecloud
Copy link

@firewave firewave mentioned this pull request Sep 23, 2025
Copy link
Contributor

@pfultz2 pfultz2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No cmake changes until #7658 is merged first.

@firewave
Copy link
Collaborator Author

We did things wrong and doing it right somehow doesn't really work although it should. The Qt documentation is unfortunately not helpful on the surface because you are supposed to use helpers but those appear to mess with the global project state which is not what we want.

So cleaning that up first could help to find potential issues with your changes.

@pfultz2
Copy link
Contributor

pfultz2 commented Sep 26, 2025

We did things wrong and doing it right somehow doesn't really work although it should.

Sure, but #7658 doesn't change that.

So cleaning that up first could help to find potential issues with your changes.

#7658 works fine, you are just creating more work for me for no reason. Those changes should be merged in already, its been almost 3 months now. For now, we should hold off on any cmake changes until #7658 is merged in.

@firewave
Copy link
Collaborator Author

Sure, but #7658 doesn't change that.

That is not clear because I haven't gotten this to work yet and did not try your changes on top.

#7658 works fine, you are just creating more work for me for no reason. Those changes should be merged in already, its been almost 3 months now. For now, we should hold off on any cmake changes until #7658 is merged in.

Just because it builds doesn't mean that it is fine. Especially if you make those changes on top of something which is is improperly done or tested.

And it has not been long. I have sometimes been waiting longer than that just to get a single comment. (That is actually not something to brag about - it's just horrible).

Also what was the point of those changes? There was a small issue which was easily fixed and somehow it triggered you to do these unnecessary changes which will fix no known issues.

And your CMake refactoring is actually creating more work for me. This modern (which is often not equal to "better") CMake (C++, Python etc.) is great to easily on-board people which have no idea what they are doing by providing puzzle parts they only need to arrange. If makes things cleaner and by the obfuscation it introduces it makes things actually much harder to understand if you have to change something internally. It is much better for a user of it but not for a developer.

Qt did this a few versions back and now they have some big init function you are supposed to call and sets all things up in the project but does not consider not all that the Qt application might only be a small part of a bigger project. And on top of that is basically no documentation on what it actually does because you don't have to know anymore but it does everything for you.

So if something breaks and I have no idea to do it with the modern stuff and cannot be properly in the modern way, it would be up to you. So you basically just took "ownership" of the CMake maintenance with your cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants