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

2209 font sizes too small #2222

Merged
merged 6 commits into from
Aug 23, 2015

Conversation

michaelgregorius
Copy link
Contributor

New pull request for #2209 because I have rebased my branch which does not seem to be a good idea when it was already pushed to origin.

Fixes the font size in the file browsers and enables dynamic rendering of controllers.

Removes one of the several calls to pointSizeF. This method seems to
return a font which has the same height in pixels on all displays
(regardless of the display's actual DPI value). In the long run these
calls will all have to be removed to make LMMS usable on high DPI
displays.
ControllerRackView now uses a layout to organize the ControllerViews.
The ControllerViews in turn use layouts to organize their widgets
(labels and push button). ControllerView now inherits from QFrame
instead of QWidget. The (static) background image for controllers was
deleted because the ControllerView can now be resized dynamically.

Song: Added specific signals for added and removed controllers. Also
removed a TODO by putting the functionality to remove all controllers in
a method (removeAllControllers).

Deleted Controllers now don't unregister from Song during deletion. This
has to be done by the client (Hollywood principle - "Don't call us, we
call you.").

TODO: For some strange reason I cannot programmatically resize the
controller rack to make it fit the controllers on my screen.
The PluginDescWidget doesn't use calls to pointSize anymore. Also the
name of the plugin is only painted bold when hovered over with the
mouse. The animation speed was increased a bit as well. Hope it is not
too fast for displays with smaller resolutions. The problem with the
current implementation is that it increases the height by incremental
steps of 1 pixel (triggered by a QTimer) which gives a slower speed on
high DPI displays. In the future this implementation might be improved,
e.g. by using the animation classes provided by Qt.

The SideBarWidget also does not use calls to pointSize anymore. Instead
the standard font is increased by 2 (typographical) points and then
rendered at the right place using information from QFontMetrics. In the
long run the header of the SideBarWidget should be organized using a
layout, e.g. a layout with one QLabel to render the icon and one QLabel
to render the header text. This would ensure that the black background
would always be large enough and that the fonts do not protude into the
actual content.
Fixed the rendering of the tact numbers in the timeline widget. Before
this fix they were not readable because they were too big. Interestingly
in this case the fix is to use a font size in pixels (half the height of
the widget). The numbers are now also rendered a bit darker than the
lines.

Also removed the pixmap for the timeline from the code and from the
filesystem. It was only used to determine the fixed height of the widget
but not rendered. Therefore it was removed and the height is now
directly set to 18 pixels which was the height of the pixmap.
setWindowIcon( embed::getIconPixmap( "controller" ) );
setWindowTitle( tr( "Controller Rack" ) );

m_scrollArea = new QScrollArea( this );
m_scrollArea->setVerticalScrollBarPolicy( Qt::ScrollBarAlwaysOn );
m_scrollArea->setHorizontalScrollBarPolicy( Qt::ScrollBarAlwaysOff );
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I agree with this.

Previously, it was impossible to resize the controller rack small enough to where it had a horizontal scrollbar. Now it is possible. I prefer the old behavior, mainly because now when I open an empty controller rack & add a new controller, the window will not be large enough to fit all of it.

I'm don't know if this is a result of removing the setHorizontalScrollBarPolicy or setMinimumWidth calls. FWIW I do agree with you making the vertical scrollbar behavior automatic.

@Wallacoloo
Copy link
Member

Other than that one comment I Ieft, this looks great & works fine on my system. Those fonts used to be hard to read even on my low-dpi 1366x768 monitor.

@Wallacoloo
Copy link
Member

@michaelgregorius can you address my line comment above? I would like to merge this, but that comment needs to be discussed first.

Removes the potential horizontal scrollbar from the controller rack
view.

Also sets the MDI window of the controller rack to a bigger size in the
constructor and moves it towards the other windows. This code is active
in the case where there is no default template from which the window
states are loaded.
@michaelgregorius
Copy link
Contributor Author

@Wallacoloo: I have removed the horizontal scrollbar as proposed. I have also changed the default size of the controller rack to a sensible size in case there is no default template and moved the controller rack window more towards the other windows. To see the results of these changes you will have to delete or temporarily rename the file lmms-home/templates/default.mpt and start the application.

@Wallacoloo
Copy link
Member

@michaelgregorius that's exactly what I had in mind. Thanks.

One minor issue - whereas all the other windows seem to have some minimum allowable height (see photo below), I can actually shrink the ControllerRack vertically to where even the window decoration is no longer visible, and thus the window can never be moved, resized, or really even used ever again:

lmms-2222-minwindowheight

So some minimum vertical height may also be required.

@midi-pascal
Copy link
Contributor

@Wallacoloo On which platform? I'm using Ubuntu and I cannot reduce the controller window height more than its default size although it does not resize well horizontally. Only the frame is resized but not its contents.

@michaelgregorius
Copy link
Contributor Author

@Wallacoloo Unfortunately I cannot reproduce this in my version as well. Are there any specific steps that you took? Or is it possible that this is somehow related to the window manager being used? Although I would assume that all the window management for the MDI windows is handled by Qt and should therefore be independent.

@tresf
Copy link
Member

tresf commented Aug 16, 2015

should therefore be independent

Yes, should be. 😺

@Wallacoloo
Copy link
Member

Ah, sorry. Looks like I forgot to recompile after switching from a different branch... T_T This error doesn't occur on your branch.

Anyway, on your branch, the minimum width isn't maintained - I can resize the ControllerRack to look like this, which isn't all that useful:

lmms-2222-minwidthissue

What I had in mind was restricting the minimal width of the window such that the contents could always be viewed without needing to scroll horizontally. This patch to your code seems to do that:

diff --git a/src/gui/widgets/ControllerRackView.cpp b/src/gui/widgets/ControllerRackView.cpp
index 2bf7054..0ce1cdd 100644
--- a/src/gui/widgets/ControllerRackView.cpp
+++ b/src/gui/widgets/ControllerRackView.cpp
@@ -88,6 +88,7 @@ ControllerRackView::ControllerRackView( ) :
        subWin->setAttribute( Qt::WA_DeleteOnClose, false );
        subWin->move( 680, 310 );
        subWin->resize(400, 200);
+       subWin->setMinimumWidth( 300 );
 }

and then I can no longer shrink it further than this:

lmms-2222-minwidthfix

Unfortunately, adding that patch in re-introduces the above error where I could resize it vertically to no minimum, so instead it should maybe be subWin->setMinimumSize(300, 100); or something. The 300 should really be calculated dynamically, based on the minimum size that the ControllerView is allowed to go to + padding, but I couldn't figure out how to determine that. I thought maybe subWin->setSizePolicy(QSizePolicy::Minimum, QSizePolicy::Preferred); instead of the setMinimim calls would do it, but no luck.

@midi-pascal I am also on Ubuntu.

@michaelgregorius
Copy link
Contributor Author

@Wallacoloo That's also what I tried to do. I tried to compute the minimum widget of the QScrollArea by checking the width of each ControllerView that's added but that gave me very strange results. One of the problems is that the minimum width of the ControllerView now depends on the used font size and also on the name of the controller. If someone decides to add a "SuperDuperNewControllerWithALongName" then 300px won't suffice.

That's why I decided to give the controller rack a bigger default width (which unfortunately only work if there is no default template with a small width) as a first solution. Perhaps someone else can find a more elegant solution but I have to admit that I am not that knowledgeable concerning the Qt layout rules.

@midi-pascal
Copy link
Contributor

You could use:
subwin->setMinimumHeight(100);
or
subwin->setMinimumSize(QSize(300, 100));
Make the window fit its contents is an other story because it changes depending if the scrollbar is shown or not.
I had hard times recently doing this for a MDI subwindow containing a QTableView and my solution is not very elegant IMHO.
If interested anyway:
After filling the table, I call adjustSize(); on the QTableView (very important) then a custom method that returns true if a scrollbar will be needed depending on the window size and the number of items in the table
Then an excerpt of the constructor code to size the window to its minimum:

gs = m_SubWindow->geometry();

// (Re-)Adjust the window width to fit the contents exactly
QRect ge = ui->eventsTable->geometry();
int frameWidth = ui->eventsTable->frameWidth() * 2;
int width = ge.width() + frameWidth;
if (willNeedVerticalScrollBar()) {
    width += ui->eventsTable->verticalScrollBar()->sizeHint().width() +
                     (frameWidth * 2);
}
else {
    width += frameWidth;
}
m_SubWindow->resize(width, gs.height());
m_SubWindow->show();

@Wallacoloo
Copy link
Member

Thanks for sharing your solution @midi-pascal, but it does appear to be fairly limited. And as @michaelgregorius points out, there is no constant width for which we can guarantee all controllers will fit into, because of the variable length of their names. So a dynamically-resizing window is pretty much the only thing that would prevent the horizontal scrollbar from ever appearing, and I'm not entirely in favor of that.

Given that there is no ideal solution to this, I'm happy to merge as-is. Is everyone OK with merging?

@midi-pascal
Copy link
Contributor

👍

@tresf
Copy link
Member

tresf commented Aug 18, 2015

The "Controls" button will now keep its minimal size when the
ControllerView is resized.
@michaelgregorius
Copy link
Contributor Author

@tres Thanks for the link! I think most of the widgets in the ControllerView should already have a reasonable SizePolicy. However, after reading the stackoverflow post I have improved the layout a bit by preventing the "Controls" button from resizing when the widget is enlarged.

I assume that the problem with dynamic widget sizes will keep cropping up in other places as well once more and more code is switched away from using fixed sizes. I guess I will need to play around more with Qt's layout system and experiment with custom widgets to get a feeling of how all that stuff works together. But for now I'd also vote to commit what we have so far. I know that the current state is not optimal but on the other hand it's not that it is unusable.

Merging #2273 might also alleviate the problem a bit because it will make saving and using (default) templates simpler. Once this is done the user can enlarge the controller rack to a reasonable size and then store this as the default template.

@Wallacoloo
Copy link
Member

And we merge.

I tested @michaelgregorius 's latest commit, and it's still working fine.

Wallacoloo added a commit that referenced this pull request Aug 23, 2015
@Wallacoloo Wallacoloo merged commit 3548629 into LMMS:master Aug 23, 2015
@Wallacoloo Wallacoloo mentioned this pull request Aug 23, 2015
@michaelgregorius michaelgregorius deleted the 2209-font-sizes-too-small branch August 23, 2015 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants