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.

Sign up for GitHub

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

Jump to bottom

hsMessageBox Cleanups #1466

Merged
merged 4 commits into from
Aug 29, 2023
Merged

hsMessageBox Cleanups #1466

merged 4 commits into from
Aug 29, 2023

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Aug 15, 2023 •
edited
Loading

  • Moves hsMessageBox implementation out of CoreLib and into PubUtilLib
  • Splits the Win32-specific implementation into its own file (matching how macOS is handled)
  • Adds overloads for hsMessageBox that take ST::string
  • Silences (on gcc & clang) deprecation warnings from OpenSSL about RC4

I'm unsure on whether it makes sense to change the hsMessageBox stuff to always convert to ST::string and then have a single implementation, or if it's more performant having it split up as it currently is...

@dpogue dpogue marked this pull request as ready for review August 15, 2023 05:46
dgelessus
dgelessus suggested changes Aug 15, 2023
View reviewed changes
Sources/Plasma/CoreLib/HeadSpin_Mac.mm Outdated Show resolved Hide resolved
Sources/Plasma/CoreLib/HeadSpin.cpp Outdated Show resolved Hide resolved
Sources/Plasma/CoreLib/HeadSpin.h Outdated Show resolved Hide resolved
@dgelessus
Copy link
Contributor

Not something that needs to be done in this PR, but I think it would make sense to pull hsMessageBox out into its own header. Possibly even into another library, somewhere under PubUtilLib. If my search is correct, hsMessageBox isn't called anywhere in CoreLib or NucleusLib, so it would be great if we could build those libraries without depending on GUI libraries for no reason.

@dpogue
Copy link
Member Author

dpogue commented Aug 15, 2023

I think it would make sense to pull hsMessageBox out into its own header. Possibly even into another library, somewhere under PubUtilLib.

I like this idea

@dpogue dpogue force-pushed the cleanups branch 4 times, most recently from 74d5332 to 9d91abf Compare August 16, 2023 04:37
dgelessus
dgelessus suggested changes Aug 16, 2023
View reviewed changes
Copy link
Contributor

@dgelessus dgelessus left a comment

Choose a reason for hiding this comment

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

Otherwise this looks good I think.

Sources/Plasma/PubUtilLib/plMessageBox/hsMessageBox.h Outdated Show resolved Hide resolved
Sources/Plasma/PubUtilLib/plMessageBox/hsMessageBox_Mac.mm Outdated Show resolved Hide resolved
@Hoikas Hoikas merged commit 6c15dab into H-uru:master Aug 29, 2023
14 checks passed
@dpogue dpogue deleted the cleanups branch September 8, 2023 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zrax zrax zrax left review comments

@dgelessus dgelessus dgelessus approved these changes

@Hoikas Hoikas Hoikas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dpogue @dgelessus @zrax @Hoikas