macOS native bar button items #16922

NSAntoine · 2023-02-06T19:21:06Z

yeah

hrydgard

Some initial comments.

As mentioned I dislike the callback, would be nice if the update could be done the moment the touch bar is clicked, before the actual menu shows, if it's possible to catch that event.

The backend setting doesn't work properly (admittedly, the auto restart is already not working, but there's not even a notification). Maybe remove from the menu for now.

Window menu should be towards the right, and not be the first one, File should go first after the application menu.

hrydgard · 2023-02-06T20:34:08Z

SDL/MacOSBarItems.h

+#ifndef MacOSBarItems_h
+#define MacOSBarItems_h


#pragma once plz

hrydgard · 2023-02-06T20:34:35Z

SDL/MacOSBarItems.mm

+    [[BarItemsManager sharedInstance] setupAppBarItems];
+}
+
+// im soooooo sorry for whoever had to read this impl


unnecessary comment :)

This code is not nearly bad enough to apologize for, heh.

-[Unknown]

hrydgard · 2023-02-06T20:37:38Z

SDL/MacOSBarItems.mm

+    [[NSNotificationCenter defaultCenter] addObserverForName:@"ConfigDidChange" object:nil queue:nil usingBlock:^(NSNotification * _Nonnull note) {
+        NSString *value = [note object];
+        // NOTE: Though it may seem like it,
+        // the next few lines were not written by yandere dev


A joke reference to Yandere simulator developer who was a meme about DRY violation and exhaustive if/else coding.

Shouldn't be a commend imho as I don't really like the shaming :)

Pic for reference: https://i.redd.it/gyubzyq87xmx.png

unknownbrackets · 2023-02-07T04:24:54Z

UI/MainScreen.cpp

-			if (tab == 1) {
-				gameBrowsers_[tab]->SetPath(Path(filename));
-			}
+			INFO_LOG(SYSTEM, "Got folder: '%s'", filename.c_str());;


nit: s/;;/;/.

-[Unknown]

unknownbrackets · 2023-02-07T04:27:28Z

UI/MainScreen.cpp

-			}
+			INFO_LOG(SYSTEM, "Got folder: '%s'", filename.c_str());;
+			// switch to the 'Games' tab which has the file browser
+			tabHolder_->SetCurrentTab(1);


If you had no recent before, then it's actually tab 0. I guess you could check if the first view's tag is "MainScreenRecentGames" and use 1 if it is.

I realize the old code hardcoded 1 so was actually wrong.

-[Unknown]

unknownbrackets · 2023-02-07T04:36:07Z

UI/GameSettingsScreen.cpp

@@ -1563,6 +1575,7 @@ UI::EventReturn GameSettingsScreen::OnAudioDevice(UI::EventParams &e) {
 		g_Config.sAudioDevice.clear();
 	}
 	System_SendMessage("audio_resetDevice", "");
+	PostDarwinNotificationIfPossible("AudioConfigurationHasChanged", "CurrentDeviceWasChanged");


If we need to keep this, it should be tied to System_SendMessage() only. The implementation within System_SendMessage() would be allowed to call PostDarwinNotificationIfPossible(), but that would only be within SDL/ or ios/ code.

If there was any PostMessageToWindowsMenubar(), I would have an identical opinion.

That said, I agree with Henrik - if it's possible to update the menu items on some event prior to loading, that would be better. Sounds like there is a way with NSMenuDelegate.

-[Unknown]

unknownbrackets · 2023-02-07T04:38:24Z

Core/Config.cpp

@@ -1301,6 +1301,15 @@ void Config::SetAppendedConfigIni(const Path &path) {
 	appendedConfigFileName_ = path;
 }

+void Config::updateAfterSettingAutoFrameSkip() {


nit: Should be a capital U to follow other methods.

That said, maybe this could be combined with PostLoadCleanup() instead (it probably should have the iFrameSkip = 1 bit anyway), and we could just run that when needed. Better to be more generic in methods exposed by Config, I'd say.

-[Unknown]

unknownbrackets · 2023-02-07T04:39:13Z

Core/Config.h

+
+

nit: Spacious.

-[Unknown]

unknownbrackets · 2023-02-07T04:44:34Z

SDL/MacOSBarItems.mm

+
+    [parent addItem:[NSMenuItem separatorItem]];
+
+#define MENU_ITEM(variableName, localizedTitleName, SEL, ConfigurationValueName) \


nit: It feels like this could just be a regular method that returns NSMenuItem *. It also feels like it could automatically call `[parent addItem:] for you. It's fine, just nitmumbling to myself.

-[Unknown]

unknownbrackets · 2023-02-07T04:45:27Z

SDL/MacOSBarItems.mm

+    [[BarItemsManager sharedInstance] setupAppBarItems];
+}
+
+// im soooooo sorry for whoever had to read this impl


This code is not nearly bad enough to apologize for, heh.

-[Unknown]

unknownbrackets · 2023-02-07T04:49:12Z

SDL/MacOSBarItems.mm

+-(std::vector<GPUBackend>)allowedGPUBackends {
+    std::vector<GPUBackend> allBackends = {
+        GPUBackend::OPENGL, GPUBackend::VULKAN,
+        GPUBackend::DIRECT3D11, GPUBackend::DIRECT3D9


I'll be impressed if anyone ever gets these two working on Cocoa.

-[Unknown]

Oh yeah that's basically impossible, heh, I did think 'hey these can't ever really work on macOS or be enabled' but then got distracted by something else, will remove :p

unknownbrackets · 2023-02-07T04:50:21Z

SDL/SDLMain.cpp

@@ -846,6 +852,12 @@ int main(int argc, char *argv[]) {
 	int mouseWheelMovedDownFrames = 0;
 	bool mouseCaptured = false;
 	bool windowHidden = false;
+
+#if PPSSPP_PLATFORM(MAC)
+		// setup menu items for macOS


nit: Overindent.

-[Unknown]

unknownbrackets · 2023-02-07T04:51:51Z

UI/GameSettingsScreen.cpp

-			NativeResized();
-			return UI::EVENT_CONTINUE;
-		});
+        OnClickPostDarwinNotification(vSync, "ConfigDidChange", "VSync")


nit: Spaces stranded in a land of tabs. Happens a few other times, but I guess these lines will go away anyway when the menu builds itself.

More importantly, I think we still need the vsync NativeResized(), at least on other platforms?

-[Unknown]

More importantly, I think we still need the vsync NativeResized(), at least on other platforms?

oops, forgot this, thanks for pointing out

hrydgard

Really appreciate the effort to keep the logic mostly in /SDL!

Two of my previous comments still apply:

Window menu should not be left-most
Backend selector still doesn't actually work properly (should restart the emu).

hrydgard · 2023-02-14T17:47:16Z

UI/DarwinFileSystemServices.mm

@@ -109,3 +109,7 @@ - (void)documentPicker:(UIDocumentPickerViewController *)controller didPickDocum
                                              forKey:@(PreferredMemoryStickUserDefaultsKey)];
    g_Config.memStickDirectory = path;
 }
+
+void PostDarwinNotification(const char *name, const char *value) {


Is this still needed?

hrydgard · 2023-02-14T17:47:27Z

SDL/SDLMain.cpp

+    // setup menu items for macOS
+    initBarItemsForApp();


wrong indentation

hrydgard · 2023-02-14T17:48:04Z

UI/GameSettingsScreen.cpp

-	audioSettings->Add(new CheckBox(&g_Config.bEnableSound, a->T("Enable Sound")));
-
+	CheckBox *enableSound = audioSettings->Add(new CheckBox(&g_Config.bEnableSound,a->T("Enable Sound")));
+	


white space?

SDL/CocoaBarItems.mm

hrydgard

Still has the bad submodule upgrades. Otherwise, I'm happy to get this in now, can be refined as we go later.

NSAntoine added 3 commits February 6, 2023 22:17

macOS bar button items

6d474ac

EMERGENCY FIX

cfe6ac5

fix on other platforms

070194e

hrydgard requested changes Feb 6, 2023

View reviewed changes

hrydgard added the Platform-specific (Mac OS X) label Feb 6, 2023

hrydgard added this to the v1.15.0 milestone Feb 6, 2023

unknownbrackets reviewed Feb 7, 2023

View reviewed changes

NSAntoine added 2 commits February 14, 2023 19:11

address concerns

c64aa9d

i forgot this

6b0cec2

hrydgard requested changes Feb 14, 2023

View reviewed changes

NSAntoine and others added 3 commits March 23, 2023 21:27

Merge branch 'master' into serena/macos-bar-menu-items

645407f

fixes

84849d9

fix openSystemFileBrowser

ea4e294

hrydgard reviewed Mar 24, 2023

View reviewed changes

SDL/CocoaBarItems.mm Outdated Show resolved Hide resolved

hrydgard approved these changes Mar 24, 2023

View reviewed changes

Switch to boot

67896bf

hrydgard merged commit 67896bf into hrydgard:master Mar 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

macOS native bar button items #16922

macOS native bar button items #16922

NSAntoine commented Feb 6, 2023

hrydgard left a comment

hrydgard Feb 6, 2023

hrydgard Feb 6, 2023

unknownbrackets Feb 7, 2023

hrydgard Feb 6, 2023

iota97 Feb 7, 2023

unknownbrackets Feb 7, 2023

unknownbrackets Feb 7, 2023

unknownbrackets Feb 7, 2023

unknownbrackets Feb 7, 2023

unknownbrackets Feb 7, 2023

unknownbrackets Feb 7, 2023

unknownbrackets Feb 7, 2023

unknownbrackets Feb 7, 2023

NSAntoine Feb 7, 2023

unknownbrackets Feb 7, 2023

unknownbrackets Feb 7, 2023

NSAntoine Feb 7, 2023

hrydgard left a comment

hrydgard Feb 14, 2023

hrydgard Feb 14, 2023

hrydgard Feb 14, 2023

hrydgard left a comment


		[parent addItem:[NSMenuItem separatorItem]];

		#define MENU_ITEM(variableName, localizedTitleName, SEL, ConfigurationValueName) \

		audioSettings->Add(new CheckBox(&g_Config.bEnableSound, a->T("Enable Sound")));

		CheckBox *enableSound = audioSettings->Add(new CheckBox(&g_Config.bEnableSound,a->T("Enable Sound")));

macOS native bar button items #16922

macOS native bar button items #16922

Conversation

NSAntoine commented Feb 6, 2023

hrydgard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrydgard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrydgard left a comment

Choose a reason for hiding this comment