Add a new window decoration to MDISubWindows #2516

Merged
merged 2 commits into from May 3, 2016

Projects

None yet
@BaraMGB
Contributor
BaraMGB commented Jan 25, 2016

Okay, this is only a basis for a discussion. Reference to #2450

Greetings, Steffen

@circuitfry

I'll at least try to help dress it up nice and pretty with some suggestions by referring to the coding conventions. https://github.com/LMMS/lmms/wiki/Coding-conventions

*** please note that I am referring to your commit's new line numbers!

My suggestions for SubWindow.h:

  • Please swap lines 31 and 32 so that they are sorted by name. Thank you.
  • Please move line 29 so that it comes two lines after line 32. I noticed your intentions. Thank you.
  • This isn't your fault, but could you please put spaces around the = signs on line 44? Thank you.

And for SubWindow.cpp:

  • Please sort lines 29 through 34 by name and move line 35 to line 36. Thank you.
  • Forgive me if I'm mistaken, but if lines 53 and 56 are objects, please remove the space between the name of the objects and their respective opening parenthesis. Thank you.
  • From line 54 and onward, please add spaces after the commas in the parameters to make them more readable. Thank you.
  • Please break line 63 into two lines so that the length doesn't go beyond 80 characters. Thank you.
  • Lines 53 and 64 appear to be hard-coding colors. I understand that we're still working on CSS implementation. I'm listing it here as part of this to-do list for convenience sake.
  • Please add spaces around the -> in line 67, if possible. Thank you.
  • Please decrease the indentation of lines 70 through 75 and put spaces around = signs. Thank you.
  • This isn't your fault, but could you fix the spacing in line 102? Please remove the space between "if" and "(". Thank you.
  • Line 104 is having coding convention troubles, if you would please. Also not your fault. Thank you.

Hope this helps! Thank you.

@BaraMGB
Contributor
BaraMGB commented Jan 27, 2016

Okay, I've changed the suggestions you gave. Thank you.

@Umcaruje
Member

screenshot from 2016-01-27 14 07 01

I fiddled around with your code, and paired it up with a custom theme + color + icons, and it looks amazing!

Here's the stuff I changed:

  • I defined a new Qcolor called c, and used it to easily change the color:
QColor c("#2c974f");
QBrush brush(c);
  • I also set the pen to Qt::NoPen, so the windows are borderless. I also commented out lines 60, 61 and 62, since they were no longer needed.
  • I also changed the QRectF rect(0,0,width()-1,20); to QRectF rect(0,0,width(),20);, to work nicely with the borderless design.
  • Made new pixel perfect icons in inkscape.
    Get them: icons.zip
@cubician
Contributor

That's gorgeous!
On Jan 27, 2016 6:26 AM, "Umcaruje" notifications@github.com wrote:

[image: screenshot from 2016-01-27 14 07 01]
https://cloud.githubusercontent.com/assets/6282045/12614537/db4a5352-c500-11e5-922d-e1782b0c74fd.png

I fiddled around with your code, and paired it up with a custom theme +
color + icons, and it looks amazing!

Here's the stuff I changed:

  • I defined a new Qcolor called c, and used it to easily change the
    color:

QColor c("#2c974f");
QBrush brush(c);

I also set the pen to Qt::NoPen, so the windows are borderless. I also
commented out lines 60, 61 and 62, since they were no longer needed.

I also changed the QRectF rect(0,0,width()-1,20); to QRectF
rect(0,0,width(),20);, to work nicely with the borderless design.

*Made new pixel perfect icons in inkscape.
Get them: icons.zip https://github.com/LMMS/lmms/files/106637/icons.zip


Reply to this email directly or view it on GitHub
#2516 (comment).

@zonkmachine
Member

👍

@RebeccaDeField
Contributor

@Umcaruje I can't tell from the screenshot, but is there any variation of the shade when the title bar is active vs inactive? That might add some more usability.

@tresf
Member
tresf commented Jan 28, 2016

@Umcaruje I can't tell from the screenshot, but is there any variation of the shade when the title bar is active vs inactive? That might add some more usability.

👍 Good catch.

@Umcaruje
Member

@Umcaruje I can't tell from the screenshot, but is there any variation of the shade when the title bar is active vs inactive? That might add some more usability.

Nope, that needs to be implemented, and also buttons don't change on hover, but I think that'll be solved by CSS.

@BaraMGB
Contributor
BaraMGB commented Jan 30, 2016

Okay, I added some QPropertys which can be modified per style.css. Have a look.

@IvanMaldonado
Contributor

Tested, it works for me

Manjaro 15.12 Capella
Kernel: x86_64 Linux 4.1.18-1-MANJARO

captura de pantalla_2016-02-22_20-27-30

@RebeccaDeField
Contributor

Just made a mockup for title bars that I think would suit the current default theme well. Please take a look.

titlebaridea

@dednikko

I like it. What will the difference be for the title bars that are IN focus
vs OUT of docs?

On Tue, Feb 23, 2016, 5:20 PM Rebecca DeField notifications@github.com
wrote:

Just made a mockup for title bars that I think would suit the current
default theme well. Please take a look.

[image: titlebaridea]
https://cloud.githubusercontent.com/assets/7960169/13270155/04748018-da41-11e5-9eaf-b878f227c6de.png


Reply to this email directly or view it on GitHub
#2516 (comment).

@dednikko

*out of focus

On Tue, Feb 23, 2016, 5:45 PM Nikko Rocksalot nikko.rocksalot@gmail.com
wrote:

I like it. What will the difference be for the title bars that are IN
focus vs OUT of docs?

On Tue, Feb 23, 2016, 5:20 PM Rebecca DeField notifications@github.com
wrote:

Just made a mockup for title bars that I think would suit the current
default theme well. Please take a look.

[image: titlebaridea]
https://cloud.githubusercontent.com/assets/7960169/13270155/04748018-da41-11e5-9eaf-b878f227c6de.png


Reply to this email directly or view it on GitHub
#2516 (comment).

@RebeccaDeField
Contributor

titlebaridea
Out of focus

titlebaridea2
In focus

@dednikko

Looks clear to me! Thanks!

On Tue, Feb 23, 2016, 7:19 PM Rebecca DeField notifications@github.com
wrote:

[image: titlebaridea]
https://cloud.githubusercontent.com/assets/7960169/13272518/9369d2a4-da51-11e5-893f-0955b6521a39.png
Out of focus

[image: titlebaridea2]
https://cloud.githubusercontent.com/assets/7960169/13272519/936b13a8-da51-11e5-8e58-7b855a69ba5b.png
In focus


Reply to this email directly or view it on GitHub
#2516 (comment).

@BaraMGB
Contributor
BaraMGB commented Feb 24, 2016

@RebeccaDeField you have a gradient in there, right? Okay, I add that property soon. And correct the typo @Umcaruje :-)

@RebeccaDeField
Contributor

@BaraMGB 👍

@Umcaruje Umcaruje referenced this pull request Mar 3, 2016
Open

Switching to Qt5 #2611

13 of 16 tasks complete
@Umcaruje
Member
Umcaruje commented Mar 6, 2016

I worked on this a bit and made a new branch on my fork:
master...Umcaruje:subwindow

I simplified the code a lot, so now we only have one rectrangle for the title, also implemented @RebeccaDeField's color idea. I have yet to fix the titles and the buttons.

Screenshot:
screenshot from 2016-03-04 01 11 27

Review and ideas welcome. I'll post around the challenges of proper title rendering and buttons a bit later.

@Umcaruje Umcaruje referenced this pull request Mar 6, 2016
Closed

New Default Theme #2587

@RebeccaDeField
Contributor

@Umcaruje Great work! 👍

My 2 cents: this may be because you have zoomed-in your screenshot, but I feel that the rounded corners are a bit jagged. Is there anything we can do to smooth the corners out a bit? If not, it might be best to keep the corners pointed/straight.

@claell
claell commented Mar 7, 2016

@RebeccaDeField I noticed the same regarding the corners. I don't know, what looks best, round or pointed, but I think even pointed ones should be at least rounded a bit.

@RebeccaDeField
Contributor

@claell Perhaps a 2px radius will look better than 3px.

@claell
claell commented Mar 15, 2016

Maybe, I don't know. Cannot imagine what would look better, but I guess it would be worth a try.

@RebeccaDeField
Contributor

@Umcaruje Could you try a 2px radius on the corners? If it still doesn't look enough smooth, I think we should go with straight corners.

@Umcaruje
Member
Umcaruje commented Apr 3, 2016

@RebeccaDeField are you talking about the bottom or upper corners? Since this is a hack, and we're drawing over the actual window, my control of this is very limited. I'm actually drawing non-rounded rectrangles and lines with the code.

@RebeccaDeField
Contributor

@Umcaruje It's the upper corners. I really do think that using straight corners would be best in this case. That will keep things more simple.

Thanks for your work on this! 👍

@claell
claell commented Apr 4, 2016

@Umcaruje So it is probably not possible to draw a rounded rectangle?

@BaraMGB
Contributor
BaraMGB commented Apr 7, 2016

@Umcaruje Perhaps, you can tell me, what's in your mind. I would only add the three buttons and the customization would done in the css file. Why you want add a toolbar? I have tested this with the close button without a toolbar and it works fine.

@BaraMGB
Contributor
BaraMGB commented Apr 7, 2016

@claell It isn't possible to change the shape of the title bar at all.

@tresf
Member
tresf commented Apr 7, 2016

@claell It isn't possible to change the shape of the title bar at all.

@BaraMGB can you describe what happens if you try to make a portion transparent? I ask because if you can make a region transparent, then you may also be able to adjust the shape of the corners.

@BaraMGB
Contributor
BaraMGB commented Apr 7, 2016

@tresf if we make something transparent the old blue window decoration shines through. And if we want draw over the edges of the old decoration it will not be drawn.

@Umcaruje I have implemented the three buttons. It works so far. Please check: https://github.com/BaraMGB/lmms/tree/SubWindowButton A problem I have to solute is to change from minimized window status back to normal.

@claell
claell commented Apr 7, 2016

@BaraMGB And the overdrawing is needed to make themes possible or is there another reason for using this hack as @Umcaruje calls it?

@BaraMGB
Contributor
BaraMGB commented Apr 7, 2016

The blue window decoration is hard coded in Qt. Actually you can't change this. This hack is a trick to paint over the decoration. The old one is there, yet.

@tresf
Member
tresf commented Apr 7, 2016

if we make something transparent the old blue window decoration shines through.

What if we hack it out with the ever-so-buggy CSS disappearing title bar trick? 😄

QMdiSubWindow { border: 1px solid #000000; background: #000000 }
QMdiSubWindow:title { background: #000000 }

Edit: I agree though. Just leave it square. 👍

@claell
claell commented Apr 7, 2016

Ok, did not know that they would hardcode such thing.

@BaraMGB
Contributor
BaraMGB commented Apr 8, 2016

@tresf

What if we hack it out with the ever-so-buggy CSS disappearing title bar trick? 😄
QMdiSubWindow { border: 1px solid #000000; background: #000000 }
QMdiSubWindow:title { background: #000000 }

I've never seen that before 😁 unfortunately this doesn't make the old decoration disappear. It makes it only grey (or background coloured). But nice try 😜

@RebeccaDeField If we minimize a window, the minimize button changes to a restore button. For this we need an icon, too.
@Umcaruje
I have implemented this functionality, yet. I clean up the code a little bit and amend this pull request for testing.

@BaraMGB
Contributor
BaraMGB commented Apr 8, 2016

Okay, one thing I have to do is code a dynamic title length for windows which are smaller than the title string. That's for tomorrow.

Please test

edit: @tresf gnarf, what's wrong here? 😣

@RebeccaDeField
Contributor

If we minimize a window, the minimize button changes to a restore button. For this we need an icon, too.

On it :)

@Umcaruje
Member
Umcaruje commented Apr 8, 2016

@BaraMGB @RebeccaDeField I'm in Berlin now for the miniLAC, I won't be able to work on this or look at the code until I get back home. Regards

@RebeccaDeField
Contributor

Here is a link to the restore up icon. I also make a few edits to the other icons to make them a bit crisper.

@BaraMGB
Contributor
BaraMGB commented Apr 10, 2016

I think, it's almost done.

  • the button design is now complete in the style.css
  • I added a QLabel for the title text. It is much more convince than the QPainter.drawText(). The QLabel we can move around and shrink. And we can use QLabel::sizeHint() which gives us the width of the string in pixel. It truncate the title text now if the subwindow is smaller than the title text.
  • The title text font is now full configurable in the styles.css. You can change the font-size, bold, italic, colour and so on. But without alignment. It will always be in the center.

But now I don't know how to insert the svg icons by @RebeccaDeField. @Umcaruje can you help here?

And everybody love screenshots (isn't it @tresf 😄 )

bildschirmfoto von 2016-04-10 19-22-38

@RebeccaDeField
Contributor

But now I don't know how to insert the svg icons by @RebeccaDeField. @Umcaruje can you help here?

Since he's busy with the miniLAC I was wondering if anyone else was available to help with this?
@michaelgregorius @tresf @StakeoutPunch @LocoMatt

@liushuyu
Contributor
@RebeccaDeField
Contributor
RebeccaDeField commented Apr 17, 2016 edited

@liushuyu Not sure, but I think he is trying to use the graphics that I created exported as .pngs. I think that would be consistent with the rest of the icons.

@BaraMGB
Contributor
BaraMGB commented Apr 17, 2016

AFAIK, Qt's SVG handling is very strange, and the render of svg is buggy. It needs tons of hacks to
get it worked. :-(

In that case it would be the best way to use PNG. Any opinions against?

Not sure, but I think he is trying to use the graphics that I created exported as .pngs.

I think, that's no problem. I can do that for my self.

@tresf
Member
tresf commented Apr 17, 2016

@BaraMGB yes, let's use PNGs for now that's fine.

SVG compat tends to come down to the standards being used (the SVG is an XML file that contains embedded style information and compatibility for that can change between SVG versions, platforms, browsers, frameworks, etc).

I ran into this compat problem with @budislav and we were able to circumvent it... assuming @RebeccaDeField is using Adobe Illustrator, there's a way to export it for maximum compatibility.

Anyway, yes PNG is fine, but long term, let's not go and write-off the idea of using SVGs in LMMS. We'll eventually support them.

@circuitfry
circuitfry commented Apr 17, 2016 edited

PNGs are still super popular compared to SVGs and LMMS has a hungry theme making community. I agree with us implementing SVGs later.

@Umcaruje
Member
Umcaruje commented Apr 17, 2016 edited

I agree on using PNG for now. We can switch to SVG at a later time 👍

assuming @RebeccaDeField is using Adobe Illustrator

Actually @tresf, afaik she uses Inkscape. Open source power

@tresf
Member
tresf commented Apr 17, 2016

afaik she uses Inkscape.

Ok, well that has some optimized output control for compat as well, but it's worth noting/remembering for when we eventually start incorporating SVGs into our codebase. 👍

image

@RebeccaDeField
Contributor
RebeccaDeField commented Apr 17, 2016 edited

It looks like we've come to the consensus that @BaraMGB can use the .PNGs for now.

I have been very excited to see .SVGs being more supported (in websites and programs) over the years and I hope that continues to grow. If we want to switch the title bar icons to SVG, the rest of the icons should probably be converted too for consistency, but that might a project for the future.

Actually @tresf, afaik she uses Inkscape. Open source power
Yes, I do

@RebeccaDeField
Contributor

@BaraMGB Just want to check in on this.

P.S. In your last screenshot the font was serif, and most of the fonts in LMMS are sans-serif. Is that because of your personal theme settings/GTK theme, or can we change the font?

@Umcaruje
Member

@RebeccaDeField @BaraMGB I'll look at the code this weekend and leave comments this weekend.

@BaraMGB
Contributor
BaraMGB commented Apr 20, 2016

@RebeccaDeField I've added the icons. The italic serif font was for demonstrate the possibilities with the styles.css. It's now sans serif and bold. You can change it per css.

@Umcaruje Thank you.

@RebeccaDeField
Contributor

Thank you @BaraMGB @Umcaruje !!

@BaraMGB
Contributor
BaraMGB commented Apr 24, 2016

Find just a little bug right now and correct it.

@Umcaruje Umcaruje commented on an outdated diff Apr 25, 2016
data/themes/default/style.css
@@ -540,6 +540,42 @@ BBTCOView {
qproperty-textColor: rgb( 255, 255, 255 );
}
+/* subwindows in MDI-Area */
+SubWindow {
+ color: qlineargradient(x1: 0, y1: 0, x2: 0, y2: 1,
+ stop: 0 #4b525c, stop: 1.0 #31363d);
+ qproperty-activeColor: qlineargradient(x1: 0, y1: 0, x2: 0, y2: 1,
+ stop: 0 #33383e, stop: 1.0 #1a1c20);
+ qproperty-textShadowColor: rgb(0, 0, 0);
+ qproperty-borderColor: rgb(0, 0, 0);
+}
+
+/*SubWindow Title Text */
+SubWindow > QLabel {
+ color: rgb(255, 255, 255);
+ font-family: Verdana, 'Lucida Sans Unicode', sans-serif;
@Umcaruje
Umcaruje Apr 25, 2016 Member

This line should be removed. We should use the default font, as we use that for every other part of LMMS.

@Umcaruje Umcaruje commented on an outdated diff Apr 25, 2016
include/SubWindow.h
private:
+ QPushButton * m_closeBtn;
+ QPushButton * m_minimizeBtn;
+ QPushButton * m_maximizeBtn;
@Umcaruje
Umcaruje Apr 25, 2016 Member

Mind the indentation over here please, looks like you have mixed tabs and spaces.

@Umcaruje Umcaruje commented on an outdated diff Apr 25, 2016
src/gui/SubWindow.cpp
SubWindow::SubWindow(QWidget *parent, Qt::WindowFlags windowFlags)
: QMdiSubWindow(parent, windowFlags)
{
// initialize the tracked geometry to whatever Qt thinks the normal geometry currently is.
// this should always work, since QMdiSubWindows will not start as maximized
- m_trackedNormalGeom = normalGeometry();
+ m_trackedNormalGeom = normalGeometry();
+
+ // inits the colors
+ m_activeColor = Qt::SolidPattern;
+ m_textShadowColor = Qt::black;
+ m_borderColor = Qt::black;
+
+ //close, minimize, maximize and restore(after minimize) buttons
+ m_closeBtn = new QPushButton( embed::getIconPixmap( "close" ), QString::null, this );
+ m_closeBtn->resize(17, 17);
@Umcaruje
Umcaruje Apr 25, 2016 Member

You should define a const int btnSize = 17; or something similar, and just easily replace every instance of 17 with that. Usually you want to use as low amount of 'magic numbers' as possible.

@Umcaruje Umcaruje commented on an outdated diff Apr 25, 2016
src/gui/SubWindow.cpp
SubWindow::SubWindow(QWidget *parent, Qt::WindowFlags windowFlags)
: QMdiSubWindow(parent, windowFlags)
{
// initialize the tracked geometry to whatever Qt thinks the normal geometry currently is.
// this should always work, since QMdiSubWindows will not start as maximized
- m_trackedNormalGeom = normalGeometry();
+ m_trackedNormalGeom = normalGeometry();
+
+ // inits the colors
+ m_activeColor = Qt::SolidPattern;
@Umcaruje
Umcaruje Apr 25, 2016 Member

Indentation issues here too

@Umcaruje Umcaruje commented on an outdated diff Apr 25, 2016
src/gui/SubWindow.cpp
+ const int titleBarHeight = 24;
+ QRect rect(0, 0, width(), titleBarHeight);
+ bool isActive = SubWindow::mdiArea()->activeSubWindow() == this;
+
+ p.fillRect(rect, isActive ? activeColor() : p.pen().brush() );
+
+ // window border
+ p.setPen(borderColor());
+
+ // bottom, left, and right lines
+ p.drawLine( 0, height() - 1, width(), height() - 1 );
+ p.drawLine( 0, titleBarHeight, 0, height() - 1 );
+ p.drawLine( width() - 1, titleBarHeight, width() - 1, height() - 1 );
+
+ //window icon
+ QPixmap winicon(widget()->windowIcon().pixmap(QSize(17,17)));
@Umcaruje
Umcaruje Apr 25, 2016 Member

same applies here as in 11c25a6#r60984624 , a const int to replace the instances of 17 should be made.

@Umcaruje Umcaruje commented on an outdated diff Apr 25, 2016
src/gui/SubWindow.cpp
void SubWindow::resizeEvent(QResizeEvent * event)
{
+ /* button adjustments*/
+ m_closeBtn->move(width()-22,3);
+ m_maximizeBtn->hide();
+ m_restoreBtn->hide();
+ int buttonBarWidth = 22;
+ if( windowFlags() & Qt::WindowMinimizeButtonHint)
+ {
+ int d = 0;
@Umcaruje
Umcaruje Apr 25, 2016 Member

What's d?

@Umcaruje Umcaruje commented on an outdated diff Apr 25, 2016
src/gui/SubWindow.cpp
void SubWindow::resizeEvent(QResizeEvent * event)
{
+ /* button adjustments*/
+ m_closeBtn->move(width()-22,3);
+ m_maximizeBtn->hide();
+ m_restoreBtn->hide();
+ int buttonBarWidth = 22;
+ if( windowFlags() & Qt::WindowMinimizeButtonHint)
+ {
+ int d = 0;
+ buttonBarWidth = buttonBarWidth + 18;
+ if ( windowFlags() & Qt::WindowMaximizeButtonHint)
+ {
+ d = 18;
+ buttonBarWidth = buttonBarWidth + 18;
+ m_maximizeBtn->show();
+ m_maximizeBtn->move(width()-40,3);
@Umcaruje
Umcaruje Apr 25, 2016 Member

why is it width-40, and why can't a variable handle this? Generally this whole chunk of code looks too complex and is full of magic numbers.

@Umcaruje
Member

Ok I reviewed the code. It works nicely, but I think the resizeEvent is waay to complex atm, and I don't think this can be merged yet. There is too much magic numbers and I think that's just too much code to run every time we resize a window. Also, the CSS changes are nice, but are wildly inconsistent with the rest of the program, so I think its best to keep the text at default font.

Oh, and another thing: your text gets cut off when the title bar gets too narrow. Implementing eliding in these situations would make a really nice visual touch.

Anyways, top notch work so far, this is really progressing well 🍻

@Umcaruje Umcaruje commented on an outdated diff Apr 25, 2016
src/gui/SubWindow.cpp
+ m_maximizeBtn->resize(17, 17);
+ m_maximizeBtn->setFocusPolicy(Qt::NoFocus);
+ m_maximizeBtn->setToolTip(tr("Maximize"));
+ connect(m_maximizeBtn, SIGNAL(clicked(bool)),this,SLOT(showMaximized()));
+
+ m_minimizeBtn = new QPushButton( embed::getIconPixmap( "minimize" ), QString::null, this );
+ m_minimizeBtn->resize(17, 17);
+ m_minimizeBtn->setFocusPolicy(Qt::NoFocus);
+ m_minimizeBtn->setToolTip(tr("Minimize"));
+ connect(m_minimizeBtn, SIGNAL(clicked(bool)),this,SLOT(showMinimized()));
+
+ m_restoreBtn = new QPushButton( embed::getIconPixmap( "restore" ), QString::null, this );
+ m_restoreBtn->resize(17, 17);
+ m_restoreBtn->setFocusPolicy(Qt::NoFocus);
+ m_restoreBtn->setToolTip(tr("Restore"));
+ connect(m_restoreBtn, SIGNAL(clicked(bool)),this,SLOT(showNormal()));
@Umcaruje
Umcaruje Apr 25, 2016 Member

I know we keep one eye shut on the 'space between parentheses' rule, but you should really put a space after your commas.

@RebeccaDeField
Contributor

@Umcaruje Thank you for working on this!

It seems that there is a good amount of work ahead for the title bars, so I have decided to keep the current title bars for version of my theme that is going on the LSP so that I can move my theme into a PR as soon as possible :)

Keep up the great work @BaraMGB! I look forward to seeing this worked into to the default theme in the future.

@BaraMGB
Contributor
BaraMGB commented Apr 26, 2016

I'll clean this up this week. Thank you @Umcaruje

@Umcaruje
Member
Umcaruje commented Apr 26, 2016 edited

Oh and, this is why I wanted to have a toolbar or a button group out there: You can just add buttons to the toolbar/group, you avoid all the calculations that you have for the buttons, and just need to reposition the toolbar based on its width and do the same with the QLabel. e.g.:

m_btnToolbar->move( width()-m_btnToolbar.width(), 3 );

and for QLabel you should lose the need for buttonBarWidth variable.

@BaraMGB
Contributor
BaraMGB commented Apr 26, 2016

Oh and, this is why I wanted to have a toolbar or a button group out there: You can just add buttons > to the toolbar/group, you avoid all the calculations that you have for the buttons, and just need to
reposition the toolbar based on its width and do the same with the QLabel.

I'm afraid, we loose the move functionality of the window and therefore we have to reimplement this. That's not a trivial thing, I think. And it doesn't avoids us from check, what kind of window it is ( minimizeable, maximizable and so on).

why is it width-40, and why can't a variable handle this? Generally this whole chunk of code looks too > complex and is full of magic numbers.

Here you are right. I can avoid the "magic numbers" by set three QPoints for the button position.

    QPoint rightButtonPos( width() -  22, 3 );
    QPoint middleButtonPos( width() - 40, 3 );
    QPoint leftButtonPos( width() - 58, 3 );

But here we have magic numbers again. I can set in variables to explain what's going on. But then the code is bloated again with calculations:

    int rightSpace = 3;
    int buttonWidth = 17;
    int buttonInterSpace = 1;

    QPoint rightButtonPos( width() - rightSpace - buttonWidth, 3 );
    QPoint middleButtonPos( width() - rightSpace - ( 2 * buttonWidth ) - buttonInterSpace, 3 );
    QPoint leftButtonPos( width() - rightSpace - ( 3 * buttonWidth ) - ( 2 * buttonInterSpace ), 3 );

I vote for the hard coded numbers.

@tresf
Member
tresf commented Apr 26, 2016 edited

the code is bloated again with calculations:

    int rightSpace = 3;
    int buttonWidth = 17;
    int buttonInterSpace = 1;   
    QPoint rightButtonPos( width() - rightSpace - buttonWidth, 3 );
    QPoint middleButtonPos( width() - rightSpace - ( 2 * buttonWidth ) - buttonInterSpace, 3 );
    QPoint leftButtonPos( width() - rightSpace - ( 3 * buttonWidth ) - ( 2 * buttonInterSpace ), 3 );

Lesser of two evils. What you consider bloated is themable. 👍

If you really want the best of both worlds, write a small toolbar wrapper around the button positions (taking an Array of QPushButton perhaps) which performs the layout logic right to left in an additive fashion. Even the QPoint y-cordinate shouldn't be hard-coded. Perhaps overkill, but it would help manage the nudging of elements, which is what the aforementioned toolbar as well as a layout manager would do under the covers. :)

I'm sure you've thought of this, but can't you get the width (e.g. size) from the QPushButtons themselves? http://doc.qt.io/qt-5/qwidget.html#size-prop

Also, I'd refer to anything that spaces two elements as padding. 👍

@BaraMGB
Contributor
BaraMGB commented Apr 27, 2016

Okay, I have to explain some things a little bit more, perhaps:

Since we override the paintevent() in this subwindow, the blue title bar and its button disappear. But the functionality of this elements is there, yet. You can move the window per drag the title bar. And you can close, minimize and maximize the window if you click at the point where the button should be. Therefore we have to cover up the old place for the button at exact the same place. If you move the close button (for example) to an other place than width()-22,3 then you have this close button and an invisible close button at width()-22,3.

I have started some tests. I tried to add a QHBoxLayout to the Subwindow. This fails because it appears under the title bar. My next try was to add an widget at 0,0 and add the Layout to this. At first it looks good but there are some problems: The buttons don't take the theme from the styles.css and moving the window per drag the title bar is interrupted. Sometimes you can drag, most of the time you can't. Yes, we could override the mouseevent() of this widget to reimplement the move functionality. But hey we talk about set three buttons at the right place 😜

My suggestion:
I use some If statements in the resizeevent to check which kind of window we have (minimizable, maximizable, isMinimized) and then I move() the buttons on the right place. That's simple enough, I think. And it works like a charm. 😬

I'll clean up the code this week.

@BaraMGB
Contributor
BaraMGB commented Apr 29, 2016 edited

Okay, I think, I have the code simplified a lot. I hope it's near to merge now.
@Umcaruje please have a look.

Edit: @tresf @Umcaruje I need a clear order one for all time now: Do we use spaces between parentheses or not? In the code conventions I can't see this rule any more. Also the 4 lines between functions rule I can't see. What's happen with this?

Thank you

@Umcaruje Umcaruje commented on an outdated diff Apr 29, 2016
src/gui/SubWindow.cpp
+ p.setPen(borderColor());
+
+ // bottom, left, and right lines
+ p.drawLine(0, height() - 1, width(), height() - 1);
+ p.drawLine(0, m_titleBarHeight, 0, height() - 1);
+ p.drawLine(width() - 1, m_titleBarHeight, width() - 1, height() - 1);
+
+ //window icon
+ QPixmap winicon(widget()->windowIcon().pixmap(m_buttonSize));
+ p.drawPixmap(3, 3, m_buttonSize.width(), m_buttonSize.height(), winicon);
+}
+
+
+
+
+void SubWindow::setTextToLabel(QLabel *label, QString text)
@Umcaruje
Umcaruje Apr 29, 2016 edited Member

I'd suggest this gets renamed to something like elideText

@Umcaruje Umcaruje commented on an outdated diff Apr 29, 2016
src/gui/SubWindow.cpp
+ m_minimizeBtn = new QPushButton( embed::getIconPixmap( "minimize" ), QString::null, this );
+ m_minimizeBtn->resize(m_buttonSize);
+ m_minimizeBtn->setFocusPolicy(Qt::NoFocus);
+ m_minimizeBtn->setToolTip(tr("Minimize"));
+ connect(m_minimizeBtn, SIGNAL(clicked(bool)), this, SLOT(showMinimized()));
+
+ m_restoreBtn = new QPushButton( embed::getIconPixmap( "restore" ), QString::null, this );
+ m_restoreBtn->resize(m_buttonSize);
+ m_restoreBtn->setFocusPolicy(Qt::NoFocus);
+ m_restoreBtn->setToolTip(tr("Restore"));
+ connect(m_restoreBtn, SIGNAL(clicked(bool)), this, SLOT(showNormal()));
+
+ // QLabel for window title and shadow effect
+ m_shadow = new QGraphicsDropShadowEffect();
+ m_shadow->setColor(m_textShadowColor);
+ m_shadow->setBlurRadius(3);
@Umcaruje
Umcaruje Apr 29, 2016 Member

I'd suggest this blur gets removed, since all the other drop shadows in LMMS are not blurred, so this is inconsistent with the rest of the codebase.

@Umcaruje
Member

Do we use spaces between parentheses or not? In the code conventions I can't see this rule any more.

It's not a rule anymore, so you can do as you want I guess. Most of the codebase has spaces there. Infix operators (=, +, -, *, /, etc.) though, should have a space before and after. That's a convention.

I reviewed this, and this looks much better. The Travis build is failing though.

I'm still a bit concerned if there will be a performance hit, when we recalculate the buttons position every time we resize, when in reality that needs to happen only when the window gets created, and when it gets minimised/restored.

Idea: could something like the needsUpdate method we use for drawing TCO's be implemented? Basically, it only redraws the pattern when there is a change. You can look up #2574 to see how all that works.

I'm not experienced in this, and might be wrong, so @LMMS/developers do you think running this code in the resizeEvent would cause performance issues?

@Umcaruje Umcaruje commented on an outdated diff Apr 29, 2016
include/SubWindow.h
private:
+ const QSize m_buttonSize;
+ const int m_titleBarHeight = 24;
@Umcaruje
Umcaruje Apr 29, 2016 Member

Appearently this is failing the Travis builds, as its not allowed by the C++ ISO standard:
http://stackoverflow.com/questions/12757501/why-the-iso-c-standard-forbids-the-initialization-for-members

@tresf
Member
tresf commented Apr 29, 2016

I'm not experienced in this, and might be wrong, so @LMMS/developers do you think running this code in the resizeEvent would cause performance issues?

Since resizing is done so rarely, the hit would be minimal to performance (that doesn't mean it's good design though).

I'm still a bit concerned if there will be a performance hit, when we recalculate the buttons position every time we resize, when in reality that needs to happen only when the window gets created, and when it gets minimised/restored.

Nobel point. @BaraMGB are there some wasted CPU cycles with the logic or is it necessary to recalculate each time?

@BaraMGB
Contributor
BaraMGB commented Apr 29, 2016

We could check if the width changed. I look into this.

@tresf
Member
tresf commented Apr 29, 2016 edited

We could check if the width changed. I look into this.

Well, if the button position(s) potentially change with every width changes, running it on every resize isn't that bad of a design actually. If that's the case, the wasted cycles would only be on vertical resize, which is benign enough IMO. We aren't writing a video game, so the logic wouldn't be executing hundreds of times a second. 👍

@Umcaruje
Member

Ah, right, I completely forgot that button positions need to be recalculated for every time we change the width of the window. Yeah, the times where you only vertical resize are very rare, so this sounds very reasonable. Thanks for explaining @tresf 👍

@BaraMGB
Contributor
BaraMGB commented Apr 29, 2016

Okay, I updated these few things.

Regards

@Umcaruje Umcaruje commented on an outdated diff Apr 29, 2016
data/themes/default/style.css
+SubWindow {
+ color: qlineargradient(x1: 0, y1: 0, x2: 0, y2: 1,
+ stop: 0 #4b525c, stop: 1.0 #31363d);
+ qproperty-activeColor: qlineargradient(x1: 0, y1: 0, x2: 0, y2: 1,
+ stop: 0 #33383e, stop: 1.0 #1a1c20);
+ qproperty-textShadowColor: rgb(0, 0, 0);
+ qproperty-borderColor: rgb(0, 0, 0);
+}
+
+/*SubWindow Title Text */
+SubWindow > QLabel {
+ color: rgb(255, 255, 255);
+ font-size: 12px;
+ font-style: normal;
+ font-weight: bold;
+ }
@Umcaruje
Umcaruje Apr 29, 2016 Member

Remove the indentation on the closing brackets please, so its consistent with the rest of the stylesheet.

@Umcaruje Umcaruje commented on an outdated diff Apr 29, 2016
data/themes/default/style.css
+
+/*SubWindow Title Text */
+SubWindow > QLabel {
+ color: rgb(255, 255, 255);
+ font-size: 12px;
+ font-style: normal;
+ font-weight: bold;
+ }
+
+/*SubWindow titlebar button */
+SubWindow > QPushButton {
+ background-color: rgba( 255, 255, 255, 0% );
+ border-width: 0px;
+ border-color: none;
+ border-style: none;
+ }
@Umcaruje
Umcaruje Apr 29, 2016 Member

Same as above

@Umcaruje Umcaruje commented on an outdated diff Apr 29, 2016
data/themes/default/style.css
+
+/*SubWindow titlebar button */
+SubWindow > QPushButton {
+ background-color: rgba( 255, 255, 255, 0% );
+ border-width: 0px;
+ border-color: none;
+ border-style: none;
+ }
+
+SubWindow > QPushButton:hover{
+ background-color: rgba( 255, 255, 255, 15% );
+ border-width: 1px;
+ border-color: rgba( 0, 0, 0, 20% );
+ border-style: solid;
+ border-radius: 2px;
+ }
@Umcaruje
Umcaruje Apr 29, 2016 Member

Same as above

@Umcaruje Umcaruje commented on an outdated diff Apr 29, 2016
src/gui/SubWindow.cpp
SubWindow::SubWindow(QWidget *parent, Qt::WindowFlags windowFlags)
- : QMdiSubWindow(parent, windowFlags)
+ : QMdiSubWindow(parent, windowFlags)
+ , m_buttonSize (17,17)
@Umcaruje
Umcaruje Apr 29, 2016 Member

I'd suggest to remove the space between the perenthesis and the variable names, and move the colons and commas, like this:

SubWindow::SubWindow(QWidget *parent, Qt::WindowFlags windowFlags) :
    QMdiSubWindow(parent, windowFlags),
    m_buttonSize(17,17),
    m_titleBarHeight(24)

That's how this usually looks in the rest of the codebase.

@Umcaruje
Member

screenshot from 2016-04-30 00 19 55

Here's how it looks. I noticed how the text isn't centered vertically now, that can be fixed by adjusting the row height in CSS, or some QLabel trickery maybe.

I made some other style remarks, but this looks good to merge. 👍 Great job @BaraMGB

@Umcaruje Umcaruje commented on an outdated diff Apr 29, 2016
src/gui/SubWindow.cpp
+
+ QPoint rightButtonPos( width() - rightSpace - m_buttonSize.width() , 3 );
+ QPoint middleButtonPos( width() - rightSpace - ( 2 * m_buttonSize.width() ) - buttonGap, 3 );
+ QPoint leftButtonPos( width() - rightSpace - ( 3 * m_buttonSize.width() ) - ( 2 * buttonGap ), 3 );
+
+ //The buttonBarWidth relates on the count of button.
+ //We need it to calculate the width of window title label
+ int buttonBarWidth = rightSpace + m_buttonSize.width();
+
+ //set the buttons on their positions.
+ //the close button is ever needed and on the rightButtonPos
+ m_closeBtn->move(rightButtonPos);
+
+ //here we ask: is the Subwindow maximizable and/or minimizable
+ //then we set the buttons and show them if needed
+ if ( windowFlags() & Qt::WindowMaximizeButtonHint )
@Umcaruje
Umcaruje Apr 29, 2016 Member

I'd suggest to remove the space that comes after if. Generally, you need to get your spaces in order. Having space or no space are both valid options, but it should be consistent across the whole file.

@Umcaruje Umcaruje commented on an outdated diff Apr 29, 2016
src/gui/SubWindow.cpp
+ if (m_maximizeBtn->isHidden())
+ {
+ m_restoreBtn->move(middleButtonPos);
+ }
+ else
+ {
+ m_restoreBtn->move(leftButtonPos);
+ }
+ m_restoreBtn->show();
+ m_minimizeBtn->hide();
+ }
+ }
+
+ // title QLabel adjustments
+ m_windowTitle->setAlignment( Qt::AlignHCenter );
+ m_windowTitle->setFixedWidth( widget()->width() - (menuButtonSpace + buttonBarWidth) );
@Umcaruje
Umcaruje Apr 29, 2016 Member

Same applies to space after/before parentheses. Either is fine, but you have to pick one.

@Umcaruje Umcaruje commented on an outdated diff Apr 29, 2016
data/themes/default/style.css
+/* subwindows in MDI-Area */
+SubWindow {
+ color: qlineargradient(x1: 0, y1: 0, x2: 0, y2: 1,
+ stop: 0 #4b525c, stop: 1.0 #31363d);
+ qproperty-activeColor: qlineargradient(x1: 0, y1: 0, x2: 0, y2: 1,
+ stop: 0 #33383e, stop: 1.0 #1a1c20);
+ qproperty-textShadowColor: rgb(0, 0, 0);
+ qproperty-borderColor: rgb(0, 0, 0);
+}
+
+/*SubWindow Title Text */
+SubWindow > QLabel {
+ color: rgb(255, 255, 255);
+ font-size: 12px;
+ font-style: normal;
+ font-weight: bold;
@Umcaruje
Umcaruje Apr 29, 2016 Member

I just noticed that the text before wasn't bold. I'd get it back to regular weight, unless you think it increases readability.

@BaraMGB
Contributor
BaraMGB commented Apr 30, 2016 edited

I noticed how the text isn't centered vertically now

Yes, when it is exactly vertical centred it seems a little bit to down for my eyes. I think the gradient makes an optical illusion. But I can set it per code in the vertical centre.

I correct the spaces now and remove the bold tag. Actually the theming should do some one else after it is merged.

EDIT: Okay, updated.

bildschirmfoto von 2016-04-30 15-12-00

This is how it looks now.

@Umcaruje Umcaruje and 1 other commented on an outdated diff Apr 30, 2016
src/gui/SubWindow.cpp
+ m_restoreBtn->move( middleButtonPos );
+ }
+ else
+ {
+ m_restoreBtn->move( leftButtonPos );
+ }
+ m_restoreBtn->show();
+ m_minimizeBtn->hide();
+ }
+ }
+
+ // title QLabel adjustments
+ m_windowTitle->setAlignment( Qt::AlignHCenter );
+ m_windowTitle->setFixedWidth( widget()->width() - ( menuButtonSpace + buttonBarWidth ) );
+ m_windowTitle->move( menuButtonSpace,
+ ( m_titleBarHeight / 2 ) - ( m_windowTitle->sizeHint().height() / 2) );
@Umcaruje
Umcaruje Apr 30, 2016 Member

Can you explain why this line was broken down like this?

@BaraMGB
BaraMGB Apr 30, 2016 Contributor

Because of

Try to limit line lengths to 80 characters
Not an absolute requirement – sometimes longer lines can't be avoided. But it is a friendly thing to do.

@Umcaruje
Umcaruje Apr 30, 2016 Member

Yes, but you just indented the line back where it would be anyway if you did not break, so the readability didn't really approve at all.

@Umcaruje Umcaruje commented on an outdated diff Apr 30, 2016
data/themes/default/style.css
@@ -409,8 +409,8 @@ QToolButton#stopButton {
/* all tool buttons */
QToolButton:hover {
- background: qlineargradient(x1:0, y1:0, x2:0, y2:1, stop:0 #c0cdd3, stop:1 #71797d);
- color: white;
+ background: qlineargradient(x1:0, y1:0, x2:0, y2:1, stop:0 #c0cdd3, stop:1 #71797d);
+ color: white;
@Umcaruje
Umcaruje Apr 30, 2016 Member

Please convert all indentation to use tabs.

@Umcaruje Umcaruje commented on an outdated diff Apr 30, 2016
src/gui/SubWindow.cpp
-SubWindow::SubWindow(QWidget *parent, Qt::WindowFlags windowFlags)
- : QMdiSubWindow(parent, windowFlags)
+
+SubWindow::SubWindow( QWidget *parent, Qt::WindowFlags windowFlags )
+ : QMdiSubWindow( parent, windowFlags )
@Umcaruje
Umcaruje Apr 30, 2016 Member

Reposting the comment. I'd suggest to move the colons and commas, like this:

SubWindow::SubWindow(QWidget *parent, Qt::WindowFlags windowFlags) :
    QMdiSubWindow(parent, windowFlags),
    m_buttonSize(17,17),
    m_titleBarHeight(24)

That's how this usually looks in the rest of the codebase, unless you have a strong reason for keeping it this way.

@Umcaruje
Member

Yes, when it is exactly vertical centred it seems a little bit to down for my eyes. I think the gradient makes an optical illusion. But I can set it per code in the vertical centre.

Hmm yeah, its off by a pixel. I'd suggest moving it one pixel up.

@BaraMGB
Contributor
BaraMGB commented May 2, 2016

@Umcaruje Update.

@Umcaruje Umcaruje commented on an outdated diff May 2, 2016
src/gui/SubWindow.cpp
+ {
+ m_restoreBtn->move( middleButtonPos );
+ }
+ else
+ {
+ m_restoreBtn->move( leftButtonPos );
+ }
+ m_restoreBtn->show();
+ m_minimizeBtn->hide();
+ }
+ }
+
+ // title QLabel adjustments
+ m_windowTitle->setAlignment( Qt::AlignHCenter );
+ m_windowTitle->setFixedWidth( widget()->width() - ( menuButtonSpace + buttonBarWidth ) );
+ m_windowTitle->move( menuButtonSpace, ( m_titleBarHeight / 2 ) - ( m_windowTitle->sizeHint().height() / 2) -1);
@Umcaruje
Umcaruje May 2, 2016 Member

space needs to be added between - and 1 here.

@Umcaruje
Member
Umcaruje commented May 2, 2016

@BaraMGB Great job! You have one space to add, and styling to fix in the style.css, and this is ready to get merged.

@BaraMGB
Contributor
BaraMGB commented May 2, 2016 edited

@Umcaruje

You have one space to add,

done

@Umcaruje
Member
Umcaruje commented May 2, 2016

@BaraMGB If you want, you could provide me with commit access to your fork, and I'll be glad to fix up the CSS indentation, so we can finally merge this one out 😉

@BaraMGB
Contributor
BaraMGB commented May 2, 2016

Okay, I have to look how this works.

@BaraMGB
Contributor
BaraMGB commented May 2, 2016

Done

@Umcaruje
Member
Umcaruje commented May 2, 2016

Ok, done. I've fixed the identation and rephrased some of the comments to make them clearer. I'll leave open for 24 hours, if someone else has any more input, and afterwards, I'm merging 🎉

@BaraMGB
Contributor
BaraMGB commented May 2, 2016

Has someone test this on Mac and Windows?

@Umcaruje
Member
Umcaruje commented May 2, 2016 edited

Has someone test this on Mac and Windows?

Nope. @tresf could you test this on a mac?

@Umcaruje
Member
Umcaruje commented May 2, 2016

A friend of mine tested it on windows 7, all looks well:
image

@tresf
Member
tresf commented May 2, 2016

👍 @Umcaruje was this Qt4 or Qt5 being tested?

@Umcaruje
Member
Umcaruje commented May 3, 2016

👍 @Umcaruje was this Qt4 or Qt5 being tested?

Qt4, when I tried to install the qt5 packages, my VM couldn't find them in the trusty ppa.

@midi-pascal
Contributor

@Umcaruje You can easily install Qt5 from the Qt site (This is what I did to have both).
Qt5 can be installed alongside with Qt4 with no interference.
My 2 cents 😉

@Umcaruje
Member
Umcaruje commented May 3, 2016

You can easily install Qt5 from the Qt site (This is what I did to have both).

Yes, I know, but this is for windows builds, which use a specific custom mingw PPA

@tresf
Member
tresf commented May 3, 2016

If you have dated PPAs...

sudo add-apt-repository ppa:tobydox/mingw-x-trusty
sudo apt-get update
sudo apt-get install mingw32-qt5-base mingw64-qt5-base
@Umcaruje
Member
Umcaruje commented May 3, 2016 edited

um, after some research, the correct packages are called mingw32-x-qt5base and mingw64-x-qt5base

I'll update the tutorial on wiki.

Edit: the current way of making qt5 executables is also wrong. You can call qt5 building by doing

../cmake/build_mingw64.sh -qt5

or

../cmake/build_mingw32.sh -qt5

I'll update this as well.

@tresf
Member
tresf commented May 3, 2016

Edit: the current way of making qt5 executables is also wrong. You can call qt5 building by doing

Not, it is not wrong. That's just a helper to set the QT5 export.

@tresf
Member
tresf commented May 3, 2016

The correct packages are called mingw32-x-qt5base and mingw64-x-qt5base

👍

@Umcaruje
Member
Umcaruje commented May 3, 2016

Ok, its been over 24 hours, this has been tested on both linux and windows, and this PR has been open for enough time for devs to make remarks. That said, the travis builds also passed, so I'm merging this.
🎉 🎉 🎉 🎉 🎉

@Umcaruje Umcaruje merged commit 0c9bf9b into LMMS:master May 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@BaraMGB
Contributor
BaraMGB commented May 3, 2016

Thanks to all! Especially to @Umcaruje for the great help. I hope this makes a lot of people happy. 🎉

@RebeccaDeField
Contributor
RebeccaDeField commented May 3, 2016 edited

Thank you to everyone who worked hard to make this a reality! I think I can speak on behalf on all of the themers when I say this feature really improves the interface :)

@IvanMaldonado
Contributor

Been waiting for this like a kid for Christmas for so much time. Thank you!

@Umcaruje
Member
Umcaruje commented May 3, 2016

Thanks on the kind words, but I really have to thank @BaraMGB for putting up with this PR, and @RebeccaDeField for providing the graphics. This is a prime example of teamwork and I'm very glad that we finally implemented this much wanted feature.

@BaraMGB BaraMGB deleted the BaraMGB:SubwindowDecoration branch May 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment