Skip to content
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

Enable app class independent usage #203

Merged
merged 6 commits into from
Jan 24, 2025
Merged

Conversation

bebuch
Copy link
Contributor

@bebuch bebuch commented Jan 16, 2025

This is a pure extension and fully backwards compatible. (Unless I made a mistake. 😉)

I have two requirements for the library that are not currently possible:

  1. I want to use it as a pre-compiled library while still being able to choose my QXxxApplication class at build time.
  2. I need to be able to decide at runtime whether to use the single instance stuff (without starting a server and so on).

This PR allows both by making the SingleApplication class an object that is not derived from QXxxApplication. On the user side, you need to instantiate a separate SingleApplication object after your QXxxApplication object. I've added a new example separate_object which mirrors sending_arguments but uses SingleApplication as a separate object.

Of course, the better approach would be to put the implementation into a separate class with an appropriate name and then use that to implement SingleApplication as it currently is. I would be delighted if this were implemented for 4.x. Then you could also use the functionality with installed CMake Config and not just as a Git submodule.

If you are willing to merge this, I would also create a PR with CMake install for CMake Config files in “FreeStanding” mode.

If you don't want to merge this, but are willing to merge the “better approach” mentioned, then I can also implement a corresponding PR for 3.x.

@itay-grudev What do you think?

@itay-grudev
Copy link
Owner

I get it. It looks OK as a concept. Isn't there some extra work required for example calls like this one won't work when it doesn't inherit from QCoreApplication:

::exit( EXIT_SUCCESS );

@itay-grudev
Copy link
Owner

itay-grudev commented Jan 16, 2025

If you finish it and it is entirely backwards compatible I don't mind merging it as a v3.6 as I don't currently have the time to finish the v4 implementation that doesn't use QSharedMemory.

@bebuch
Copy link
Contributor Author

bebuch commented Jan 17, 2025

Thanks for the fast review, sounds good!

I get it. It looks OK as a concept. Isn't there some extra work required for example calls like this one won't work when it doesn't inherit from QCoreApplication:

::exit( EXIT_SUCCESS );

If I understand this correctly you call the C standard library funktion ::exit here. According to Stackoverflow, this is equivalent to the C++ std::exit pendant.

To my knowledge, it makes no difference whether I call this from a QCoreApplication class or from somewhere else. Am I wrong there? 🤔

@bebuch bebuch force-pushed the master branch 2 times, most recently from 2b9c101 to 09e4580 Compare January 17, 2025 13:48
@bebuch
Copy link
Contributor Author

bebuch commented Jan 17, 2025

I've fixed the bug with the preprocessor string compartion. Looks like this is not possible. Instead I've added a new file FreeStandingSingleApplication that works as the fake QXxxApplication base class.

In the new separate_object example I've renamed the executable to separate_object.

I've successfully tested all 5 examples locally on Windows with clang. I'm going to test with MSVC and on Linux with GCC as well.

Please run the CI pipelines to validate that my changes didn't break anything.

I'm going to add a new CI pipeline step for the new example and add documentation on how to use the free standing mode.

@bebuch
Copy link
Contributor Author

bebuch commented Jan 17, 2025

I added CI steps for the new separate_object example with QMake and CMake.

I added a CMake install in free standing mode that generates CMake config files and removes the QAPPLICATION_CLASS macro from the installed header file. I added user doc to readme and notes to changelog.

A notable change is, that I added the library version (3.6.0!) to the CMakeLists.txt statement project! Thats required to generate Config files that can test version compatability.

From my side this is ready for merge now. Let me know I you want changes or more explenation 😉

@bebuch
Copy link
Contributor Author

bebuch commented Jan 17, 2025

ld: warning: ignoring file 'QtCore.framework/Versions/5/QtCore': found architecture 'x86_64', required architecture 'arm64'
ld: warning: ignoring file 'QtNetwork.framework/Versions/5/QtNetwork': found architecture 'x86_64', required architecture 'arm64'

This looks like a broken build environment to me. 🤔

@bebuch
Copy link
Contributor Author

bebuch commented Jan 17, 2025

I've changed the MacOS plattform from latest to 11. I don't know if this works, its just a try. This version did run on x86_64 instead of arm64.

@bebuch
Copy link
Contributor Author

bebuch commented Jan 17, 2025

Okay, from what I've found MacOS runners are now arm64 only. X86_64 is no longer supported with macos-latest. According to an fmt issue macos-11 is also no longer supported.

https://github.com/jurplel/install-qt-action

If I understand the action file right, than this is the repo responsible for installing Qt. In this case it's probably a bug on their side with the Qt5 install on current (arm64) MacOS.

Maybe it's the best to temporarily disable MacOS in the CI. What do you think?

@bebuch
Copy link
Contributor Author

bebuch commented Jan 18, 2025

I've made a new PR #204 to fix the MacOS CI build. If that succeeds and got merged, I will rebase this PR and we can continue 😅

@bebuch
Copy link
Contributor Author

bebuch commented Jan 20, 2025

I rebased this on my CI-fix PR #204 and the CI succeeds now.

https://github.com/bebuch/SingleApplication/actions/runs/12872062233/

This is ready for merge from my side. (If you accept #204 as well.)

@itay-grudev itay-grudev merged commit 0ba7b6c into itay-grudev:master Jan 24, 2025
9 checks passed
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