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

[WIP] Allow skin styles for sidebar context menu #2297

Closed
wants to merge 33 commits into from

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Sep 23, 2019

Closed and split up:
Skins styles: #2337
C++ changes: #2338


IMO the right-click menus like in Tracks, sidebar and cover art are an annoying issue because the (often ugly) OS themes crashes into our designed Mixxx skins.
For Tracks and cover art a custom style can be applied by addressing QMenu and QMenu QCheckBox.

To make this work for the library sidebar context menu, too, I set the sidebar widget as parent for each library feature's context menus. For now, I did this for History and Computer feature.
Please check if this is the right approach, then I'll add this to the other features as well. (I need to clean up a bit anyway)

Also I removed a c++ stylesheet for the Tracks > Crates menu. This allows to override the OS styled checkboxes in the Crates menu, as well.

qmenu-styles_LateNight

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you for investigate this.
The solution looks good and I have left some comments.

src/library/browse/browsefeature.cpp Outdated Show resolved Hide resolved
src/library/browse/browsefeature.cpp Show resolved Hide resolved
src/library/browse/browsefeature.cpp Show resolved Hide resolved
@@ -240,6 +241,8 @@ void Library::bindSidebarWidget(WLibrarySidebar* pSidebarWidget) {
pSidebarWidget->slotSetFont(m_trackTableFont);
connect(this, SIGNAL(setTrackTableFont(QFont)),
pSidebarWidget, SLOT(slotSetFont(QFont)));
m_pSetlogFeature->bindSidebarWidget(pSidebarWidget);
Copy link
Member

Choose a reason for hiding this comment

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

Instead if name ach feature individual, you can use a loop over m_features

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, indeed. The entire feature section can be optimized. There's a related ToDo from @rryan about that, but I didn't look into it, yet.

@@ -2083,11 +2083,56 @@ WTrackProperty#SamplerTitle_mini {
###### Misc ##################################################
##############################################################*/

QToolTip {
QToolTip,
QMenu {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this still work with #2238? Do you need to style CueMenu and ColorMenu in addition to QMenu?

Copy link
Member Author

Choose a reason for hiding this comment

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

It works, see the screenshot in the PR description which I composed from the hotcue label branch and this branch.
There are a few minor adjustments necessary though.
The LateNight screenshot only shows that it works in general, I'll take care of the other skins and the fine tuning when #2238 is merged

@Be-ing
Copy link
Contributor

Be-ing commented Sep 29, 2019

Some merge conflicts have developed.

Is this still a WIP?

@ronso0
Copy link
Member Author

ronso0 commented Sep 29, 2019

My next step is to aply the solution for AutoDJ & History feature also to other features that have a sidebar context menu.
I'm not planning to overhaul the entire feature loading sequence here, like creating a separate method.

@ronso0
Copy link
Member Author

ronso0 commented Sep 29, 2019

Is this still a WIP?

yes: I still need to implement the solution for AutoDJ, Playlists & Crates.

@ronso0 ronso0 closed this Oct 5, 2019
@ronso0 ronso0 reopened this Oct 5, 2019
@ronso0 ronso0 changed the title [WIP] Allow skin styles for sidebar context menu Allow skin styles for sidebar context menu Oct 6, 2019
@ronso0
Copy link
Member Author

ronso0 commented Oct 7, 2019

I guess I can revert 6d9cb58 "default m_pMenu to nullptr", right?
If there's no sidebar widget there'll b no right-click and no menu so it doesn't matter at all.

@ronso0
Copy link
Member Author

ronso0 commented Oct 7, 2019

The menu now looks nice for all library features, and also for the hotcue menu #2238
Please test.

QMenu contextMenu;
contextMenu.addMenu(&crateMenu);
contextMenu.exec(globalPos);
m_pMenu->addMenu(m_pCrateMenu);
Copy link
Contributor

Choose a reason for hiding this comment

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

change m_pMenu to m_pCrateMenu

Copy link
Contributor

Choose a reason for hiding this comment

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

and there are more build errors in library.c around line 132 "BrowseFeature"

Copy link
Member Author

Choose a reason for hiding this comment

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

for AutoDJ crateMenu leftover was the issue.
m_pMenu and m_pCrateMenu are correct

m_pBrowseFeature is also fixed now

Copy link
Member Author

Choose a reason for hiding this comment

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

thx!

@nopeppermint
Copy link
Contributor

nice

  • styling for Shade could be better

@ronso0
Copy link
Member Author

ronso0 commented Oct 7, 2019

* styling for Shade could be better

Haa! probably because I didn't do anything in Shade so far..neither in Deere
@daschuer If yo don't have any style preferences I'll base the tooltip and context menu style on each scheme, like in LateNight.
@Be-ing What about Deere? Any ideas?

@ronso0
Copy link
Member Author

ronso0 commented Oct 23, 2019

The playlist context menus of Traktor and Rhythmbox playlist are not styled.

okay, so I guess I'll need to add the menu changes to library/baseexternallibraryfeature.cpp/h

I really don't have any idea how to accomplish this.
AFAICT BaseExternalLibraryFeature is loaded only once when each of the features is instantiated.
addFeature and bindSidebarWidget are separate functions in library.cpp.
How would I pass the sidebar widget pointer to each instance of BaseExternalLibraryFeature?

@Be-ing
Copy link
Contributor

Be-ing commented Oct 26, 2019

The menus in Deere do now show any indicator for submenus.

@ronso0
Copy link
Member Author

ronso0 commented Oct 26, 2019

The menus in Deere do now show any indicator for submenus.

yeah I know.. this is still WIP. I stumble over many issues, also unrelated to this PR, especially with scaled Library widgets.

@ronso0
Copy link
Member Author

ronso0 commented Oct 26, 2019

Also this has grown beyond 'simply styling the menus'. I tried to match WEffectSelector and QToolTip and QMenu (in various places: hotcues, library header, tracks, sidebar, cover art, editlines), and by fixing all those incosistencies (besides the scaling issues with fonts and library controls) this PR is now kind of a GUI round-house kick.
Stay tuned! And please test the effect selector on Windows and MacOS.

@daschuer Please check the library in Shade. I dimmed the table header buttons and IMO all color schemes are much more pleasant now.

@ronso0
Copy link
Member Author

ronso0 commented Oct 26, 2019

Did anyone ever try (or has any hints on how) to style the main menu bar?
This would the icing on the cake...

Btw: I still need help with passing sidebar widget to the baseexternallibraryfeature to style the iTunes and Banshee menu

@ronso0
Copy link
Member Author

ronso0 commented Oct 27, 2019

Haa! Got it, I think.
Instead of this complex passing around of m_pSideBarWidget I can parent the menu on the fly to QApplication::focusWidget(). will test this, it would simplify the entire story and solve the issue with iTunes and Banshee..

@ronso0
Copy link
Member Author

ronso0 commented Oct 28, 2019

I tested the suggested parent approach m_pMenu->setParent(anyWidget) with the Crates feature but this creates more issues than it could potentially avoid:

  • menu pops up at wrong place (cursorPos relative to widget 0,0 + global cursorPos)
  • menu is cropped by parent widget's geometry
  • menu won't close on selection or if I click outside the menu

That said, I'll have to try something else.

@ronso0
Copy link
Member Author

ronso0 commented Oct 29, 2019

I think I've found a way to address the external features, as well.
Nevertheless, I'll split off the skin changes from this PR and leave only a small qss snippet for testing that can easily be removed, because the skin changes are huge and I'd like to include the new AutoDJ drop-down.

@ronso0 ronso0 changed the title Allow skin styles for sidebar context menu [WIP] Allow skin styles for sidebar context menu Oct 29, 2019
@ronso0
Copy link
Member Author

ronso0 commented Oct 29, 2019

Skins styles: #2337
C++ changes: #2338

@ronso0 ronso0 closed this Oct 29, 2019
@ronso0
Copy link
Member Author

ronso0 commented Oct 29, 2019

Thanks for your help!

@ronso0 ronso0 deleted the sidebar-qmenu-styling branch October 29, 2019 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants