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

Lib redesign skin fixes #1671

Merged

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented May 19, 2018

Edit I removed the feature label from the sidebar in all skins. The feature title is available via BreadCrumbs above tracks table.

Tango:

  • move BreadCrumbs back above tracks table
  • style AutoDJ spinbox like beatsize spinboxes
  • fix minor issues

Deere: move Lib maximize toggle above WButtonbar, push it next to Searchbox if sidebar is (almost) collapsed

2 1 1-skin-fixes-deere

LateNight:

  • fix feature button bar style
  • fix feature buttons & feature backgrounds
  • style AutoDJ spinbox like beatsize spinboxes
  • add translucent scrollbar handles to sidebar
  • redesign toolbar divider to differ from splitter handle

2 1 1-skin-fixes_latenight

Please test!

@Be-ing
Copy link
Contributor

Be-ing commented May 19, 2018

screenshots?

@ronso0
Copy link
Member Author

ronso0 commented May 19, 2018

screens added

Copy link
Contributor

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

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

The text above the control pane in LateNight is cut off for me with Qt5 and a scale factor of 2.5.
screenshot from 2018-05-19 17-08-58

Interestingly the feature name text above the track table does not show at all in Tango:
screenshot from 2018-05-19 17-18-01
While we have only one feature showing, I think this is preferable. If you could make the other skins do this, that would be nice.

Aside from that I noticed some minor code quality nitpicks.

-->
<Template>
<WidgetGroup>
<ObjectName>TEST2</ObjectName>
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you forget to remove this line?

a) If the library sidebar is big enough they are shown above the tree view.
b) If the sidebar is too narrow, they are shown above the tracks table,
next to the searchbox
Depending on sidebar size, the WidgetStacks flip '[Tango],lib_sidebar_collapsed'
Copy link
Contributor

Choose a reason for hiding this comment

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

[Tango]?

<Children>

<Template src="skin:library_sidebar_tester.xml"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

tester?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can name it library_sidebar_width_detector.xml

Copy link
Member Author

Choose a reason for hiding this comment

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

or we move the button bar outside the splitter, then this SizeAwareStack can be removed as well

</WidgetGroup>

<Splitter>
<ObjectName>HoriSplitter</ObjectName>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please spell out "HorizontalSplitter"

<Layout>horizontal</Layout>
<SizePolicy>me,me</SizePolicy>
<Children>

<Splitter>
<ObjectName>LibrarySplitter</ObjectName>
<ObjectName>VertSplitter</ObjectName>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please spell out "VerticalSplitter"

a) If the library sidebar is big enough they are shown above the tree view.
b) If the sidebar is too narrow, they are shown above the tracks table,
next to the searchbox
Depending on sidebar size, the WidgetStacks flip '[Tango],lib_sidebar_collapsed'
Copy link
Contributor

Choose a reason for hiding this comment

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

[Tango]? Do you mean [Skin]?

@ronso0
Copy link
Member Author

ronso0 commented May 21, 2018

The text above the control pane in LateNight is cut off for me with Qt5 and a scale factor of 2.5.

yep, I used a different hack to hide it. works @100% but scaling doesn't respect all sizes declared in css. Changed it so it works like in Tango now.

While we have only one feature showing, I think this is preferable. If you could make the other skins do this, that would be nice.

Sure. Also applied some basic fixes in Shade.
@daschuer Do you plan to update the Shade library styling? Apart from the feature icons it looks like qt default.

ronso0 pushed a commit to ronso0/mixxx that referenced this pull request May 21, 2018
@ronso0 ronso0 changed the title Lib redesign skin fixes [WIP] Lib redesign skin fixes May 21, 2018
@ronso0
Copy link
Member Author

ronso0 commented Jun 14, 2018

@Be-ing
Conflicts are resolved and the library is laid out the same way in all skins now.
Please test.

@ronso0
Copy link
Member Author

ronso0 commented Jun 14, 2018

Though I'm wondering why the Library preview button in LateNight has some kind of minimum width defined: the preview column can be resized but the button itself occupies 65px anyway, so the icon is shifted and only partially visible if the preview column is too small

@nopeppermint
Copy link
Contributor

in latenight if you disable "show text in sidebar icon" it doesn't rescale automatically, you need to change the window size for this..

latenight

the bpm "locked" is somehow broken:

bpm

@nopeppermint
Copy link
Contributor

Shade Autodj Fade time is almost invisible
shade

@nopeppermint
Copy link
Contributor

nopeppermint commented Jun 16, 2018

library styling for latenight is somehow broken:
latenight_hover compared to 2.1
latenight_2 1

  • hover for preview button is extended over his column and the played checkbox locks better in 2.1

@nopeppermint
Copy link
Contributor

Deere preview button is broken
deere

@ronso0
Copy link
Member Author

ronso0 commented Jun 17, 2018

Deere preview button is broken

Strange, didn't touch it. Fixed.

I'm wondering why the Library preview button in LateNight has some kind of minimum width

Got it! It was a leftover/merge conflict:

WBaseLibrary QPushButton {
  min-width: 65px;
  min-height: 22px;
  max-height: 22px;

in latenight if you disable "show text in sidebar icon" it doesn't rescale automatically

Can't confirm, behaves basicallly the same in all skins.
Though I can't test with most recent lib-redesign branch because I messd up my qt souces somehow..

Shade Autodj Fade time is almost invisible

Those special library features are still unstyled as I was waiting for some feedback from @daschuer
So it's the OS them issue again. I'll take care of that and the feature buttons soonish

@ronso0
Copy link
Member Author

ronso0 commented Jun 19, 2018

Now Shade library is styled as well.
Please test

@nopeppermint
Copy link
Contributor

it seems that there are now "Conflicting files"

@nopeppermint
Copy link
Contributor

nopeppermint commented Jun 19, 2018

latenight:
fixed:

  • played checkbox
  • bpm "locked"

still open:

  • hover for preview button is extended over his column

And I got a crash while I was trying to load a random track to autodj

Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Warning [Main]: Could not load random track.
Segmentation fault

as Autodj source I had an empty crate, but I was not able to reproduce this crash..

@nopeppermint
Copy link
Contributor

fixed:

  • Deere preview button

Though I can't test with most recent lib-redesign branch because I messd up my qt souces somehow..

I just compiled your branch, or should I compile the lib-redesign branch and copy only the skin folder from your branch?

@ronso0
Copy link
Member Author

ronso0 commented Jun 20, 2018

it seems that there are now "Conflicting files"

Fixed..again. @Be-ing @daschuer Please review and merge this so I don't need to fix the merge conflicts (Edit and screwed up styles introduced by auto-merge) again.
Any further issues can be fixed in a follow-up PR

@ronso0
Copy link
Member Author

ronso0 commented Jun 20, 2018

And I got a crash while I was trying to load a random track to autodj

Not related to this PR as it changes skin files only.
Please post those issues to lib-redesign branch or to respective zulip chat.

@ronso0
Copy link
Member Author

ronso0 commented Jun 20, 2018

Screen shots:
lib-redesign-skin-fixes-deere

lib-redesign-skin-fixes-latenight

lib-redesign-skin-fixes-shade_dark

lib-redesign-skin-fixes-tango

@ronso0
Copy link
Member Author

ronso0 commented Jun 20, 2018

@nopeppermint
Due to the conflicts I just rebased to jmigual-lib-redesign, so it shouldn't make any difference which branch you compile as long as you use the skins from here.

@nopeppermint
Copy link
Contributor

@ronso0 nice !
there might be more minor fix but in general it looks good on all skins under Linux Mint x64

one thing I see compared with branch 2.1, the title of the currently selected widget (library, autodj... ) needs a lot of vertical space.

compare_2 1_2 2

I think one's the suggested layout in #1117 (comment) is implemented, we need less space as well

Single library after first start: search box next to bread crumbs (goes below if table width gets too small to show both side by side))

@Be-ing
Copy link
Contributor

Be-ing commented Jun 23, 2018

There are still some small issues, but those can be fixed later. This branch has enough improvements to be merged now. Thanks @ronso0, and thanks @nopeppermint for review.

@Be-ing Be-ing changed the title [WIP] Lib redesign skin fixes Lib redesign skin fixes Jun 23, 2018
@Be-ing Be-ing merged commit 1ede6dd into mixxxdj:jmigual-library-redesign Jun 23, 2018
@Be-ing
Copy link
Contributor

Be-ing commented Jun 23, 2018

Minor issues in Tango:

  • The cover art toggle icon feels out of place and IMO wastes space. Couldn't it be moved to the skin settings menu? None of the other skins have it by the feature buttons.
  • Although I can use the splitter between the control pane and track table, I don't see it so it would be difficult to discover if I was not looking for it.

@ronso0
Copy link
Member Author

ronso0 commented Jun 24, 2018

The cover art toggle icon feels out of place and IMO wastes space

yeah, this can be removed. I notice I don't use it at all anymore. It was intended as a quick toggle instead of going to the View menu, so IMO it doesn't make sense to have it in Skin Settings.

The splitter graphic shows up when I load now-merged & updated Tango from lib-redesign branch. There were issues after rebasing but it should be fine now.

@ronso0 ronso0 deleted the lib-redesign-skin-fixes branch June 24, 2018 13:10
@ronso0
Copy link
Member Author

ronso0 commented Jun 24, 2018

follow-up is #1723

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants