gh-obsproject/obs-browser
Watch
1
0
Fork
You've already forked obs-browser
 0
Code Issues 37 Pull requests 13 Releases 15 Wiki Activity

macOS Browser Panel Support #197

Merged
tbodt merged 4 commits from macos into master 2021-01-16 20:16:35 -06:00
Conversation 70 Commits 4 Files changed 3 +33 -23
tbodt commented 2020-02-09 01:53:47 -06:00 (Migrated from github.com)

Description

Mac support for browser panels.

Depends on https://github.com/obsproject/obs-studio/pull/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.
<!--- Please fill out the following template, which will help other contributors review your Pull Request. --> <!--- Make sure you’ve read the contribution guidelines here: https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst --> ### Description Mac support for browser panels. Depends on https://github.com/obsproject/obs-studio/pull/2386 ### Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open Mantis issue, or implements feature request --> <!--- from the Ideas page, please link to the issue here. --> ### How Has This Been Tested? Build and run on 10.14.6, click around, not that many things crash ### Types of changes <!--- What types of changes does your PR introduce? Uncomment all that apply --> <!--- - Bug fix (non-breaking change which fixes an issue) --> - New feature (non-breaking change which adds functionality) <!--- - Performance enhancement (non-breaking change which improves efficiency) --> <!--- - Code cleanup (non-breaking change which makes code smaller or more readable) --> <!--- - Breaking change (fix or feature that would cause existing functionality to change) --> <!--- - Documentation (a change to documentation pages) --> ### Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] My code has been run through [clang-format](https://github.com/obsproject/obs-studio/blob/master/.clang-format). - [x] I have read the [**contributing** document](https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst). - [x] My code is not on the master branch. - [x] The code has been tested. - [x] All commit messages are properly formatted and commits squashed where appropriate.
tbodt commented 2020-02-09 02:20:10 -06:00 (Migrated from github.com)

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
TODO: - [ ] copy/paste with keyboard shortcuts and menu bar - [x] automatically symlink CEF into rundir/Frameworks - [x] (stretch goal) script to download and build the right version of CEF
tbodt commented 2020-02-16 16:50:31 -06:00 (Migrated from github.com)

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

The packaging is incredibly janky, so I'm just going to say it's Somebody Else's Problem
jp9000 commented 2020-02-16 18:06:11 -06:00 (Migrated from github.com)

Packaging?

Packaging?
tbodt commented 2020-02-16 18:07:56 -06:00 (Migrated from github.com)

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.

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 commented 2020-02-16 18:15:49 -06:00 (Migrated from github.com)

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

Yea I'm confused, we already include it for the browser source. Were you meaning something different?
tbodt commented 2020-02-16 18:17:42 -06:00 (Migrated from github.com)

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?

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 commented 2020-02-16 18:31:05 -06:00 (Migrated from github.com)

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.

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 commented 2020-02-17 12:38:56 -06:00 (Migrated from github.com)

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.

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.
PatTheMav (Migrated from github.com) reviewed 2020-04-21 13:50:47 -05:00
panel/browser-panel.cpp
PatTheMav (Migrated from github.com) commented 2020-04-21 13:50:47 -05:00

Calling objc_msgSend directly is discouraged and Xcode 11 refuses to compile, this works however:

	id view = (id)cefBrowser->GetHost()->GetWindowHandle();
        SEL removeFromViewSel = sel_getUid("removeFromSuperview");
        
        typedef id(*remove_panel_func)(id, SEL);
        remove_panel_func removePanel = (remove_panel_func)objc_msgSend;
        
        removePanel(view, removeFromViewSel);
Calling `objc_msgSend` directly is discouraged and Xcode 11 refuses to compile, this works however: ``` id view = (id)cefBrowser->GetHost()->GetWindowHandle(); SEL removeFromViewSel = sel_getUid("removeFromSuperview"); typedef id(*remove_panel_func)(id, SEL); remove_panel_func removePanel = (remove_panel_func)objc_msgSend; removePanel(view, removeFromViewSel); ```
PatTheMav commented 2020-04-21 13:56:11 -05:00 (Migrated from github.com)

@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.

@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 commented 2020-05-11 12:04:46 -05:00 (Migrated from github.com)

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.

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 commented 2020-05-11 12:18:05 -05:00 (Migrated from github.com)

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.

> 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 commented 2020-05-18 05:16:54 -05:00 (Migrated from github.com)

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.

> 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 commented 2020-05-18 05:49:51 -05:00 (Migrated from github.com)

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.

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.
👍 1
WizardCM commented 2020-05-18 05:53:33 -05:00 (Migrated from github.com)

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.

> 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](https://doc.qt.io/qt-5/qt.html#WindowType-enum)? According to a quick Google, modifying the styling of the widget itself won't help.
WizardCM commented 2020-05-29 05:50:23 -05:00 (Migrated from github.com)

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.

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 commented 2020-06-27 14:41:47 -05:00 (Migrated from github.com)

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

I'm running into the `objc_msgSend` issue when I try to build this
PatTheMav commented 2020-06-27 16:34:34 -05:00 (Migrated from github.com)

@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 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 commented 2020-06-27 19:43:56 -05:00 (Migrated from github.com)

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

Is someone else going to take up the work on this pull request?
eric (Migrated from github.com) reviewed 2020-06-27 19:45:52 -05:00
panel/browser-panel.cpp
eric (Migrated from github.com) commented 2020-06-27 19:45:51 -05:00

To make it easier to make that change:

		SEL removeFromViewSel = sel_getUid("removeFromSuperview");

		typedef id(*remove_panel_func)(id, SEL);
		remove_panel_func removePanel = (remove_panel_func)objc_msgSend;

		removePanel(view, removeFromViewSel);
To make it easier to make that change: ```suggestion SEL removeFromViewSel = sel_getUid("removeFromSuperview"); typedef id(*remove_panel_func)(id, SEL); remove_panel_func removePanel = (remove_panel_func)objc_msgSend; removePanel(view, removeFromViewSel); ```
tbodt (Migrated from github.com) reviewed 2020-06-27 21:49:50 -05:00
panel/browser-panel.cpp
tbodt (Migrated from github.com) commented 2020-06-27 21:49:50 -05:00

I prefer something a little more compact

		((id (*)(id, SEL)) obj_msgSend)(view, sel_getUid("removeFromSuperview"));
I prefer something a little more compact ```suggestion ((id (*)(id, SEL)) obj_msgSend)(view, sel_getUid("removeFromSuperview")); ```
tbodt commented 2020-06-27 21:53:19 -05:00 (Migrated from github.com)

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

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 commented 2020-06-27 21:54:26 -05:00 (Migrated from github.com)

Went ahead and rebased this as well

Went ahead and rebased this as well
WizardCM commented 2020-06-27 22:15:30 -05:00 (Migrated from github.com)

Great! Time to discuss potential solutions for the TODOs.

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

For 1, I think that ideally the correct behaviour is treating the Qt shortcuts as priority, and if the user has a CEF panel focused, allowing OBS to pass things back and forth. The problem is that the OBS Copy/Paste isn't designed for text, it's purely for sources. Showing different copy/paste options while a panel is focused would probably be a very unpleasant hack UX-wise, so I'm not sure what we should do here.
For 2/3, does https://github.com/obsproject/obs-studio/pull/2890 help in achieving them?

detached panels are reset, maybe the Resize() method needs to be implemented for macOS as well

Has this been explored? The goal is to resize the window based on the size of the widget.

github.com/obsproject/obs-browser@2e5f316b54/panel/browser-panel.cpp (L272-L275)

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.

I'd do it opposite - let's give the new functionality (ie. browser panels) the less common keys, the check itself is pretty simple otherwise

github.com/obsproject/obs-browser@2e5f316b54/panel/browser-panel-client.cpp (L204-L206)

Great! Time to discuss potential solutions for the TODOs. > TODO: > > * [ ] copy/paste with keyboard shortcuts and menu bar > * [x] automatically symlink CEF into rundir/Frameworks > * [x] (stretch goal) script to download and build the right version of CEF For 1, I think that ideally the correct behaviour is treating the Qt shortcuts as priority, and if the user has a CEF panel focused, allowing OBS to pass things back and forth. The problem is that the OBS Copy/Paste isn't designed for text, it's purely for sources. Showing different copy/paste options while a panel is focused would probably be a very unpleasant hack UX-wise, so I'm not sure what we should do here. For 2/3, does https://github.com/obsproject/obs-studio/pull/2890 help in achieving them? > detached panels are reset, maybe the `Resize()` method needs to be implemented for macOS as well Has this been explored? The goal is to resize the window based on the size of the widget. https://github.com/obsproject/obs-browser/blob/2e5f316b5473f5e6c37a04c67000432001cac7f5/panel/browser-panel.cpp#L272-L275 > 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. I'd do it opposite - let's give the new functionality (ie. browser panels) the less common keys, the check itself is pretty simple otherwise https://github.com/obsproject/obs-browser/blob/2e5f316b5473f5e6c37a04c67000432001cac7f5/panel/browser-panel-client.cpp#L204-L206
tbodt commented 2020-06-27 22:20:26 -05:00 (Migrated from github.com)

For 2/3, does obsproject/obs-studio#2890 help in achieving them?

Yep, let's cross those off the list (thanks @PatTheMav!)

The difficulty of resizing is much more annoying to me than any of the other problems you mention, but it seems like that's a problem with all the panels and not just browser panels. Is there another issue open for that?

> For 2/3, does obsproject/obs-studio#2890 help in achieving them? Yep, let's cross those off the list (thanks @PatTheMav!) The difficulty of resizing is much more annoying to me than any of the other problems you mention, but it seems like that's a problem with all the panels and not just browser panels. Is there another issue open for that?
tbodt commented 2020-06-27 22:22:49 -05:00 (Migrated from github.com)

The problem is that the OBS Copy/Paste isn't designed for text, it's purely for sources.

Copy/pasting with keyboard shortcuts seems to work fine in QT text fields, so maybe we can do whatever that's doing in browser panels.

It seems weird to me that OBS has completely custom handling of copy/paste though: shouldn't QT be handling that by passing it to whatever the currently focused thing is? Then the same action can work for sources and everything else.

> The problem is that the OBS Copy/Paste isn't designed for text, it's purely for sources. Copy/pasting with keyboard shortcuts seems to work fine in QT text fields, so maybe we can do whatever that's doing in browser panels. It seems weird to me that OBS has completely custom handling of copy/paste though: shouldn't QT be handling that by passing it to whatever the currently focused thing is? Then the same action can work for sources and everything else.
WizardCM commented 2020-06-27 22:36:38 -05:00 (Migrated from github.com)

The difficulty of resizing is much more annoying to me than any of the other problems you mention, but it seems like that's a problem with all the panels and not just browser panels. Is there another issue open for that?

Based on my own research, as far as I can tell this is a limitation of macOS "tool" windows, and can probably be reproduced with non-Qt apps. We don't currently have an issue open on our side, no.

It seems weird to me that OBS has completely custom handling of copy/paste though: shouldn't QT be handling that by passing it to whatever the currently focused thing is? Then the same action can work for sources and everything else.

Qt normally does, yes. The problem is with how CEF handles focus - either due to how we have it implemented, or for some other reason. Switching focus from OBS itself to a browser panel does not provide any event to Qt that focus has changed. Once you then unfocus the entire OBS window (including the dock) without clicking within OBS itself, Qt never receives an unfocus event, because CEF never passes it over to Qt. This can result in a disconnect, where OBS still thinks it's focused (breaking hotkeys, for example). In my testing, the CEF Focus Handler just doesn't do what it should. You can find my attempts here: https://github.com/obsproject/obs-browser/compare/master...WizardCM:fix-focus - but I've basically hit a wall.

> The difficulty of resizing is much more annoying to me than any of the other problems you mention, but it seems like that's a problem with all the panels and not just browser panels. Is there another issue open for that? Based on my own research, as far as I can tell this is a limitation of macOS "tool" windows, and can probably be reproduced with non-Qt apps. We don't currently have an issue open on our side, no. > It seems weird to me that OBS has completely custom handling of copy/paste though: shouldn't QT be handling that by passing it to whatever the currently focused thing is? Then the same action can work for sources and everything else. Qt normally does, yes. The problem is with how CEF handles focus - either due to how we have it implemented, or for some other reason. Switching focus from OBS itself to a browser panel does not provide any event to Qt that focus has changed. Once you then unfocus the entire OBS window (including the dock) without clicking within OBS itself, Qt never receives an unfocus event, because CEF never passes it over to Qt. This can result in a disconnect, where OBS still thinks it's focused (breaking hotkeys, for example). In my testing, the CEF Focus Handler just doesn't do what it should. You can find my attempts here: https://github.com/obsproject/obs-browser/compare/master...WizardCM:fix-focus - but I've basically hit a wall.
tbodt commented 2020-06-27 22:44:02 -05:00 (Migrated from github.com)

AFAIK macOS doesn't have a native concept of "tool" windows, or docks of the type QT supports. That's entirely implemented within QT. I just tried another QT app with docks (https://sqlitebrowser.org/) and resizing was much smoother than in OBS, so it must be a problem with OBS.

AFAIK macOS doesn't have a native concept of "tool" windows, or docks of the type QT supports. That's entirely implemented within QT. I just tried another QT app with docks (https://sqlitebrowser.org/) and resizing was much smoother than in OBS, so it must be a problem with OBS.
👍 2
RytoEX commented 2020-06-28 00:18:03 -05:00 (Migrated from github.com)

@tbodt
Do you know what version of Qt they are using? As I understand it, there have been a lot of changes between Qt 5.10 and 5.15 that may change performance on macOS, some causing performance regressions and some improvements.

@tbodt Do you know what version of Qt they are using? As I understand it, there have been a lot of changes between Qt 5.10 and 5.15 that may change performance on macOS, some causing performance regressions and some improvements.
tbodt commented 2020-06-28 00:21:12 -05:00 (Migrated from github.com)

They use 5.12 apparently. https://github.com/sqlitebrowser/sqlitebrowser/pull/2287#issuecomment-645774467

They use 5.12 apparently. https://github.com/sqlitebrowser/sqlitebrowser/pull/2287#issuecomment-645774467
WizardCM commented 2020-06-28 18:37:52 -05:00 (Migrated from github.com)

They use 5.12 apparently. sqlitebrowser/sqlitebrowser#2287 (comment)

In that case, let's try something.

  • Try OBS in the "System" theme (Settings -> General) and restart OBS. Does it solve the issue, at least with non-browser docks?

If this solves the issue, this is either caused by specific QSS code on our end, or it's caused by Qt's "reset" when custom QSS is used. If either of those is the case, should be easy to fix with our own QSS.

If this doesn't solve the issue, then SQLiteBrowser is doing something different from us, and we may still be able to solve it by looking at their code.

> They use 5.12 apparently. [sqlitebrowser/sqlitebrowser#2287 (comment)](https://github.com/sqlitebrowser/sqlitebrowser/pull/2287#issuecomment-645774467) In that case, let's try something. * Try OBS in the "System" theme (Settings -> General) and restart OBS. Does it solve the issue, at least with non-browser docks? If this solves the issue, this is either caused by specific QSS code on our end, **or** it's caused by Qt's "reset" when custom QSS is used. If either of those is the case, should be easy to fix with our own QSS. If this doesn't solve the issue, then SQLiteBrowser is doing something different from us, and we may still be able to solve it by looking at their code.
RytoEX commented 2020-06-28 19:34:50 -05:00 (Migrated from github.com)

To clarify, as far as I know, the graphics changes in Qt that I'm referencing were initially released in 5.12.3 and 5.14. Additional changes were done in 5.12.6, 5.12.7, 5.12.8, 5.12.9, 5.14.1, 5.14.2, 5.15, and the upcoming 5.15.1.

To clarify, as far as I know, the graphics changes in Qt that I'm referencing were initially released in 5.12.3 and 5.14. Additional changes were done in 5.12.6, 5.12.7, 5.12.8, 5.12.9, 5.14.1, 5.14.2, 5.15, and the upcoming 5.15.1.
tbodt commented 2020-06-29 14:28:57 -05:00 (Migrated from github.com)

I did try the "System" theme in OBS and it behaved badly in the same way.

I did try the "System" theme in OBS and it behaved badly in the same way.
ArtiomNosov commented 2020-10-11 08:37:29 -05:00 (Migrated from github.com)

Hi, I built OBS Studio and Xcode gave an error.

CI + -G Xcode (image)https://user-images.githubusercontent.com/71022195/95679952-be670580-0bde-11eb-8ef8-94e2f3caee5e.png
Hi, I built OBS Studio and Xcode gave an error. <details><summary>CI + -G Xcode (image)</summary>https://user-images.githubusercontent.com/71022195/95679952-be670580-0bde-11eb-8ef8-94e2f3caee5e.png</details>
PatTheMav commented 2020-10-11 10:17:52 -05:00 (Migrated from github.com)

@H20CH3PO2 what was the precise make call that you used to configure the Xcode project? Also what Xcode version and macOS version do you have there?

@H20CH3PO2 what was the precise make call that you used to configure the Xcode project? Also what Xcode version and macOS version do you have there?
ArtiomNosov commented 2020-10-12 08:13:19 -05:00 (Migrated from github.com)

@PatTheMav
I used (wiki/inst intructions)

cmake \
 -DCMAKE_OSX_DEPLOYMENT_TARGET=10.13 \
 -DQTDIR="/tmp/obsdeps" \
 -DSWIGDIR="/tmp/obsdeps" \
 -DDepsPath="/tmp/obsdeps" \
 -DDISABLE_PYTHON=ON \
 -DBUILD_BROWSER=ON \
 -DCEF_ROOT_DIR="/pre-built-cef-3770_macosx64" \
 -GXcode ..

or also, with expanded vars, used CI+-GXcode at another build dir

cmake \
 -DENABLE_SPARKLE_UPDATER=ON \
 -DCMAKE_OSX_DEPLOYMENT_TARGET=10.13 \
 -DDISABLE_PYTHON=ON \
 -DQTDIR="/tmp/obsdeps" \
 -DSWIGDIR="/tmp/obsdeps" \
 -DDepsPath="/tmp/obsdeps" \
 -DVLCPath="${DEPS_BUILD_DIR}/vlc-${VLC_VERSION:-${CI_VLC_VERSION}}" \
 -DBUILD_BROWSER=ON \
 -DBROWSER_DEPLOY=ON \
 -DBUILD_CAPTIONS=ON \
 -DWITH_RTMPS=ON \
 -DCEF_ROOT_DIR="${DEPS_BUILD_DIR}/cefbinary${CEF_BUILD_VERSION:-${CI_CEF_VERSION}}_macosx64" \
 -GXcode ..

My macOS ver: 10.15.7 (19H2)
My Xcode ver: 12.0.1 (12A7300)

@PatTheMav I used (wiki/inst intructions) ``` cmake \ -DCMAKE_OSX_DEPLOYMENT_TARGET=10.13 \ -DQTDIR="/tmp/obsdeps" \ -DSWIGDIR="/tmp/obsdeps" \ -DDepsPath="/tmp/obsdeps" \ -DDISABLE_PYTHON=ON \ -DBUILD_BROWSER=ON \ -DCEF_ROOT_DIR="/pre-built-cef-3770_macosx64" \ -GXcode .. ``` or also, with expanded vars, used CI+-GXcode at another build dir ``` cmake \ -DENABLE_SPARKLE_UPDATER=ON \ -DCMAKE_OSX_DEPLOYMENT_TARGET=10.13 \ -DDISABLE_PYTHON=ON \ -DQTDIR="/tmp/obsdeps" \ -DSWIGDIR="/tmp/obsdeps" \ -DDepsPath="/tmp/obsdeps" \ -DVLCPath="${DEPS_BUILD_DIR}/vlc-${VLC_VERSION:-${CI_VLC_VERSION}}" \ -DBUILD_BROWSER=ON \ -DBROWSER_DEPLOY=ON \ -DBUILD_CAPTIONS=ON \ -DWITH_RTMPS=ON \ -DCEF_ROOT_DIR="${DEPS_BUILD_DIR}/cefbinary${CEF_BUILD_VERSION:-${CI_CEF_VERSION}}_macosx64" \ -GXcode .. ``` My macOS ver: 10.15.7 (19H2) My Xcode ver: 12.0.1 (12A7300)
PatTheMav commented 2020-10-12 08:34:41 -05:00 (Migrated from github.com)

Maybe stupid question, but did you run the additional fix-ups for CEF (see below)?

sed -i '.orig' '/add_subdirectory(tests\/ceftests)/d' ./CMakeLists.txt
sed -i '.orig' s/\"10.9\"/\"10.13\"/ ./cmake/cef_variables.cmake
cmake \
        -DCMAKE_CXX_FLAGS="-std=c++11 -stdlib=libc++"\
        -DCMAKE_EXE_LINKER_FLAGS="-std=c++11 -stdlib=libc++"\
        -DCMAKE_OSX_DEPLOYMENT_TARGET=10.11 \
        ..
make -j4
if [ ! -d libcef_dll ]; then mkdir libcef_dll; fi
Maybe stupid question, but did you run the additional fix-ups for CEF (see below)? ``` sed -i '.orig' '/add_subdirectory(tests\/ceftests)/d' ./CMakeLists.txt sed -i '.orig' s/\"10.9\"/\"10.13\"/ ./cmake/cef_variables.cmake cmake \ -DCMAKE_CXX_FLAGS="-std=c++11 -stdlib=libc++"\ -DCMAKE_EXE_LINKER_FLAGS="-std=c++11 -stdlib=libc++"\ -DCMAKE_OSX_DEPLOYMENT_TARGET=10.11 \ .. make -j4 if [ ! -d libcef_dll ]; then mkdir libcef_dll; fi ```
ArtiomNosov commented 2020-10-12 09:59:59 -05:00 (Migrated from github.com)

@PatTheMav
Thank you. The instructions did say, “Tell the user to move the necessary files,” but I had no idea about the tests that weren't working. I apologize.

@PatTheMav Thank you. The instructions did say, “Tell the user to move the necessary files,” but I had no idea about the tests that weren't working. I apologize.
PatTheMav commented 2020-10-12 10:04:28 -05:00 (Migrated from github.com)

All good - the combined build script contains all the "secrets" to successfully build OBS and bundle/package it with all bells and whistles. Feel free to copy from it what you need.

You can also just use the script to build it, make code-changes and re-run the script to compile/bundle (which is necessary for CEF anyway) with just one command. It will take care of dependencies and all.

All good - the combined build script contains all the "secrets" to successfully build OBS and bundle/package it with all bells and whistles. Feel free to copy from it what you need. You can also just use the script to build it, make code-changes and re-run the script to compile/bundle (which is necessary for CEF anyway) with just one command. It will take care of dependencies and all.
PatTheMav commented 2020-11-15 12:48:00 -06:00 (Migrated from github.com)

I've played around with this a bit and IMO this piece should be removed:

github.com/obsproject/obs-browser@39f7ea62b4/CMakeLists.txt (L255)

The main reason is that CEF expects to run as part of an application bundle - it will crash otherwise (as it tries to find a bundle path and fails). So creating an app bundle is necessary to run a build with CEF included on macOS to begin with, which the build script will take care of (e.g. to create a proper bundle from an Xcode build works via BUILD_DIR="xcodebuild" BUILD_CONFIG="Debug" ./CI/full-build-macos.sh -d -s -b).

With this change, when OBS is compiled with browser support but not bundled, the app will crash as soon as a browser panel or browser source is active in the current profile.

I've played around with this a bit and IMO this piece should be removed: https://github.com/obsproject/obs-browser/blob/39f7ea62b4d7a884dc953464fd42899fb373e16a/CMakeLists.txt#L255 The main reason is that CEF expects to run as part of an application bundle - it will crash otherwise (as it tries to find a bundle path and fails). So creating an app bundle is _necessary_ to run a build with CEF included on macOS to begin with, which the build script will take care of (e.g. to create a proper bundle from an Xcode build works via `BUILD_DIR="xcodebuild" BUILD_CONFIG="Debug" ./CI/full-build-macos.sh -d -s -b`). With this change, when OBS is compiled with browser support but _not_ bundled, the app will crash as soon as a browser panel or browser source is active in the current profile.
PatTheMav commented 2020-11-16 18:38:12 -06:00 (Migrated from github.com)

Small update - I've tried the last 5 days to find the root cause of Qt resizing the browser dock to 80x80 pixels if it has been closed in a detached state and to be brutally honest, I hit a roadblock.

  • 80x80 is set as minimumSize in AddExtraBrowserDock
  • According to Qt's docs, a QtDockWidget will size itself to fit its content (it cannot be resized itself)
  • When detaching, Qt will call QCefWidgetInternal::showEvent with the size used in the docked state
  • When closing and reopening the detached dock, QCefWidgetInternal::resizeEvent is called with 80x80
  • Re-attaching the dock will size it back up into the size in the main window, calling QCefWidgetInternal::resizeEvent multiple times until it reaches the final size
  • Detaching it, makes Qt call QCefWidgetInternal::resizeEvent again with a size of 80x80

When removing the minimumSize on the dock widget, Qt will not show the detached dock. Investigating the frame stack and checking the sizes reported by QCefWidgetInternal as well as the CefBrowserHostView shows that both have a correct size, yet Qt ignores it and sets the minimumSize. The dock itself also becomes non-resizable.

Only restarting OBS makes the dock resizable again, re-attaching and detaching will keep the size, until the dock is closed in the detached state again.

This seems to require someone with more Qt/OBS UI knowledge to figure out, as I tried a number of hacks to set/reset/override the sizes at multiple events, but Qt always ignores these values or sets the dock size as well as the contained widget's sizes to 80 by 80 points before the event handlers in QCefWidgetInternal are called.

Small update - I've tried the last 5 days to find the root cause of Qt resizing the browser dock to 80x80 pixels if it has been closed in a detached state and to be brutally honest, I hit a roadblock. * 80x80 is set as `minimumSize` in `AddExtraBrowserDock` * According to Qt's docs, a `QtDockWidget` will size itself to fit its content (it cannot be resized itself) * When detaching, Qt will call `QCefWidgetInternal::showEvent` with the size used in the docked state * When closing and reopening the detached dock, `QCefWidgetInternal::resizeEvent` is called with 80x80 * Re-attaching the dock will size it back up into the size in the main window, calling `QCefWidgetInternal::resizeEvent` multiple times until it reaches the final size * Detaching it, makes Qt call `QCefWidgetInternal::resizeEvent` again with a size of 80x80 When removing the `minimumSize` on the dock widget, Qt will _not_ show the detached dock. Investigating the frame stack and checking the sizes reported by `QCefWidgetInternal` as well as the `CefBrowserHostView` shows that _both_ have a correct size, yet Qt ignores it and sets the `minimumSize`. The dock itself also becomes non-resizable. Only restarting OBS makes the dock resizable again, re-attaching and detaching will keep the size, _until_ the dock is closed in the detached state again. This seems to require someone with more Qt/OBS UI knowledge to figure out, as I tried a number of hacks to set/reset/override the sizes at multiple events, but Qt always ignores these values or sets the dock size as well as the contained widget's sizes to 80 by 80 points before the event handlers in `QCefWidgetInternal` are called.
ArtiomNosov commented 2020-11-22 07:33:32 -06:00 (Migrated from github.com)

The most unpleasant bug that I have is a crash when the program is started with browserdock initialization. Before that, I open dockbrowser and close OBS Studio after which is saved in the settings that it should open dock at startup. I am working in Xcode. I did everything for the make according to the instructions. My macOS version 10.15.7. I would like to know if you get a similar bug.

The most unpleasant bug that I have is a crash when the program is started with browserdock initialization. Before that, I open dockbrowser and close OBS Studio after which is saved in the settings that it should open dock at startup. I am working in Xcode. I did everything for the make according to the instructions. My macOS version 10.15.7. I would like to know if you get a similar bug.
zavitax commented 2020-11-27 10:44:05 -06:00 (Migrated from github.com)

re the issue where the dock widget locks to 80x80 size after undocking-closing-showing again.
here's the fix:

you have to change how BrowserDock::closeEvent behaves

instead of relying on the event propagating to the parent and doing the work, all you have to do is to add those two lines at the end of the method:

setVisible(false); // this will hide the QDockWidget
event->ignore(); // this will prevent the event from propagating where it shouldn't

with this change, everything begins behaving as it should.

here's the complete working version of the method:

void BrowserDock::closeEvent(QCloseEvent *event)
{
    OBSDock::closeEvent(event);

    if (!event->isAccepted()) {
        return;
    }

    static int panel_version = -1;
    if (panel_version == -1) {
        panel_version = obs_browser_qcef_version();
    }

    if (panel_version >= 2 && !!cefWidget) {
        cefWidget->closeBrowser();
    }

    setVisible(false); // this will hide the QDockWidget
    event->ignore(); // this will prevent the event from propagating where it shouldn't
}

you might want to put the last two lines between an #ifdef APPLE_ & #endif pair -- I haven't tested this change on Windows.

re the issue where the dock widget locks to 80x80 size after undocking-closing-showing again. here's the fix: you have to change how BrowserDock::closeEvent behaves instead of relying on the event propagating to the parent and doing the work, all you have to do is to add those two lines at the end of the method: ``` setVisible(false); // this will hide the QDockWidget event->ignore(); // this will prevent the event from propagating where it shouldn't ``` with this change, everything begins behaving as it should. here's the complete working version of the method: ``` void BrowserDock::closeEvent(QCloseEvent *event) { OBSDock::closeEvent(event); if (!event->isAccepted()) { return; } static int panel_version = -1; if (panel_version == -1) { panel_version = obs_browser_qcef_version(); } if (panel_version >= 2 && !!cefWidget) { cefWidget->closeBrowser(); } setVisible(false); // this will hide the QDockWidget event->ignore(); // this will prevent the event from propagating where it shouldn't } ``` you might want to put the last two lines between an #ifdef __APPLE___ & #endif pair -- I haven't tested this change on Windows.
zavitax commented 2020-11-27 10:51:34 -06:00 (Migrated from github.com)

one more thing

with this fix, you can remove the cefWidget->closeBrowser() call completely and allow browsers to run in the background.

my theory is that the close event propagated to CEF, and caused havoc inside. once you set event->ignore() this no longer happens

the close event starts behaving the same as hiding the dock widget from the menu

one more thing with this fix, you can remove the `cefWidget->closeBrowser()` call completely and allow browsers to run in the background. my theory is that the close event propagated to CEF, and caused havoc inside. once you set `event->ignore()` this no longer happens the close event starts behaving the same as hiding the dock widget from the menu
PatTheMav commented 2020-11-27 13:37:11 -06:00 (Migrated from github.com)

@zavitax This is great news - that fixes it indeed. Would you have any insight why this is only an issue on macOS but not on Windows? I'm trying to figure out if this will have unintended consequences on Windows where the docks work as intended right now.

@zavitax This is great news - that fixes it indeed. Would you have any insight why this is only an issue on macOS but not on Windows? I'm trying to figure out if this will have unintended consequences on Windows where the docks work as intended right now.
zavitax commented 2020-11-27 13:43:12 -06:00 (Migrated from github.com)

@PatTheMav given my extremely low mileage with macOS (just a bit over a month), I can only speculate that this is due to differences in docks implementation between the platforms, which is kind of inevitable. I would love to know more details myself.
This can really stem from CEF, Qt or macOS internals...

@PatTheMav given my extremely low mileage with macOS (just a bit over a month), I can only speculate that this is due to differences in docks implementation between the platforms, which is kind of inevitable. I would love to know more details myself. This can really stem from CEF, Qt or macOS internals...
tbodt commented 2020-11-27 14:33:44 -06:00 (Migrated from github.com)

Just rebased

Just rebased
zavitax commented 2020-11-28 07:50:14 -06:00 (Migrated from github.com)

@tbodt @WizardCM with regards to resize grip widths:

First and foremost, it is a function of a stylesheet margin: if you set dock->setStyleSheet("QDockWidget { margin: 0px 4px 4px 4px; }"); you get a large enough margin to grab onto. I'm using 4px since it corresponds to the 4 pixels used by the private QWidgetResizeHandler implementation.

Enlarging the margin without replacing QWidgetResizeHandler has no effect except a visual one.

I've also tried implementing a rudimentary QWidgetResizeHandler with #define RANGE 24 (RANGE = default size grip width) and set margin: 24px - this had the expected effect of having a much wider resize grip areas.

So, this can be easily fixed by modifying the QDockWidget theme stylesheet.

@tbodt @WizardCM with regards to resize grip widths: First and foremost, it is a function of a stylesheet margin: if you set `dock->setStyleSheet("QDockWidget { margin: 0px 4px 4px 4px; }");` you get a large enough margin to grab onto. I'm using 4px since it corresponds to the 4 pixels used by the private `QWidgetResizeHandler` implementation. Enlarging the margin without replacing `QWidgetResizeHandler` has no effect except a visual one. I've also tried implementing a rudimentary `QWidgetResizeHandler` with `#define RANGE 24` (RANGE = default size grip width) and set `margin: 24px` - this had the expected effect of having a much wider resize grip areas. So, this can be easily fixed by modifying the `QDockWidget` theme stylesheet.
tbodt commented 2020-11-28 12:15:18 -06:00 (Migrated from github.com)

I'm not convinced there needs to be a margin. Other Qt apps seem to work fine without one. The QSplitterHandle is supposed to overlap the dock widget by a bit, so mouse events near the edge get captured by the splitter handle. I checked the view hierarchy in Xcode and they do overlap, so not sure what's happening with the event handling.

I'm not convinced there needs to be a margin. Other Qt apps seem to work fine without one. The QSplitterHandle is supposed to overlap the dock widget by a bit, so mouse events near the edge get captured by the splitter handle. I checked the view hierarchy in Xcode and they do overlap, so not sure what's happening with the event handling.
zavitax commented 2020-11-28 12:34:16 -06:00 (Migrated from github.com)

Are we talking about floating or docked widgets? QSplitterHandle seems to apply to docked panels. I was addressing floating state.

Are we talking about floating or docked widgets? QSplitterHandle seems to apply to docked panels. I was addressing floating state.
tbodt commented 2020-11-28 12:35:49 -06:00 (Migrated from github.com)

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.

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.
zavitax commented 2020-11-28 12:40:17 -06:00 (Migrated from github.com)

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.

> 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 commented 2020-11-28 17:33:27 -06:00 (Migrated from github.com)

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.

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 commented 2020-11-28 19:11:25 -06:00 (Migrated from github.com)

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.

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 commented 2020-11-28 19:25:30 -06:00 (Migrated from github.com)

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?).

github.com/obsproject/obs-browser@35b26e9dc0/panel/browser-panel-client.cpp (L200-L209)

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?). https://github.com/obsproject/obs-browser/blob/35b26e9dc01221b0ab19f45a537caeee077886fb/panel/browser-panel-client.cpp#L200-L209
PatTheMav commented 2020-11-28 19:54:17 -06:00 (Migrated from github.com)

As mentioned in https://github.com/obsproject/obs-browser/pull/197#issuecomment-626829236:

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.

As mentioned in https://github.com/obsproject/obs-browser/pull/197#issuecomment-626829236: > 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 commented 2020-11-28 19:56:27 -06:00 (Migrated from github.com)

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?

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?
ArtiomNosov (Migrated from github.com) requested changes 2020-11-29 07:03:37 -06:00
panel/browser-panel.hpp
ArtiomNosov (Migrated from github.com) commented 2020-11-29 07:01:04 -06:00

#ifndef _WIN32
#include <dlfcn.h>
#endif
Because does not compile on Windows

#ifndef _WIN32 #include <dlfcn.h> #endif Because does not compile on Windows
PatTheMav commented 2020-12-03 09:19:28 -06:00 (Migrated from github.com)

@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.

@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.
PatTheMav (Migrated from github.com) reviewed 2020-12-03 09:21:01 -06:00
CMakeLists.txt
PatTheMav (Migrated from github.com) commented 2020-12-03 09:21:00 -06:00

This change must be reverted - CEF will make OBS crash if it is found and loaded without OBS being packaged into an application bundle

Adding this would prohibit debugging OBS in Xcode without bundling it manually on every build. Also this is taken care of by full-build-macos when an application bundle is created.

This change must be reverted - CEF will make OBS crash if it is found and loaded without OBS being packaged into an application bundle Adding this would prohibit debugging OBS in Xcode without bundling it manually on every build. Also this is taken care of by `full-build-macos` when an application bundle is created.
PatTheMav commented 2020-12-03 09:24:24 -06:00 (Migrated from github.com)

This needs to be changed at least for macOS:

github.com/obsproject/obs-browser@1c34fd79e4/panel/browser-panel.cpp (L83)

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.

This needs to be changed at least for macOS: https://github.com/obsproject/obs-browser/blob/1c34fd79e4fdead5c33bda0437ac40002c4cebf9/panel/browser-panel.cpp#L83 `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.
👍 1
WizardCM commented 2020-12-09 01:50:36 -06:00 (Migrated from github.com)

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

@PatTheMav I can confirm on Ubuntu 20.04 that that line requires changing too. Nice catch!
tbodt (Migrated from github.com) reviewed 2020-12-15 21:49:41 -06:00
panel/browser-panel.hpp
tbodt (Migrated from github.com) commented 2020-12-15 21:49:41 -06:00

Done. The Linux PR has a different solution for this, we can revert the changes to this file entirely if that lands.

Done. The Linux PR has a different solution for this, we can revert the changes to this file entirely if that lands.
tbodt (Migrated from github.com) reviewed 2020-12-15 21:49:57 -06:00
CMakeLists.txt
tbodt (Migrated from github.com) commented 2020-12-15 21:49:57 -06:00

Done

Done
PatTheMav commented 2020-12-16 05:13:05 -06:00 (Migrated from github.com)

I've seen @tbodt pushed some requested changes - great! Would you mind applying this change as well? macOS and Linux require it: https://github.com/obsproject/obs-browser/pull/197#issuecomment-738076375

I've seen @tbodt pushed some requested changes - great! Would you mind applying this change as well? macOS and Linux require it: https://github.com/obsproject/obs-browser/pull/197#issuecomment-738076375
tbodt commented 2020-12-16 15:31:50 -06:00 (Migrated from github.com)

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

@PatTheMav I can't reproduce the crash. Is there something special I need to do?
PatTheMav commented 2020-12-18 14:19:49 -06:00 (Migrated from github.com)

@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.

> @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.
PatTheMav commented 2020-12-22 10:19:23 -06:00 (Migrated from github.com)

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.

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 commented 2021-01-16 20:15:10 -06:00 (Migrated from github.com)

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.

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.
tbodt commented 2021-01-17 02:08:32 -06:00 (Migrated from github.com)

🚀

🚀
Sign in to join this conversation.
Reviewers
No reviewers
ArtiomNosov
Labels
Clear labels   
area/accessibility
Improves accessibility of the program

  
area/dependency
Issue is in external library or dependency, not OBS itself

  
Confirmed

   
Crash Report

   
Delayed
Merging has been delayed due to an issue

  
Design Issue
There is an issue with incorporating this pull request's design. It may take some time to finalize.

  
Duplicate

   
Feedback required
Issue does not contain enough information or is missing the template

  
Good first issue
New to contributing to OBS? Start here!

  
kind/bug
Categorizes issue or PR as related to a bug.

  
kind/cleanup
Non-breaking change which makes code smaller or more readable

  
kind/enhancement
Enhancements are not bugs or new features but can improve usability or performance.

  
platform/linux

   
platform/macos

   
platform/windows

   
priority/high
This is an important PR

  
Question

   
Request for Comments
More feedback & discussion is requested

  
Seeking Other Contributors
This PR needs help from any other developers who are willing to give a crack at this code

  
Seeking Testers

   
Won't Fix / By Design
No labels
area/accessibility
area/dependency
Confirmed
Crash Report
Delayed
Design Issue
Duplicate
Feedback required
Good first issue
kind/bug
kind/cleanup
kind/enhancement
platform/linux
platform/macos
platform/windows
priority/high
Question
Request for Comments
Seeking Other Contributors
Seeking Testers
Won't Fix / By Design
Milestone
Clear milestone
No items
No milestone
OBS Studio 27.0
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-obsproject/obs-browser!197
No description provided.