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

replace CONFIG_DARWIN with Q_OS_DARWIN; update deprecated Q_OS_MAC and Q_OS_MACX #442

Closed
wants to merge 11 commits into from

Conversation

ulmus-scott
Copy link
Contributor

Was part of #430

Added a few new commits regarding linux vs __linux__

@ulmus-scott ulmus-scott mentioned this pull request Dec 30, 2021
1 task
@ulmus-scott
Copy link
Contributor Author

Some of the macOS/Darwin conditional code probably needs to be tested to see if it is still needed, e.g.

#ifdef Q_OS_MAC
// Qt 4.4 has window-focus problems
gCoreContext->OverrideSettingForSession("RunFrontendInWindow", "1");
#endif

@ulmus-scott
Copy link
Contributor Author

Ignore 4c56a8b
Its not supposed to be in this pull request.

@linuxdude42
Copy link
Contributor

I like all the bits of this pull request, but it is a mashup of several unrelated things. Please refocus it specifically on the CONFIG_DARWIN-> Q_OS_DARWIN and similar directive changes like the linux->linux and "if x" to "if defined(x)".

The change for reordering compat.h should be separate pull requests so it can have a proper discussion. I think it looks good, but I'd like to consider it separately from all the preprocessor directive changes.

In the future, changes like cleaning up doxygen, removing XRANDR, removing mythio.h, deleting unused code in mythmusic, and removing the math checks from configure should all be separate pull requests. I cherry-picked them this time, and am currently running them through my build system. If there aren't any problems I'll push them in an hour or so.

The change to httprequest.cpp should be enhanced to remove all references to USE_SETSOCKOPT and submitted as a separate pull. That code was all commented out by 9846f8f in 2014 and can be removed.

Please don't gratuitously reorder the include file statements in the code.

@ulmus-scott ulmus-scott mentioned this pull request Jan 2, 2022
@ulmus-scott
Copy link
Contributor Author

I like all the bits of this pull request, but it is a mashup of several unrelated things. Please refocus it specifically on the CONFIG_DARWIN-> Q_OS_DARWIN and similar directive changes like the linux->linux and "if x" to "if defined(x)".

In the future, changes like cleaning up doxygen, removing XRANDR, removing mythio.h, deleting unused code in mythmusic, and removing the math checks from configure should all be separate pull requests. I cherry-picked them this time, and am currently running them through my build system. If there aren't any problems I'll push them in an hour or so.

My goal was to reduce use of mythconfig.h. I was cleaning up the code as I saw it. I agree the pull requests could have been more focused.

The change to httprequest.cpp should be enhanced to remove all references to USE_SETSOCKOPT and submitted as a separate pull. That code was all commented out by 9846f8f in 2014 and can be removed.

I'll look into it.

The change for reordering compat.h should be separate pull requests so it can have a proper discussion. I think it looks good, but I'd like to consider it separately from all the preprocessor directive changes.

See #447

Please don't gratuitously reorder the include file statements in the code.

Reordering the includes does serve a purpose; however, I agree they should probably be in a separate commit, maybe even a separate pull request.

@ulmus-scott ulmus-scott changed the title Update Q_OS defines, rearrange compat.h replace CONFIG_DARWIN with Q_OS_DARWIN; update depcrecated Q_OS_MAC and Q_OS_MACX Jan 2, 2022
@ulmus-scott ulmus-scott changed the title replace CONFIG_DARWIN with Q_OS_DARWIN; update depcrecated Q_OS_MAC and Q_OS_MACX replace CONFIG_DARWIN with Q_OS_DARWIN; update deprecated Q_OS_MAC and Q_OS_MACX Jan 2, 2022
@linuxdude42
Copy link
Contributor

I just pushed a version of this without the include file reordering.

@linuxdude42 linuxdude42 closed this Jan 5, 2022
@ulmus-scott ulmus-scott deleted the defines branch January 7, 2022 19:58
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.

None yet

2 participants