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

Jump to bottom

NoGUI: Create MacOS platform #11527

Merged
merged 1 commit into from Jun 2, 2023
Merged

NoGUI: Create MacOS platform #11527

merged 1 commit into from Jun 2, 2023
+451 −0

Conversation

Hibyehello
Copy link
Contributor

@Hibyehello Hibyehello commented Feb 2, 2023 •
edited

Previously dolphin no-gui on MacOS only had the headless option, this adds another platform that uses Appkit.

I still need to add all of the keydown event's that the other platforms have. This is all complete as well

Just wanted to get feedback while I add the events. Events are now added

@Hibyehello Hibyehello marked this pull request as ready for review February 2, 2023 05:25
@phire
Copy link
Member

phire commented Feb 2, 2023

I don't really know how to review the ObjectiveC code, but the rest looks fine.

TellowKrinkle
TellowKrinkle reviewed Feb 2, 2023
View reviewed changes
Copy link
Contributor

@TellowKrinkle TellowKrinkle left a comment

Choose a reason for hiding this comment

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

BTW instead of manually handling keyboard shortcuts, if you make a menubar that includes the keyboard shortcuts you want to include, macOS should handle them for you and users can then override those with their own in System Preferences.

Source/Core/DolphinNoGUI/CMakeLists.txt Outdated Show resolved Hide resolved
Source/Core/DolphinNoGUI/MainNoGUI.cpp
#endif
#ifdef __APPLE__
,
"macos"
Copy link
Contributor

Choose a reason for hiding this comment

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

C is okay with trailing commas here, and I think it would look much better if we didn't have single lines with nothing but a comma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't exactly sure why we were doing it that way, I was just following the style

Source/Core/DolphinNoGUI/PlatformMacos.mm Outdated Show resolved Hide resolved
Source/Core/DolphinNoGUI/PlatformMacos.mm Outdated Show resolved Hide resolved
Source/Core/DolphinNoGUI/PlatformMacos.mm Outdated Show resolved Hide resolved
Source/Core/DolphinNoGUI/PlatformMacos.mm Outdated Show resolved Hide resolved
Source/Core/DolphinNoGUI/PlatformMacos.mm Outdated Show resolved Hide resolved
Source/Core/DolphinNoGUI/PlatformMacos.mm Outdated Show resolved Hide resolved
Source/Core/DolphinNoGUI/PlatformMacos.mm Outdated Show resolved Hide resolved
@Hibyehello
Copy link
Contributor Author

Hibyehello commented Feb 4, 2023 •
edited

I'm only facing one problem, that I know of, currently and that's properly shutting down dolphin, also I accidentally added two requestShutdown() function calls in shutdown.

@Hibyehello
Copy link
Contributor Author

I may of just resolved the shutdown issues

@Hibyehello
Copy link
Contributor Author

I've squashed my commits, all checks passed before I did so

@Hibyehello Hibyehello force-pushed the MacOSPlatform branch 2 times, most recently from 2fff45d to c5d73bc Compare February 5, 2023 00:58
@Hibyehello
Copy link
Contributor Author

Fixed a problem with the window not opening over terminal when launching

@Rumi-Larry
Copy link

The title could read "NoGUI: Create MacOS platform"

@Hibyehello Hibyehello changed the title Create MacOS platform NoGUI: Create MacOS platform Feb 13, 2023
@Hibyehello
Copy link
Contributor Author

I think this is now in a state to merge

TellowKrinkle
TellowKrinkle reviewed Apr 20, 2023
View reviewed changes
Source/Core/DolphinNoGUI/PlatformMacos.mm
@@ -112,7 +113,7 @@ @implementation WindowDelegate
- (void)windowDidResize:(NSNotification*)notification
{
if (g_renderer)
g_renderer->ResizeSurface();
g_presenter->ResizeSurface();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably change to match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh whoops I'll adjust that

@AdmiralCurtiss
Copy link
Contributor

This should probably be rebased to get rid of the merge commit.

@Hibyehello
Copy link
Contributor Author

My plan is to rebase onto upstream, so that all the checks will pass, then I'll squash everything into one commit

TellowKrinkle
TellowKrinkle approved these changes Apr 25, 2023
View reviewed changes
Copy link
Contributor

@TellowKrinkle TellowKrinkle left a comment

Choose a reason for hiding this comment

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

Looks good (once you squash)

@Hibyehello
Copy link
Contributor Author

My computer has been refusing to push the squash for some reason

@JosJuice
Copy link
Member

You need to use the -f flag to force push.

@Hibyehello
Copy link
Contributor Author

Hibyehello commented Apr 27, 2023 •
edited

It's not that, it just sits and does nothing that I can perceive, it writes everything and then just hangs, I left it running over night and all

@Hibyehello
Copy link
Contributor Author

well ok git, let me fix that

@AdmiralCurtiss
Copy link
Contributor

I'll rebase it for you, alright?

@Hibyehello
Copy link
Contributor Author

That's fine with me, I'm still getting used to rebasing

@Hibyehello
Copy link
Contributor Author

How would I do this for future reference?

@AdmiralCurtiss
Copy link
Contributor

Assuming you're on the correct branch and the main Dolphin repository is registered as the upstream remote,

git fetch upstream
git rebase upstream/master
git push --force-with-lease

@Hibyehello
Copy link
Contributor Author

Thanks, much appreciated!

@Hibyehello
Copy link
Contributor Author

Updated the fullscreen hotkey to be the standard fullscreen hotkey for MacOS, that being fn + F

@Hibyehello
Add MacOS Platform
8324a85 
Use MacOS Standard Fullscreen hotkey
@AdmiralCurtiss AdmiralCurtiss merged commit 4c9210b into dolphin-emu:master Jun 2, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phire phire phire left review comments

@TellowKrinkle TellowKrinkle TellowKrinkle approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
6 participants
@Hibyehello @phire @Rumi-Larry @AdmiralCurtiss @JosJuice @TellowKrinkle