Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: Reorganise and refactor entire frontend codebase #11622

Open
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

PatTheMav
Copy link
Member

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 Show resolved Hide resolved
frontend/forms/OBSBasic.ui Outdated Show resolved Hide resolved
frontend/docks/YouTubeAppDock.cpp Outdated Show resolved Hide resolved
UI/api-interface.cpp Outdated Show resolved Hide resolved
frontend/OBSStudioAPI.cpp Show resolved Hide resolved
frontend/cmake/feature-macos-update.cmake Outdated Show resolved Hide resolved
frontend/cmake/os-freebsd.cmake Outdated Show resolved Hide resolved
frontend/cmake/os-linux.cmake Outdated Show resolved Hide resolved
frontend/cmake/os-windows.cmake Outdated Show resolved Hide resolved
frontend/oauth/TwitchAuth.cpp Outdated Show resolved Hide resolved
frontend/utility/MultitrackVideoOutput.hpp Outdated Show resolved Hide resolved
frontend/utility/audio-encoders.hpp Outdated Show resolved Hide resolved
frontend/utility/obf.c Outdated Show resolved Hide resolved
frontend/widgets/OBSProjector.cpp Outdated Show resolved Hide resolved
@WizardCM WizardCM added Enhancement Improvement to existing functionality Code Cleanup Non-breaking change which makes code smaller or more readable labels Dec 14, 2024
frontend/components/ColorSourceToolbar.cpp Show resolved Hide resolved
frontend/components/ComboSelectToolbar.cpp Show resolved Hide resolved
frontend/components/GameCaptureToolbar.cpp Show resolved Hide resolved
frontend/components/VisibilityItemWidget.hpp Outdated Show resolved Hide resolved
frontend/wizards/AutoConfig.cpp Outdated Show resolved Hide resolved
frontend/wizards/AutoConfig.cpp Outdated Show resolved Hide resolved
frontend/utility/BasicOutputHandler.cpp Show resolved Hide resolved
frontend/utility/BasicOutputHandler.cpp Show resolved Hide resolved
frontend/widgets/OBSBasic_SceneCollections.cpp Outdated Show resolved Hide resolved
frontend/widgets/ColorSelect.cpp Show resolved Hide resolved
frontend/components/DelButton.hpp Show resolved Hide resolved
frontend/components/EditWidget.hpp Show resolved Hide resolved
frontend/utility/ExtraBrowsersDelegate.hpp Show resolved Hide resolved
frontend/dialogs/OBSMissingFiles.cpp Show resolved Hide resolved
frontend/utility/BaseLexer.hpp Show resolved Hide resolved
frontend/components/SilentUpdateSpinBox.hpp Show resolved Hide resolved
frontend/utility/OBSEventFilter.hpp Show resolved Hide resolved
frontend/utility/SettingsEventFilter.hpp Show resolved Hide resolved
frontend/settings/OBSBasicSettings.hpp Show resolved Hide resolved
frontend/components/VolumeSlider.cpp Show resolved Hide resolved
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Finished a first pass of all files.

frontend/oauth/RestreamAuth.cpp Outdated Show resolved Hide resolved
frontend/oauth/YoutubeAuth.cpp Outdated Show resolved Hide resolved
frontend/utility/FFmpegFormat.cpp Show resolved Hide resolved
frontend/utility/OBSTranslator.cpp Show resolved Hide resolved
frontend/utility/OBSTranslator.hpp Show resolved Hide resolved
frontend/widgets/OBSBasic_YouTube.cpp Outdated Show resolved Hide resolved
frontend/widgets/OBSQTDisplay.cpp Show resolved Hide resolved
frontend/dialogs/OBSBasicInteraction.cpp Show resolved Hide resolved
frontend/dialogs/OBSWhatsNew.cpp Outdated Show resolved Hide resolved
frontend/obs-main.cpp Outdated Show resolved Hide resolved
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Mostly questions about WIN32_LEAN_AND_MEAN placement.

frontend/OBSApp.cpp Outdated Show resolved Hide resolved
frontend/OBSApp.cpp Show resolved Hide resolved
frontend/obs-main.cpp Outdated Show resolved Hide resolved
frontend/utility/AutoUpdateThread.cpp Outdated Show resolved Hide resolved
frontend/utility/platform-osx.mm Show resolved Hide resolved
frontend/utility/platform-windows.cpp Show resolved Hide resolved
frontend/widgets/OBSBasic.cpp Outdated Show resolved Hide resolved
frontend/widgets/OBSQTDisplay.cpp Show resolved Hide resolved
@PatTheMav PatTheMav force-pushed the frontend-refactor branch 2 times, most recently from b2000e6 to fa7398e Compare January 8, 2025 15:16
@RytoEX RytoEX added the Seeking Testers Build artifacts on CI label Jan 8, 2025
@RytoEX
Copy link
Member

RytoEX commented Jan 8, 2025

Builds and runs locally on Windows 11 when building with VS 2022 17.11.6. Basic Profile and Scene Collection CRUD operations seem to work. The UI seems to work as expected (basic checks of controls, Settings UI, Context Bar). Additional testing welcome.

@RytoEX RytoEX mentioned this pull request Jan 8, 2025
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Cleanup Non-breaking change which makes code smaller or more readable Enhancement Improvement to existing functionality Seeking Testers Build artifacts on CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants