Fix unnecessary pixel ratio calculations on macOS

PatTheMav commented

2021-02-21 08:45:38 -06:00

(Migrated from github.com)

Description

Disables unnecessary pixel ratio calculations on macOS which led to CEF window size and Qt window size mismatches.

Fixes https://github.com/obsproject/obs-browser/issues/265.

Motivation and Context

Window sizes in macOS are automatically translated to actual pixel sizes internally by macOS, applications are required to work with "logical" sizes (aka "dots") instead. Thus applying pixel ratios as multipliers is not necessary.

How Has This Been Tested?

Applied to local build on macOS and tested with attached and detached browser panels.

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.
I have included updates to all appropriate documentation.

### Description Disables unnecessary pixel ratio calculations on macOS which led to CEF window size and Qt window size mismatches. Fixes https://github.com/obsproject/obs-browser/issues/265. ### Motivation and Context Window sizes in macOS are automatically translated to actual pixel sizes internally by macOS, applications are required to work with "logical" sizes (aka "dots") instead. Thus applying pixel ratios as multipliers is not necessary. ### How Has This Been Tested? * Applied to local build on macOS and tested with attached and detached browser panels. ### Types of changes - Bug fix (non-breaking change which fixes an issue) ### Checklist: - [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. - [x] I have included updates to all appropriate documentation.

WizardCM commented

2021-02-27 20:20:10 -06:00

(Migrated from github.com)

I'll be honest, I'm not a fan of how much code gets duplicated. The browser code is hard enough to navigate.

PatTheMav commented

2021-02-28 04:53:11 -06:00

(Migrated from github.com)

I'll be honest, I'm not a fan of how much code gets duplicated. The browser code is hard enough to navigate.

I know, but the alternative would've been even less readable imo:

void QCefWidgetInternal::Init()
{
#ifdef __APPLE__
    WId handle = winId();
#else
    WId handle = window->winId();

    QSize size = this->size();
#ifdef SUPPORTS_FRACTIONAL_SCALING
    size *= devicePixelRatioF();
#else
    size *= devicePixelRatio();
#endif
#endif

    bool success = QueueCEFTask([this, handle]() {
		CefWindowInfo windowInfo;

		/* Make sure Init isn't called more than once. */
		if (cefBrowser)
			return;
#ifdef __APPLE__
		QSize size = this->size();
		windowInfo.SetAsChild((CefWindowHandle)handle, 0, 0,
				      size.width(), size.height());
#else
#ifdef _WIN32
        RECT rc = {0, 0, size.width(), size.height()};
#else
        CefRect rc = {0, 0, size.width(), size.height()};
#endif
        windowInfo.SetAsChild((CefWindowHandle)handle, rc);
#endif

		CefRefPtr<QCefBrowserClient> browserClient =
			new QCefBrowserClient(this, script, allowAllPopups_);

		CefBrowserSettings cefBrowserSettings;
		cefBrowser = CefBrowserHost::CreateBrowserSync(
			windowInfo, browserClient, url, cefBrowserSettings,
#if CHROME_VERSION_BUILD >= 3770
			CefRefPtr<CefDictionaryValue>(),
#endif
			rqc);
	});
    
    if (success) {
        timer.stop();
#ifndef __APPLE__
		if (!container) {
			container =
				QWidget::createWindowContainer(window, this);
			container->show();
		}

		Resize();
#endif
	}
}

If you'd prefer this style, I can adapt the PR as such.

> I'll be honest, I'm not a fan of how much code gets duplicated. The browser code is hard enough to navigate. I know, but the alternative would've been even less readable imo: ``` void QCefWidgetInternal::Init() { #ifdef __APPLE__ WId handle = winId(); #else WId handle = window->winId(); QSize size = this->size(); #ifdef SUPPORTS_FRACTIONAL_SCALING size *= devicePixelRatioF(); #else size *= devicePixelRatio(); #endif #endif bool success = QueueCEFTask([this, handle]() { CefWindowInfo windowInfo; /* Make sure Init isn't called more than once. */ if (cefBrowser) return; #ifdef __APPLE__ QSize size = this->size(); windowInfo.SetAsChild((CefWindowHandle)handle, 0, 0, size.width(), size.height()); #else #ifdef _WIN32 RECT rc = {0, 0, size.width(), size.height()}; #else CefRect rc = {0, 0, size.width(), size.height()}; #endif windowInfo.SetAsChild((CefWindowHandle)handle, rc); #endif CefRefPtr<QCefBrowserClient> browserClient = new QCefBrowserClient(this, script, allowAllPopups_); CefBrowserSettings cefBrowserSettings; cefBrowser = CefBrowserHost::CreateBrowserSync( windowInfo, browserClient, url, cefBrowserSettings, #if CHROME_VERSION_BUILD >= 3770 CefRefPtr<CefDictionaryValue>(), #endif rqc); }); if (success) { timer.stop(); #ifndef __APPLE__ if (!container) { container = QWidget::createWindowContainer(window, this); container->show(); } Resize(); #endif } } ``` If you'd prefer this style, I can adapt the PR as such.

jp9000 commented

2021-03-01 10:09:04 -06:00

(Migrated from github.com)

I think the alternative is ok, it's not that much less readable. Matt probably does have a good point. If it's okay, could you use that instead?

Also, could you put the motivation and context into the commit message? It's good to have commit messages with details about the commit so it's easier to know the context of the commit if there's by some chance a bug or something in that code that we only discover later.

I think the alternative is ok, it's not that much less readable. Matt probably does have a good point. If it's okay, could you use that instead? Also, could you put the motivation and context into the commit message? It's good to have commit messages with details about the commit so it's easier to know the context of the commit if there's by some chance a bug or something in that code that we only discover later.

PatTheMav commented

2021-03-01 13:04:38 -06:00

(Migrated from github.com)

I just force-pushed another update that removed the duplication - funnily enough the diff looks "worse".

Also ran some tests if we could get away with less of those macOS-specific changes, but this (no sub-window, getting the size within the CEF task, disabling the resize function entirely) seems to be the safest way to get it to work on macOS. From what I could tell the container and sub-window were necessary for resizing support, which works out-of-the-box on macOS.

I just force-pushed another update that removed the duplication - funnily enough the diff looks "worse". Also ran some tests if we could get away with less of those macOS-specific changes, but this (no sub-window, getting the size within the CEF task, disabling the resize function entirely) seems to be the safest way to get it to work on macOS. From what I could tell the container and sub-window were necessary for resizing support, which works out-of-the-box on macOS.

WizardCM commented

2021-03-01 15:48:29 -06:00

(Migrated from github.com)

Yeah, sometimes you have to sacrifice a good diff in favour of a more readable end result.

PatTheMav commented

2021-03-03 11:20:53 -06:00

(Migrated from github.com)

@WizardCM Looks like the Ubuntu-fix for clang-format hasn't been applied on this project yet?

Rows
Columns

Fix unnecessary pixel ratio calculations on macOS #267

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist: