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.

Sign up for GitHub

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

Jump to bottom

Add Vertical Mixer Toggle #1257

Merged
merged 4 commits into from
May 20, 2018
Merged

Add Vertical Mixer Toggle #1257

merged 4 commits into from
May 20, 2018

Conversation

admshao
Copy link
Contributor

@admshao admshao commented Apr 10, 2018 •
edited
Loading

Add toggle to change horizontal and vertical Mixer.

screenshot from 2018-04-10 14-15-01
screenshot from 2018-04-11 09-33-31
screenshot from 2018-04-11 09-33-46
screenshot from 2018-04-11 09-59-08

@admshao admshao force-pushed the vertical-volume-meter branch 5 times, most recently from 76af689 to c031174 Compare April 17, 2018 16:27
@RytoEX
Copy link
Member

RytoEX commented Apr 25, 2018

Hi! A few things here.

  1. The first three commits seem like general code fixes/cleanup that are separate from this PR's intended purpose. Why are they included here?
  2. The fourth commit seems to make a lot of changes to the QSS theme files, at least some of which don't appear to be related to this PR's intended purpose (removing comments, changing spacing, etc.).
  3. This PR currently has merge conflicts with master, so those will need to be resolved.

@admshao
Copy link
Contributor Author

admshao commented Apr 28, 2018

  1. Usually when i tackle some feature i tend to do those fixes/cleanup on the start so that if its accepted it will be linked closer to the new feature. I think its more visible to have on the same PR but on a separeted commit than to duplicate the number of PRs.

  2. The changes are directly related to the slider but the removed comment. Done.

  3. Done.

@admshao admshao force-pushed the vertical-volume-meter branch 2 times, most recently from b380993 to 1e8b501 Compare May 8, 2018 00:30
jp9000
jp9000 reviewed May 8, 2018
View reviewed changes
Copy link
Member

@jp9000 jp9000 left a comment

Choose a reason for hiding this comment

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

These are the number of little things that could be changed. If you have time, I would advise taking a look at them and amending.

Other than than those minor commit issues, this PR looks and works great. Very well done.

UI/volume-control.hpp
@@ -2,6 +2,7 @@

#include <obs.hpp>
#include <QWidget>
#include <QPaintEvent>
Copy link
Member

Choose a reason for hiding this comment

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

QPaintEvent is not necessary in this file due to type forwarding. It will in fact slow compile time of any source file that includes this header.

UI/volume-control.hpp Outdated
~VolumeMeter();
explicit VolumeMeter(QWidget *parent = nullptr,
obs_volmeter_t *obs_volmeter = nullptr);
~VolumeMeter() override;
Copy link
Member

Choose a reason for hiding this comment

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

The override keyword is not really something that you need to use on a destructor. Destructors cannot have multiple declarations/definitions, and can't change parameters, therefore the override keyword has no real purpose on a destructor.

UI/volume-control.hpp Outdated
VolControl(OBSSource source, bool showConfig = false);
~VolControl();
explicit VolControl(OBSSource source, bool showConfig = false);
~VolControl() override;
Copy link
Member

Choose a reason for hiding this comment

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

Again, the override keyword is not really something that you need to use on a destructor. Destructors cannot change parameters.

UI/volume-control.cpp
void VolumeMeter::paintInputMeter(QPainter &painter, int x, int y,
int width, int height, float peakHold)
void VolumeMeter::paintInputMeter(QPainter &painter, int x, int y, int width,
int height, float peakHold)
Copy link
Member

Choose a reason for hiding this comment

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

This probably should have gone in the "Cleanup includes and code style" commit.

UI/volume-control.cpp Outdated
@@ -9,7 +9,6 @@
#include <QSlider>
#include <QLabel>
#include <QPainter>
#include <utility>
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this probably should have gone in the "Avoid copies" commit.

jp9000
jp9000 reviewed May 19, 2018
View reviewed changes
UI/data/themes/Acri.qss Outdated
border-radius: 3px;
width: 10px;
height: 18px;
margin: 0 -3px; /* handle is placed by default on the contents rect of the groove. Expand outside the groove */
Copy link
Member

Choose a reason for hiding this comment

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

Just a tiny indent inconsistency here, you'll notice the indent of these four lines is inconsistent with the previous lines. Make sure to double-check your commit diffs in the future.

@jp9000 jp9000 merged commit 4cbdc2d into obsproject:master May 20, 2018
@admshao admshao deleted the vertical-volume-meter branch May 21, 2018 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jp9000 jp9000 jp9000 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@admshao @RytoEX @jp9000