Skip to content

macOS Browser Panel Support#197

Merged
WizardCM merged 4 commits into
obsproject:masterfrom
tbodt:macos
Jan 17, 2021
Merged

macOS Browser Panel Support#197
WizardCM merged 4 commits into
obsproject:masterfrom
tbodt:macos

Conversation

@tbodt

@tbodt tbodt commented Feb 9, 2020

Copy link
Copy Markdown
Contributor

Description

Mac support for browser panels.

Depends on obsproject/obs-studio#2386

Motivation and Context

How Has This Been Tested?

Build and run on 10.14.6, click around, not that many things crash

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.

@WizardCM WizardCM changed the title Macos macOS Browser Panel Support Feb 9, 2020
@WizardCM WizardCM added kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. priority/high This is an important PR labels Feb 9, 2020
@tbodt

tbodt commented Feb 9, 2020

Copy link
Copy Markdown
Contributor Author

TODO:

  • copy/paste with keyboard shortcuts and menu bar
  • automatically symlink CEF into rundir/Frameworks
  • (stretch goal) script to download and build the right version of CEF

@tbodt

tbodt commented Feb 16, 2020

Copy link
Copy Markdown
Contributor Author

The packaging is incredibly janky, so I'm just going to say it's Somebody Else's Problem

@jp9000

jp9000 commented Feb 17, 2020

Copy link
Copy Markdown
Member

Packaging?

@tbodt

tbodt commented Feb 17, 2020

Copy link
Copy Markdown
Contributor Author

Getting Chromium Embedded Framework.framework into the right place relative to the executable. Though it's possible this has already been solved by the CI scripts and it's just broken for me because I don't build the same way as CI.

@jp9000

jp9000 commented Feb 17, 2020

Copy link
Copy Markdown
Member

Yea I'm confused, we already include it for the browser source. Were you meaning something different?

@tbodt

tbodt commented Feb 17, 2020

Copy link
Copy Markdown
Contributor Author

No, you understood right. I guess that's not actually a problem.

Is there some way to package the app in the same way that CI does but locally?

@jp9000

jp9000 commented Feb 17, 2020

Copy link
Copy Markdown
Member

Short answer is not very easily at the moment. Long answer is we used to be able to do that with the bundle_app.py script but some things changed and now the CI scripts seem to have some added (and probably unnecessary) complexity. I think they should be something that can be simplified in to a single script but it's multiple scripts at the moment on CI. I was going to take a look at it after we release 25 originally.

@tbodt

tbodt commented Feb 17, 2020

Copy link
Copy Markdown
Contributor Author

It's very hard to resize panels with this. Seems like you need to get the mouse onto exactly the pixel between the panels, or the panel will grab the mouse event.

Comment thread panel/browser-panel.cpp Outdated
@PatTheMav

Copy link
Copy Markdown
Member

@tbodt Checked this out today, resizing is indeed hard - even if the mouse cursor changed to the vertical/horizontal resize variant, you end up creating a selection box in the canvas.

Also when closed in detached state and then reopened, the reopened window can't be resized, but dragged and re-attached.

Panels stay in their attached and detached places between OBS sessions, so that part seems to work fine.

@PatTheMav

PatTheMav commented May 11, 2020

Copy link
Copy Markdown
Member

Did some further checks today, most functionality seems fine (i.e. renaming the title, opening links in a new tab when modifier is pressed), the size is restored correctly for attached panels, but detached panels are reset, maybe the Resize() method needs to be implemented for macOS as well.

Reloading a panel is not possible as the default keystroke CMD+R is already taken by OBS to reset transforms, might be better to move those transform-keystrokes to CMD+Option+R, etc.

As for packaging, CEF expects to be placed in a proper macOS app bundle which CMake doesn't generate, so the framework references need to be patched, which Cmake would do automatically when -DBROWSER_DEPLOY=OFF is used, but that part needs to be updated and even with it in place OBS crashes right now.

I've added a PR to the main project with a cleaned-up build script which just needs to be run with -s -b to create an app bundle that will run with all necessary libraries.

@dodgepong

Copy link
Copy Markdown
Member

Reloading a panel is not possible as the default keystroke CMD+R is already taken by OBS to reset transforms, might be better to move those transform-keystrokes to CMD+Option+R, etc.

Maybe this is as good a time as any to implement a refresh button in the title bar of the dock itself.

@WizardCM

Copy link
Copy Markdown
Member

It's very hard to resize panels with this. Seems like you need to get the mouse onto exactly the pixel between the panels, or the panel will grab the mouse event.

Would adding a couple pixels of margin around the CEF wrapper in the dock do the trick? Likely best to add it on all sides.

@PatTheMav

PatTheMav commented May 18, 2020

Copy link
Copy Markdown
Member

From what I could tell this is an issue with all panels on macOS not just with the browser panel.

Aside from the compilation fix above I'd say that the non-resizable window that you end up when

  1. Detaching
  2. Closing
  3. Re-opening

the panel needs to be fixed as well and then this might be good to go. Haven't had time to figure out to test the built-in Twitch panels with oAuth-flow though.

@WizardCM

Copy link
Copy Markdown
Member

From what I could tell this is an issue with all panels on macOS not just with the browser panel.

Weird, because that's supposed to be controlled by macOS. I guess time to experiment with these? According to a quick Google, modifying the styling of the widget itself won't help.

@WizardCM

Copy link
Copy Markdown
Member

Follow-up from my previous comment - after experimenting with docks on Windows, I can (unfortunately) confirm that docks are locked to the Qt::Tool window type, which means we have very little more we can do (as in, we can't use "bigger" native decorations).

Technically, Qt supports custom window decorations, but in my testing on Windows the grips around the window turn from 5px to 1px in custom mode, and I expect the behaviour would be similar on other platforms. It's a terrible experience and I wouldn't recommend the switch.

@eric

eric commented Jun 27, 2020

Copy link
Copy Markdown
Contributor

I'm running into the objc_msgSend issue when I try to build this

@PatTheMav

Copy link
Copy Markdown
Member

@eric if I'm not mistaken this issue will occur when a more recent version of platform libraries or clang is used, so it's a must fix in my book.

@eric

eric commented Jun 28, 2020

Copy link
Copy Markdown
Contributor

Is someone else going to take up the work on this pull request?

Comment thread panel/browser-panel.cpp Outdated
@tbodt

tbodt commented Jun 28, 2020

Copy link
Copy Markdown
Contributor Author

For the curious, I'm pretty sure they changed the objc_msgSend prototype because they knew the varargs ABI wouldn't be compatible with normal parameters on the upcoming ARM macs, and decided to change the prototype a year early to get everyone ready

@tbodt

tbodt commented Jun 28, 2020

Copy link
Copy Markdown
Contributor Author

Went ahead and rebased this as well

@zavitax

zavitax commented Nov 28, 2020

Copy link
Copy Markdown
Contributor

Oh, I'm talking about docked panels. Didn't realize you were talking about something different, since they both have some kind of resize grip.

There you have it. I was not aware docked state even has an issue.

@zavitax

zavitax commented Nov 28, 2020

Copy link
Copy Markdown
Contributor

Some more info from Qt's docs:
https://doc.qt.io/qt-5/stylesheet-reference.html

QDockWidget
Supports styling of the title bar and the title bar buttons when docked.
The dock widget border can be styled using the border property. The ::title subcontrol can be used to customize the title bar. The close and float buttons are positioned with respect to the ::title subcontrol using the ::close-button and ::float-button respectively.

When the title bar is vertical, the :vertical pseudo class is set. In addition, depending on QDockWidget::DockWidgetFeature, the :closable, :floatable and :movable pseudo states are set.

Note: Use QMainWindow::separator to style the resize handle.

Warning: The style sheet has no effect when the QDockWidget is undocked as Qt uses native top level windows when undocked.

@tbodt

tbodt commented Nov 29, 2020

Copy link
Copy Markdown
Contributor Author

So far I've figured out that despite the splitter being 5px wide (in OBS default dark theme) its QWidgetWindow has a mask that ignores any input outside a 1px wide region. I'm not sure where this mask comes from. The QSplitter widget sets a mask on itself, but my understanding is that widget masks affect only drawing, not input. On top of this it looks like there might be a bug in Qt handling of mouse events on masked windows, where the mouse down event passes through but not the mouse drag or mouse up event.

@WizardCM

Copy link
Copy Markdown
Member

Unrelated to the current discussion, I just realised a discrepency of featureset - on Windows, we listen for a Ctrl+R keypress to allow users to reload the panel's contents. A similar condition should be added for macOS (I assume Cmd+R would be best?).

#ifdef _WIN32
if (event.type != KEYEVENT_RAWKEYDOWN)
return false;
if (event.windows_key_code == 'R' &&
(event.modifiers & EVENTFLAG_CONTROL_DOWN) != 0) {
browser->ReloadIgnoreCache();
return true;
}
#endif

@PatTheMav

Copy link
Copy Markdown
Member

As mentioned in #197 (comment):

Reloading a panel is not possible as the default keystroke CMD+R is already taken by OBS to reset transforms, might be better to move those transform-keystrokes to CMD+Option+R, etc.

Even better would be non-global keyboard shortcuts (that is shortcuts that change based on window focus) but that's not how Qt works iirc.

@WizardCM

Copy link
Copy Markdown
Member

At least on Windows, when CEF is focused, Qt doesn't have focus so default shortcuts don't work. I assume this behaviour is different on macOS due to the command bar? I assume you've verified the behaviour yourself by adding in the relevant check?

Comment thread panel/browser-panel.hpp Outdated
@PatTheMav

Copy link
Copy Markdown
Member

@WizardCM I wouldn't know where to implement those checks, but I can confirm that OBS intercepts all keystrokes, so even copy&paste functionality is intercepted (so no pasting passwords into the oauth panels). Right-clicking in the CEF window and choosing "copy/paste" works however.

Comment thread CMakeLists.txt Outdated
@PatTheMav

Copy link
Copy Markdown
Member

This needs to be changed at least for macOS:

BPtr<char> path = os_get_abs_path_ptr(rpath.Get());

obs_module_config_path already returns an absolute path on macOS (possibly all POSIX-based systems?) and for some reason os_get_abs_path_ptr will fail (as realpath will return an error, probably because the path didn't exist on first run).

Possible fix:

#ifdef _WIN32
		BPtr<char> rpath = obs_module_config_path(storage_path.c_str());
		BPtr<char> path = os_get_abs_path_ptr(rpath.Get());
#else
        BPtr<char> path = obs_module_config_path(storage_path.c_str());
#endif

With this Twitch oauth and panels worked fine, as did chat addons.

@WizardCM

WizardCM commented Dec 9, 2020

Copy link
Copy Markdown
Member

@PatTheMav I can confirm on Ubuntu 20.04 that that line requires changing too. Nice catch!

@tbodt tbodt force-pushed the macos branch 2 times, most recently from a9bc71c to bb43f41 Compare December 16, 2020 03:50
@PatTheMav

Copy link
Copy Markdown
Member

I've seen @tbodt pushed some requested changes - great! Would you mind applying this change as well? macOS and Linux require it: #197 (comment)

@tbodt

tbodt commented Dec 16, 2020

Copy link
Copy Markdown
Contributor Author

@PatTheMav I can't reproduce the crash. Is there something special I need to do?

@PatTheMav

Copy link
Copy Markdown
Member

@PatTheMav I can't reproduce the crash. Is there something special I need to do?

You need to delete the obs_profile_cookies directory under Application Support/obs-studio/plugin_config/obs-browser if it exists. When you launch OBS without that fix it will crash.


I've tested this PR with and without Twitch OAUTH support and also with CEF 3770 and CEF 4183, works fine with both versions. Our patched Qt 5.15.2 fixes the resize bug with detached panels and otherwise I didn't run into unexpected behaviour.

I will try testing this with a debug build tomorrow to catch any low-level CEF issues, but looks fine so far.

obs_get_abs_path_ptr on Mac uses realpath, which fails if the file
doesn't exist.
@PatTheMav

Copy link
Copy Markdown
Member

I tested this today as well, deleted the obs-browser plugin config directory before and did not get any crash so far. Also tested our Twitch authentication with it, all worked as expected (including the quirks known from the Windows side of things).

Good to merge from my POV.

@WizardCM

Copy link
Copy Markdown
Member

Confirmed everything works very well with latest OBS master on 10.14. I was unable to reproduce the quirks others have mentioned, so I have to assume they are introduced in 10.15 and above.

@WizardCM WizardCM merged commit a07cd2c into obsproject:master Jan 17, 2021
@tbodt

tbodt commented Jan 17, 2021

Copy link
Copy Markdown
Contributor Author

🚀

@tbodt tbodt deleted the macos branch January 17, 2021 08:08
@WizardCM WizardCM added this to the OBS Studio 27.0 milestone Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. priority/high This is an important PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants