UI: Reorganise and refactor entire frontend codebase #11622

PatTheMav · 2024-12-11T22:19:27Z

Description

This PR reorganises the frontend codebase to enable a switch of the project to common good practices for development large GUI applications.

The actual implementations of frontend functionality are unchanged, with the exception of cases where the new organisation of source files required them to get the project in a compilable state again.

Caution

This PR must be merged via a merge commit, as extra effort has been taken to ensure the blame history of the new files will be able to access the history of the old files. A rebase will prohibit git from figuring out these connections.

Motivation and Context

The frontend of OBS Studio is written in C++ and based on Qt6, for which good practices have evolved that are distinct from development practices common in library or pure C-based development. These practices include approaches to file naming, file structuring, class design, and header design, with the goal of improving maintainability and discoverability, as well as speed improvements for incremental builds and compiler performance.

For this refactor, the following set of rules were established and applied to the entire frontend codebase:

Frontend directory structure follows the concept of "building blocks" that make up the entirety of the application, with each class or interface put in a corresponding subdirectory
C++ class names use CapitalCase, class methods use camelCase
C++ class source files carry the name of their classes
C++ class headers contain only their own class' definition
C++ class headers only include other classes' interfaces (via their own header files) that the compiler needs, otherwise forward declarations are used

Additional rules that should be established but haven't been applied in this PR are:

Important

The following rules should be followed moving forward but have not been applied to this PR yet to keep the amount of actual code changes to a very low level.

Those changes will be applied in a later PR.

Free-standing functions are put in an anonymous namespace (the suggested practice for C++ code instead of static functions
- This also solves the issue of free-standing functions being sprinkled throughout the source code and puts them in a single self-contained space. This also makes it obvious which utility functions might already exist and re-use them instead of adding yet another variant as a new static function in another translation unit.
The inline decoration should be used according to its actual meaning in C++: That a function definition is provided "in line" (it has no direct bearing on the code inlining behaviour of compilers). As such it should never(!) be used in source files and should only ever be used in header files, and even there it should be used with all its associated caveats applied.
Free-standing functions should use camelCase, not snake_case (which instead should be used to clearly identify C functions vs C++ functions)
Avoid unnecessary abbreviations, clearly spell out words (e.g. no AdvAudio, but AdvancedAudio).
- This applies not only to class names but also function/method names
Use a single variant of capitalisation (not Youtube and YouTube in the same codebase
Don't use ambiguous method names like Active or Nudge, make their purpose explicit by their name itself, e.g. nudgePreviewItem or isOutputActive
- Use Get, Set, and Is prefixes to clearly communicate whether a method gets, sets, or gets a state/status.
Don't rely on magic global variables or extern symbols that also "magically" have to exist in the global namespace. Encapsulate data with functionality and use APIs to access data or delegate functionality to encapsulated classes entirely.

Moving forward the principle to Respect Levels Of Abstraction should be followed not only in code architecture but also method naming.

Notably there is no good reason to update all existing code to these practices, instead they
will need to be applied to any new code and to changes made to existing code.

Note

As this PR does not rename actual classes or change implementations, some idiosyncrasies of the current code base are made more apparent by this change:

Some classes carry an OBS or OBSBasic prefix, some carry no such prefix at all. Some classes have overlay generic names (e.g. DelButton, or WorkerThread). This is by design as it will hopefully spur maintainers to clean this up if only to achieve consistency in class naming.

Notably these changes also have a positive effect on incremental compile times: Header files should be seen as "class interface definitions" that are needed for code to understand the memory layout and API of another class. Such headers should only be included in other headers sparingly and instead be limited to source files as best as possible.

A single header file per class also avoids the issue of triggering recompilation of a whole set of source code just because one of many class definitions in the same header has been changed. This also improves parallelisation in build tools and improves speed of recompilation efforts by Qt's moc precompiler (as source and header files themselves become smaller).

Unfortunately the single massive OBSBasic class with its equally massive header file persists, which is also included by almost all other frontend code in one form or another. A better architecture for the frontend code could avoid this pitfall, but for the time being a single change in OBSBasic's header will trigger recompilation of a large amount of frontend code.

How Has This Been Tested?

Successfully compiled and ran OBS Studio on macOS 15, Windows 11, and Ubuntu 24.04 with browser sources enabled and disabled and service integrations enabled as well as disabled.

Types of changes

Code cleanup (non-breaking change which makes code smaller or more readable)

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.

frontend/OBSStudioAPI.hpp

RytoEX · 2024-12-12T23:09:56Z

frontend/forms/OBSBasic.ui

-              <string>label-preview-title</string>
+              <string>previewProgramLabels</string>


@Warchamp7 might have an opinion on this, but I think CSS/QSS class names should be singular (not plural). Also, if changing this, wouldn't the class rule in the theme files need to be changed? Or was this changed by mistake?

This string is in essence a CSS class name yes. Changing this would necessitate changes to the obt/ovt files and could be a breaking change to any custom themes people have made.

It is pretty common to use kebab-case for CSS classes and I have tried to be consistent about naming schemes.

I think that's an automatic change through rebasing though, the only classes that needed to be changed are the actual C++ class names in the customwidgets tree.

I'm not sure I follow.

There's a distinction here between the C++ class the widget is tied to, and the dynamic property class on the widget, which is used for styling purposes.

The thing changed here is a dynamic property that seems like an accidental modification.

Existing line on master
The styling rule in our obt file

That's what I'm saying, yes. The line should not have changed and the only reason it did was because git was able to apply the change when I rebased it to master.

RytoEX · 2024-12-12T23:21:01Z

frontend/docks/YouTubeAppDock.cpp

@@ -29,6 +34,7 @@ static constexpr const char *INGESTION_STARTED = "INGESTION_STARTED";
 static constexpr const char *INGESTION_STOPPED = "INGESTION_STOPPED";

 YouTubeAppDock::YouTubeAppDock(const QString &title) : BrowserDock(title), dockBrowser(nullptr)
+


This linebreak seems accidental.

derrod · 2024-12-13T03:48:58Z

UI/api-interface.cpp

rip the most upsettingly named file in the entire program.

frontend/OBSStudioAPI.cpp

RytoEX · 2024-12-13T19:59:04Z

frontend/cmake/feature-macos-update.cmake

@@ -0,0 +1,27 @@
+include_guard(DIRECTORY)
+
+find_package(nlohmann_json REQUIRED)


Suggested change

find_package(nlohmann_json REQUIRED)

find_package(nlohmann_json 3.11 REQUIRED)

This is the line in the original file.

That looks like a error from rebasing, was too easy for git to just remove it again.

RytoEX · 2024-12-13T20:01:32Z

frontend/cmake/os-freebsd.cmake

+  find_package(Python REQUIRED COMPONENTS Interpreter Development)
+  target_link_libraries(obs-studio PRIVATE Python::Python)
+  target_link_options(obs-studio PRIVATE LINKER:-no-as-needed)
+endif()


This file is missing the additions from 7e1e60c (#11410).

RytoEX · 2024-12-13T20:04:25Z

frontend/cmake/os-linux.cmake

 target_compile_definitions(
  obs-studio
-  PRIVATE OBS_INSTALL_PREFIX="${OBS_INSTALL_PREFIX}" $<$<BOOL:${ENABLE_PORTABLE_CONFIG}>:ENABLE_PORTABLE_CONFIG>
+  PRIVATE USE_XDG OBS_INSTALL_PREFIX="${OBS_INSTALL_PREFIX}" $<$<BOOL:${ENABLE_PORTABLE_CONFIG}>:ENABLE_PORTABLE_CONFIG>


USE_XDG was removed in c928fac (#11409).

RytoEX · 2024-12-13T20:07:36Z

frontend/cmake/os-windows.cmake

-find_package(nlohmann_json 3.11 REQUIRED)
+find_package(nlohmann_json REQUIRED)


Are we removing this version requirement on purpose?

RytoEX · 2024-12-13T22:15:24Z

frontend/oauth/TwitchAuth.cpp

+#include <ui-config.h>
 #include <qt-wrappers.hpp>


Suggested change

#include <ui-config.h>

#include <qt-wrappers.hpp>

#include <qt-wrappers.hpp>

#include <ui-config.h>

If alphabetizing, use this order. Otherwise, ignore.

RytoEX · 2024-12-13T22:22:34Z

frontend/utility/MultitrackVideoOutput.hpp

+class QWidget;
 class QString;


Swap if alphabetizing. Ignore if this order is specific.

RytoEX · 2024-12-13T22:25:16Z

frontend/utility/audio-encoders.hpp

 #include <vector>
+#include <string>


Suggested change

#include <vector>

#include <string>

#include <string>

#include <vector>

RytoEX · 2024-12-13T22:29:30Z

frontend/utility/obf.c

-#include "obf.h"
 #include <stdbool.h>

+#include "obf.h"


I thought the expected order here was:

corresponding .h file

stdlib

See frontend/utility/audio-encoders.cpp for example.

RytoEX · 2024-12-13T22:35:07Z

frontend/widgets/OBSProjector.cpp

+#include <utility/platform.hpp>
+#include <widgets/OBSBasic.hpp>
+
+#include "OBSProjector.hpp"


Wouldn't this usually be the first include?

frontend/components/ColorSourceToolbar.cpp

frontend/components/ComboSelectToolbar.cpp

frontend/components/GameCaptureToolbar.cpp

RytoEX · 2024-12-16T21:46:56Z

frontend/components/VisibilityItemWidget.hpp

+class OBSSourceLabel;
 class QCheckBox;
+
 class OBSSourceLabel;


OBSSourceLabel is here twice.

RytoEX · 2024-12-16T21:51:44Z

frontend/wizards/AutoConfig.cpp

+#include "AutoConfigStreamPage.hpp"
+#include "AutoConfigStartPage.hpp"


Suggested change

#include "AutoConfigStreamPage.hpp"

#include "AutoConfigStartPage.hpp"

#include "AutoConfigStartPage.hpp"

#include "AutoConfigStreamPage.hpp"

If alphabetical. If other order, ignore.

RytoEX · 2024-12-16T21:51:58Z

frontend/wizards/AutoConfig.cpp

+#include "ui_AutoConfigVideoPage.h"
+#include "ui_AutoConfigStreamPage.h"


Suggested change

#include "ui_AutoConfigVideoPage.h"

#include "ui_AutoConfigStreamPage.h"

#include "ui_AutoConfigStreamPage.h"

#include "ui_AutoConfigVideoPage.h"

If alphabetical. If other order, ignore.

frontend/utility/BasicOutputHandler.cpp

RytoEX · 2024-12-16T22:28:22Z

frontend/widgets/OBSBasic_SceneCollections.cpp

+#include <vector>
+#include <string>


Suggested change

#include <vector>

#include <string>

#include <string>

#include <vector>

frontend/widgets/ColorSelect.cpp

frontend/components/DelButton.hpp

frontend/components/EditWidget.hpp

frontend/utility/ExtraBrowsersDelegate.hpp

frontend/dialogs/OBSMissingFiles.cpp

RytoEX · 2024-12-17T20:27:23Z

frontend/utility/BaseLexer.hpp

@@ -0,0 +1,12 @@
+#pragma once


Missing license header?

RytoEX · 2024-12-17T20:49:09Z

frontend/components/SilentUpdateSpinBox.hpp

@@ -0,0 +1,15 @@
+#pragma once


Missing license headers.

RytoEX · 2024-12-17T20:50:10Z

frontend/utility/OBSEventFilter.hpp

@@ -0,0 +1,18 @@
+#pragma once


Missing license headers.

RytoEX · 2024-12-17T20:52:51Z

frontend/utility/SettingsEventFilter.hpp

@@ -0,0 +1,33 @@
+#pragma once


Missing license header.

RytoEX · 2024-12-17T20:54:15Z

frontend/settings/OBSBasicSettings.hpp

@@ -171,6 +136,8 @@ class OBSBasicSettings : public QDialog {
 	void SaveEncoder(QComboBox *combo, const char *section, const char *value);

 	bool ResFPSValid(obs_service_resolution *res_list, size_t res_count, int max_fps);
+
+	// TODO: Remove, orphaned method


Just leaving a review note to raise awareness of this todo.

frontend/components/VolumeSlider.cpp

RytoEX

Finished a first pass of all files.

RytoEX · 2024-12-19T19:04:09Z

frontend/oauth/RestreamAuth.cpp

+#include <ui-config.h>
 #include <qt-wrappers.hpp>


Suggested change

#include <ui-config.h>

#include <qt-wrappers.hpp>

#include <qt-wrappers.hpp>

#include <ui-config.h>

RytoEX · 2024-12-19T19:04:33Z

frontend/oauth/YoutubeAuth.cpp

+#include <ui-config.h>
+#include <qt-wrappers.hpp>


Suggested change

#include <ui-config.h>

#include <qt-wrappers.hpp>

#include <qt-wrappers.hpp>

#include <ui-config.h>

RytoEX · 2024-12-19T19:27:40Z

frontend/utility/FFmpegFormat.cpp

+vector<FFmpegFormat> GetSupportedFormats()
+{
+	vector<FFmpegFormat> formats;
+	const AVOutputFormat *output_format;


Surprised the avformat.h include wasn't needed for this.

RytoEX · 2024-12-19T19:35:24Z

frontend/utility/OBSTranslator.cpp

@@ -0,0 +1,18 @@
+#include "OBSTranslator.hpp"


OBSTranslator comes from UI\obs-app.cpp/hpp which has a license header.

RytoEX · 2024-12-19T19:35:30Z

frontend/utility/OBSTranslator.hpp

@@ -0,0 +1,14 @@
+#pragma once


OBSTranslator comes from UI\obs-app.cpp/hpp which has a license header.

RytoEX · 2024-12-19T20:51:05Z

frontend/widgets/OBSBasic_YouTube.cpp

+#include <docks/YouTubeAppDock.hpp>
+#include <dialogs/OBSYoutubeActions.hpp>


Suggested change

#include <docks/YouTubeAppDock.hpp>

#include <dialogs/OBSYoutubeActions.hpp>

#include <dialogs/OBSYoutubeActions.hpp>

#include <docks/YouTubeAppDock.hpp>

Unless include order is specific.

frontend/widgets/OBSQTDisplay.cpp

RytoEX · 2024-12-19T20:58:55Z

frontend/dialogs/OBSBasicInteraction.cpp

+#include <utility/OBSEventFilter.hpp>
+#include <utility/display-helpers.hpp>


Other files seem to do case-insensitive alphabetization. I don't yet have a strong preference, so I just wanted to see if there is an intended order.

RytoEX · 2024-12-19T21:01:12Z

frontend/dialogs/OBSWhatsNew.cpp

+#include <QPushButton>
+#include <QHBoxLayout>
+#include <QVBoxLayout>


Suggested change

#include <QPushButton>

#include <QHBoxLayout>

#include <QVBoxLayout>

#include <QHBoxLayout>

#include <QPushButton>

#include <QVBoxLayout>

If alphabetical is preferred, order like this.

RytoEX · 2024-12-19T21:22:36Z

frontend/obs-main.cpp

+#include <utility/platform.hpp>
+#include <dialogs/OBSLogViewer.hpp>


Suggested change

#include <utility/platform.hpp>

#include <dialogs/OBSLogViewer.hpp>

#include <dialogs/OBSLogViewer.hpp>

#include <utility/platform.hpp>

Alphabetize unless order is intentional.

This commit only contains Qt UI components that are self-contained, i.e. the translation units only contain code for a single class or interface and don't mix implementations.

frontend: Add new frontend directory to gitignore file

11ae694

PatTheMav force-pushed the frontend-refactor branch from 136fe35 to fa07b37 Compare December 12, 2024 13:24

RytoEX requested changes Dec 12, 2024

View reviewed changes

derrod reviewed Dec 13, 2024

View reviewed changes

UI/api-interface.cpp Outdated

Copy link

Member

derrod Dec 13, 2024

Choose a reason for hiding this comment

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

rip the most upsettingly named file in the entire program.

Warchamp7 reacted with laugh emoji

RytoEX requested changes Dec 13, 2024

View reviewed changes

WizardCM added Enhancement Improvement to existing functionality Code Cleanup Non-breaking change which makes code smaller or more readable labels Dec 14, 2024

RytoEX requested changes Dec 16, 2024

View reviewed changes

RytoEX requested changes Dec 17, 2024

View reviewed changes

RytoEX requested changes Dec 19, 2024

View reviewed changes

PatTheMav added 20 commits December 23, 2024 17:52

frontend: Add renamed Qt UI components

496a505

This commit only contains Qt UI components that are self-contained, i.e. the translation units only contain code for a single class or interface and don't mix implementations.

frontend: Prepare Qt UI components for splits

4fdaaea

frontend: Split Qt UI component into single file per C++ class

830c5de

frontend: Add renamed Qt UI dialogs

5060870

frontend: Prepare Qt UI dialogs for splits

388c36e

frontend: Split Qt UI dialogs into single files per C++ class

8699d3d

frontend: Add renamed Qt UI docks

a2743c5

frontend: Add OBSImporter and importer implementations

232ae79

frontend: Split OBSImporter into single files per C++ class

8e42c44

frontend: Add renamed OAuth implementation

304f496

frontend: Prepare OAuth implementation for splits

9a330ce

frontend: Split OAuth implementation into single files per C++ class

33825bb

frontend: Add renamed Qt Settings UI implementation

1f91f4b

frontend: Prepare Qt Settings UI files for splits

f5845b8

frontend: Split Settings implementation into single files per C++ class

d4def7f

frontend: Add renamed UI utility classes and functions

22962b4

frontend: Prepare UI utility files for splits

b63fda6

frontend: Split UI utility implementation into single files per class

a7b34c5

frontend: Add renamed Qt UI Widgets

95b5859

frontend: Prepare Qt UI Widgets for splits

05273f8

PatTheMav added 17 commits December 23, 2024 17:57

frontend: Split Qt UI Widget implementations into single files per class

88f078f

frontend: Prepare Qt UI autoconfig wizards for splits

c8cc0e9

frontend: Split Qt UI autoconfig implementations into single files

6c136f5

frontend: Prepare main application implementation for splits

53550c2

frontend: Split main application implementation into single files

7006714

frontend: Migrate Windows updater

c72978c

frontend: Migrate CMake files

623a15f

frontend: Migrate obs-frontend-api

31653e9

frontend: Migrate OBS Studio data directory

ca586f1

frontend: Migrate Qt UI files

538ff61

frontend: Migrate frontend plugins

2d79e8f

cmake: Update main CMakeLists file to use refactored frontend

3eb192a

build-aux: Replace UI directory with frontend directory for formatters

d1c012c

CI: Update actions and build scripts to use new frontend directory

91e9061

frontend: Prepare sources for merge with OBSBasic modules

ada6c24

frontend: Merge main OBSBasic source with module sources

a390a91

frontend: Finalize merge of OBSBasic Sources with module sources

2ef2956

PatTheMav force-pushed the frontend-refactor branch from fa07b37 to 2ef2956 Compare December 23, 2024 17:14

frontend: Explicitly include shellapi header for shell functions

bc8b6b0

		<string>label-preview-title</string>
		<string>previewProgramLabels</string>

		@@ -29,6 +34,7 @@ static constexpr const char *INGESTION_STARTED = "INGESTION_STARTED";
		static constexpr const char *INGESTION_STOPPED = "INGESTION_STOPPED";

		YouTubeAppDock::YouTubeAppDock(const QString &title) : BrowserDock(title), dockBrowser(nullptr)

		@@ -0,0 +1,27 @@
		include_guard(DIRECTORY)

		find_package(nlohmann_json REQUIRED)

	find_package(nlohmann_json REQUIRED)
	find_package(nlohmann_json 3.11 REQUIRED)

		#include "AutoConfigStreamPage.hpp"
		#include "AutoConfigStartPage.hpp"

		#include "ui_AutoConfigVideoPage.h"
		#include "ui_AutoConfigStreamPage.h"

		#include <docks/YouTubeAppDock.hpp>
		#include <dialogs/OBSYoutubeActions.hpp>

		#include <utility/OBSEventFilter.hpp>
		#include <utility/display-helpers.hpp>

		#include <utility/platform.hpp>
		#include <dialogs/OBSLogViewer.hpp>

UI: Reorganise and refactor entire frontend codebase #11622

Are you sure you want to change the base?

UI: Reorganise and refactor entire frontend codebase #11622

Conversation

PatTheMav commented Dec 11, 2024

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

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

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

RytoEX 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