obs-studio icon indicating copy to clipboard operation
obs-studio copied to clipboard

Separation of the controls dock from the main window

Open tytan652 opened this issue 3 years ago • 8 comments

Description

Depends on:

  • #7169

Moves the controls dock UI outside of the main window UI. And make OBSBasic no longer rely on UI element from the controls dock.

Also adds replay buffer buttons and the pause button in the UI file, those are no longer manually and programmatically created and deleted.

Motivation and Context

This PR has two purpose:

  • RFC 47, Switch to Advanced Docking System
  • Allow replacement of this dock to be easier to implement like #6421 by relying on new signals and slots.

How Has This Been Tested?

I tried to test all the feature of the dock, but please test it yourself too.

Need also some testing about what 2d60c89a02e5f1559603ed83de08a05e2077101a was fixing because we no longer rely on UI elements.

Types of changes

  • Tweak (non-breaking change to improve existing functionality)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Refactor for future features

Checklist:

  • [x] My code has been run through clang-format.
  • [x] I have read the contributing document.
  • [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.

tytan652 avatar Sep 03 '22 08:09 tytan652

Remove from the milestone, because it is related to the RFC about MVC. https://github.com/obsproject/rfcs/pull/53

tytan652 avatar Jan 15 '23 00:01 tytan652

The PR has been entirely refactored, it still miss appearance/theming bits for the small buttons of the dock.

tytan652 avatar Apr 10 '24 13:04 tytan652

#9653 was merged, so I finished theming the small buttons.

tytan652 avatar Apr 21 '24 10:04 tytan652

Apparently I forgot to submit a comment.

About UI/noncheckable-buttons.hpp, I am a little confused by its purpose. QPushButtons have checkable set to false by default. Are we sub-classing it just to communicate "you shouldn't set checkable to true on this", but not actually preventing it from being done?

RytoEX avatar May 06 '24 22:05 RytoEX

About UI/noncheckable-buttons.hpp, I am a little confused by its purpose. QPushButtons have checkable set to false by default. Are we sub-classing it just to communicate "you shouldn't set checkable to true on this", but not actually preventing it from being done?

The name I chose was not the best, I know.

In styling we rely on the checked state, but this state is kept to false in styling if checkable is set to false, so we need the button to stay with checkable to true.

The main issue that NonCheckableButton try to solve, is that we have multiple times a situation with most of the outcomes ends up with only setChecked(false) (so undoing the automatic change done by clicking the button).

So to simplify the code, I reversed that logic and overridden nextCheckState() to not change checked each time we click on the button, to change manually checked when it really needs to be changed which happens in really fewer outcomes.

tytan652 avatar May 07 '24 06:05 tytan652

Nits, mostly.

I fixed them.

Though, I wonder if these commit messages could be more clear/expressive if they described what they were doing rather than what they were not doing.

I'll try to find better names.

tytan652 avatar May 07 '24 07:05 tytan652

@RytoEX, done

tytan652 avatar May 09 '24 19:05 tytan652

Renamed noncheckable-buttons.hpp to noncheckable-button.hpp.

tytan652 avatar May 11 '24 23:05 tytan652

After rebasing, this failed to build due to previous changes in UI/window-basic-main.cpp to void OBSBasic::StartStreaming():

	auto setStreamText = [&](const QString &text) {
		ui->streamButton->setText(text);
		if (sysTrayStream)
			sysTrayStream->setText(text);
	};

The compiler error was:

no member named 'streamButton' in 'Ui::OBSBasic'

This is obvious in hindsight because this PR moves streamButton from OBSBasic to OBSBasicControls. To get this to compile, I removed this line: https://github.com/obsproject/obs-studio/blob/0cc357f6dc3667593d182017a8074cc6ab044ee3/UI/window-basic-main.cpp#L7044

This does mean that the "Preparing..." text will no longer be shown on the "Start Stream" button. I'm not sure what to do about that.

RytoEX avatar Jun 05 '24 03:06 RytoEX

I added a commit that removes the lamdba, moreover StreamingStarting behavior got split in two by #10633 with a new setText which required moving a signal call and add a new one.

tytan652 avatar Jun 05 '24 07:06 tytan652

I added a commit that removes the lamdba, moreover StreamingStarting behavior got split in two by #10633 with a new setText which required moving a signal call and add a new one.

Thanks for that. Will re-review shortly.

RytoEX avatar Jun 05 '24 19:06 RytoEX