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

Add preference option to set the row height #403

Merged
merged 7 commits into from
Nov 26, 2014

Conversation

daschuer
Copy link
Member

This is useful to display big covers in the library table. Or to tweak the count of rows that fit on the screen.

@ywwg
Copy link
Member

ywwg commented Nov 23, 2014

Shouldn't column height be derived from the library font? I'd prefer to have a preference for a font choice.

@ywwg
Copy link
Member

ywwg commented Nov 23, 2014

I'd really prefer we discuss design questions like this before you start writing code. It means you do a lot of extra work and these PRs are not a good interface for this type of design discussion.

@esbrandt
Copy link
Contributor

Basically, it is a good idea to allow to add some flexible whitespace around the text in a row. Hard to do in pure QSS, and can improve readability tremendously. Nonetheless your motivation was probably related to cover display.

Shouldn't column height be derived from the library font? I'd prefer to have a preference for a font choice.

While we at it: I'd prefer to have a extra preference for a font choice ;-)
See https://bugs.launchpad.net/mixxx/+bug/661717

My only concern is that it has an negative impact on the, already not so brilliant, library performance because of the potential bigger cover display, as discussed in #403.

@daschuer
Copy link
Member Author

I have tested this on my netbook and I have not noticed performance issues. It is probably because it is somehow self-compensating. As bigger covers are as less tracks can be displayed.

This is fix for "add an option for a non chopped cover art to library table"
https://bugs.launchpad.net/mixxx/+bug/1390868

@rryan
Copy link
Member

rryan commented Nov 24, 2014

BTW the word "column" is misspelled throughout the commit.

<number>10</number>
</property>
<property name="maximum">
<number>50</number>
Copy link
Contributor

Choose a reason for hiding this comment

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

Commit message says "..up to 60". Typo?
If the height of cells is measured in pixel, please add a px suffix to the actual value.

@esbrandt
Copy link
Contributor

The Reset defaults button in the preferences does not reset the column height to the default value.

@daschuer daschuer changed the title Add preference option to set the coloumn height Add preference option to set the column height Nov 25, 2014
@daschuer daschuer changed the title Add preference option to set the column height Add preference option to set the row height Nov 25, 2014
@daschuer
Copy link
Member Author

Now it works!
A max height of 50 works for me.

@ywwg
Copy link
Member

ywwg commented Nov 25, 2014

I would strongly prefer a font size choice that would drive the row height organically instead of a row height which won't change the library font size

@daschuer
Copy link
Member Author

@ywwg a changeable font size is a different use case.
This is manly to show a whole cover in a reasonable size in the library table.
If we would rise the font size as well, valuable infos will shifted out of sight due to the extended with of the texts on small screens.

@ywwg
Copy link
Member

ywwg commented Nov 25, 2014

Can you attach a screenshot of what this looks like?

@esbrandt
Copy link
Contributor

maxcoverwidth-compressed
In the picture, the max row height of 50 px (subject to this PR) is visible.
This is a partial screen shoot attached to another bug: https://bugs.launchpad.net/mixxx/+bug/1396056

@@ -37,7 +37,8 @@ WLibraryTableView::WLibraryTableView(QWidget* parent,
setHorizontalScrollMode(QAbstractItemView::ScrollPerPixel);

verticalHeader()->hide();
verticalHeader()->setDefaultSectionSize(20);
int coloumnHeight = m_pConfig->getValueString(ConfigKey("[Library]","RowHeight"), "20").toInt();
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed to renamecoloumnHeight here?

@ywwg
Copy link
Member

ywwg commented Nov 26, 2014

at least the text is centered. I guess it's ok

@daschuer
Copy link
Member Author

Thank you @esbrandt for the extreme cases. Here my desired ones:
(I think I will use a 30 x 30 cover. It is better for my visual brain then the 20 x 100 slice)
Height 20
default
Height 30
bildschirmfoto vom 2014-11-26 11 55 49
Height 40
bildschirmfoto vom 2014-11-26 11 59 19
Height 50
bildschirmfoto vom 2014-11-26 12 52 54

@daschuer
Copy link
Member Author

Merge?

@rryan
Copy link
Member

rryan commented Nov 26, 2014

I think you should address jus's comment about the spinbox prefix -- other
than that looks good to me.

On Wed, Nov 26, 2014, 3:24 PM Daniel Schürmann notifications@github.com
wrote:

Merge?


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

@daschuer
Copy link
Member Author

Oh, forgot that. Now it is place. Thank you for review.

daschuer added a commit that referenced this pull request Nov 26, 2014
Add preference option to set the row height
@daschuer daschuer merged commit 827b550 into mixxxdj:master Nov 26, 2014
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.

None yet

4 participants