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

Redesign of the Controller Rack #2944

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

Conversation

BaraMGB
Copy link
Contributor

@BaraMGB BaraMGB commented Jul 28, 2016

Peak controller and LFO controller will be inserted directly in the controller Rack. So we can avoid a lot of floating windows. In my opinion the rack makes more sense in this way.

controllerrack1

int titleBarHeight = 24;
QPainter p( this );
QRect rect( 1, 1, width()-2, titleBarHeight + 5 );
QLinearGradient grad( QPoint( 0, 0 ), QPoint( 0, 24 ) );
Copy link
Member

Choose a reason for hiding this comment

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

This whole thing could be avoided if you just use the background property in css and define it in css. In the end you'd set the rectrangle fill to

p.fillRect( rect, painter.background() );

@Umcaruje
Copy link
Member

I think the controllers should be centered in the window.

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jul 28, 2016

I think the controllers should be centered in the window.

I try to change the width() of the window depending of needing a scrollbar or not.

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jul 31, 2016

I updated this. The window width relates now on the need of a scrollbar.

this, SLOT( closeEffects() ) );

m_subWindow->hide();
if( !(m_controlView->windowTitle() == "Peak Controller") )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should find a more elegant way to identify the peak controller.

Copy link
Member

Choose a reason for hiding this comment

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

more elegant way to identify the peak controller

Research points to dynamic_cast against PeakController and checking for != NULL

@@ -111,7 +112,7 @@ EffectView::EffectView( Effect * _model, QWidget * _parent ) :
m_controlView = effect()->controls()->createView();
if( m_controlView )
{
if( !(m_controlView->windowTitle() == "Peak Controller") )
if( dynamic_cast<PeakControllerEffectControlDialog*>( m_controlView ) == NULL )
Copy link
Contributor Author

@BaraMGB BaraMGB Aug 2, 2016

Choose a reason for hiding this comment

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

I had to remove the Q_OBJECT macro from PeakControllerEffectDialog otherwise I get a linker error. Actually the Q_OBJECT macro isn't needed because there is no Signal/Slot stuff in this class. and everything works fine. But eventually someone can look into this.

@BaraMGB BaraMGB changed the title Redesign of the Controller Rack [WIP] Redesign of the Controller Rack Aug 10, 2016
@BaraMGB
Copy link
Contributor Author

BaraMGB commented Aug 10, 2016

This one will be not ready before 1.2 is released. I'm targeting 1.3.

@miketurn
Copy link
Contributor

@BaraMGB
Pretty cool, thank you for working on this.

#3235
Not sure if this will affect you but I figured I would mention it

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jan 13, 2017

Yes, I made a PR for this : #3240

@miketurn
Copy link
Contributor

@BaraMGB
I had a couple of ideas/questions to share (please ignore them if they are not helpful)
Some of these might be duplicates of above comments, don't understand coding.

1.) Currently you can change the names of the controllers, will you still be able to offer this?
2.) Would it be possible to be able to move items up and down? (Preferably through a mouse button drag method)
3.) At first I was going to ask if the expand/collapse arrows are necessary, but I guess it couldn't hurt to have them, they will probably would help in keeping the list clean.
4.) If these arrows do remain, would it be possible to add options to "expand all" and "collapse all"?
That might be helpful.

Again I have not used this feature much yet so you and others would be able to answer if these above questions are helpful or not.
Anyways, hope this helps. Thanks again for working on this.

@karmux
Copy link
Contributor

karmux commented Aug 25, 2019

@BaraMGB works perfectly now! 👍

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Aug 27, 2019

@PhysSong do you have time for a look at this?

src/gui/widgets/ControllerRackView.cpp Outdated Show resolved Hide resolved
include/ControllerView.h Outdated Show resolved Hide resolved
include/ControllerView.h Outdated Show resolved Hide resolved
plugins/peak_controller_effect/CMakeLists.txt Outdated Show resolved Hide resolved
src/gui/widgets/EffectView.cpp Outdated Show resolved Hide resolved
@PhysSong
Copy link
Member

I think you should export ControllerRackView class in order to make this work on Windows.

-class ControllerRackView : public QWidget, public SerializingObject
+class LMMS_EXPORT ControllerRackView : public QWidget, public SerializingObject

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Aug 29, 2019

I guess the main problem I have with this pull request is located in the PeakControllerDialog.cpp. I had to include:
#include "plugins/peak_controller_effect/peak_controller_effect_control_dialog.cpp"
I think to include a .cpp file is bad style. But I got a linker error if I include the .h instead.

Furthermore, I had to remove the Q_OBJECT macro from the PeakControllerEffectControlDialog class because I received further linker errors.

I don't know how to solve these problems. I would be very happy about help.

@PhysSong
Copy link
Member

@BaraMGB I will try to help you this weekend.

@LMMS LMMS deleted a comment from LmmsBot Sep 28, 2019
@LMMS LMMS deleted a comment from LmmsBot Sep 28, 2019
@tecknixia
Copy link
Contributor

tecknixia commented Oct 14, 2019

Linux Mint 64

I had a lot of trouble setting this up, maybe because I don't know what I'm doing yet. I do have a working version on my computer now.

I cloned LMMS/lmms and rebased from this branch. Had trouble with cmake, and had to resolve conflicts during the rebase.

I had to add QWhatsThis and displayHelp() to ControllerView.h before Qt would compile it.

Also got a message when first compiling, something about no classes for
plugins/peak_controller_effect/peak_controller... something or other.


#include "EffectControlDialog.h"
#include "plugins/peak_controller_effect/peak_controller_effect.h"
#include "plugins/peak_controller_effect/peak_controller_effect_control_dialog.cpp"
Copy link

Choose a reason for hiding this comment

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

Can't you include "embed.h" and <QIcon> and then change line 42 to:

	setWindowIcon( QIcon(embed::getIconPixmap( "controller" )) );

instead of including peak_controller_effect_control_dialog.cpp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand. Please, can you elaborate? Have you tested your suggestion?

Copy link

Choose a reason for hiding this comment

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

Yes when I remove the .cpp include it fails compiling with: error: 'embed' has not been declared.

So I added #include "embed.h". Then it fails because setWindowIcon aspects a QIcon but a QPixmap is given.

So that's why also included <QIcon> and changed the setWindowIcon line.

After that it compiles and works fine here.

@tecknixia
Copy link
Contributor

tecknixia commented Oct 15, 2019

Also, not sure if I made a mistake when resolving conflicts... (I remember being unsure about one line of code) but I can confirm, as mentioned by @zonkmachine in #3240 that it's possible to make the Controller Rack window disappear by grabbing the bottom frame and moving up.

Edit ----

So I started experimenting a bit and came up with this line:
m_subWin->setMinimumSize(m_subWin->size());

To be placed after the last command in:
ControllerRackView::ControllerRackView()

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Oct 15, 2019

@tecknixia probably it's simpler to checkout the branch directly instead of trying to merge it into your master? To fix merge conflicts can be hard.

@tecknixia
Copy link
Contributor

tecknixia commented Oct 23, 2019

@BaraMGB oh... it's a branch... that makes sense 🤣 newbie mistake.

@sound8
Copy link

sound8 commented Oct 31, 2020

Man, this looks great, does anyone know if this is still planned on being added to LMMS?

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Nov 1, 2020

I wrote this 4 years ago. This is really old code now and it would be complicated to understand what I made. If lmms want to have a single window interface, something like this is needed to catch all the floating controler windows (MDI subwindows) in a rack.
#1911

Unfortunately I don't have the time to bring this to an end. I would be happy if someone would like to take over the project.

@Spekular Spekular added the after-refactor PRs that will need to be rebased after the planned code refactor (#5592) label Jan 5, 2021
@Rossmaxx
Copy link
Contributor

now that the rework is about to finish, i think it's about time to pick this back up.

This was referenced Apr 27, 2022
@PhysSong
Copy link
Member

I have resolved conflicts and also made minor code improvements on my fork. @BaraMGB Do you want to enable "Allow edits from maintainers", or should I close this and open a new one? If you want, you can also pull from my fork.

@zonkmachine
Copy link
Member

@BaraMGB Do you want to enable "Allow edits from maintainers", or should I close this and open a new one? If you want, you can also pull from my fork.

OK. No answer so I guess @BaraMGB is busy and you can close this PR and open a new one. I'd love to see the controllers sorted in one window.

@Rossmaxx Rossmaxx modified the milestones: 1.3, 1.3+ Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
after-refactor PRs that will need to be rebased after the planned code refactor (#5592) gui needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR needs testing This pull request needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.