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

Qt/MainWindow: Add "Compact View" option that minimizes tool bar size and game list's default row height. #7720

Open
wants to merge 1 commit into
base: master
from

Conversation

3 participants
@cristian64
Copy link
Contributor

cristian64 commented Jan 20, 2019

Fixes https://bugs.dolphin-emu.org/issues/11305. Also, it minimizes the tool bar size when in Compact View.

compact view

Qt/MainWindow: Added "Compact View" option that minimizes tool bar si…
…ze and game list's default row height.

@cristian64 cristian64 force-pushed the cristian64:add_compact_view_option branch from 45141cb to daf82e8 Jan 20, 2019

@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Jan 20, 2019

@spycrab Was there any particular reason why Qt had padding between the game list entries to begin with? To me it just seemed like an unexplained change compared to wx.

@spycrab

This comment has been minimized.

Copy link
Contributor

spycrab commented Jan 21, 2019

@JosJuice I think there was some overlapping that was overcompensated for...anyway we should probably remove the margin everywhere, although I would prefer to have just a little one to better separate banners. Even if it's just a pixel.

@spycrab
Copy link
Contributor

spycrab left a comment

This needs to be improved a bit.

else
{
std::vector<int> widths;
std::transform(items.begin(), items.end(), std::back_inserter(widths),

This comment has been minimized.

@spycrab

spycrab Jan 21, 2019

Contributor

All of this seems like an awful lot of rather unpleasent code just to restore the proper widths. This should probably be handled in a different way.

if (Settings::Instance().IsCompactViewEnabled())
{
for (QWidget* widget : items)
widget->setMinimumWidth(0);

This comment has been minimized.

@spycrab

spycrab Jan 21, 2019

Contributor

Is this really required? Shouldn't hiding the widgets work too?


ToolBar::ToolBar(QWidget* parent) : QToolBar(parent)
{
setToolButtonStyle(Qt::ToolButtonTextUnderIcon);
auto update_compact_view_lambda = [this] {
if (Settings::Instance().IsCompactViewEnabled())

This comment has been minimized.

@spycrab

spycrab Jan 21, 2019

Contributor

This can be rewritten as:

setIconSize(compact ? ICON_SIZE_SMALL : ICON_SIZE);
setToolButtonStyle(compact ? Qt::ToolButtonIconOnly : Qt::ToolButtonTextUnderIcon);

(Making use of the CompactViewChanged bool compactparameter)

}
};
connect(&Settings::Instance(), &Settings::CompactViewChanged, this, update_compact_view_lambda);
update_compact_view_lambda();

This comment has been minimized.

@spycrab

spycrab Jan 21, 2019

Contributor

I think it might be better to turn this lambda into it's own method:

  1. It's not only used inline but called separately here
  2. Tidier
  3. Nicer connect syntax
const int min_width = *std::max_element(widths.begin(), widths.end()) * 0.85;
for (QWidget* widget : items)
widget->setMinimumWidth(min_width);
auto update_compact_view_lambda = [separators, items] {

This comment has been minimized.

@spycrab

spycrab Jan 21, 2019

Contributor

Please combine this with the other lambda and turn it into a proper method.

@@ -107,8 +107,14 @@ void GameList::MakeListView()
m_list->setCurrentIndex(QModelIndex());
m_list->setContextMenuPolicy(Qt::CustomContextMenu);
m_list->setWordWrap(false);
m_list->verticalHeader()->setDefaultSectionSize(m_list->verticalHeader()->defaultSectionSize() *
1.25);
auto update_compact_view_lambda = [vertical_header = m_list->verticalHeader()]

This comment has been minimized.

@spycrab

spycrab Jan 21, 2019

Contributor

This can be inlined into connect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment