Skip to content

remove effects and samplers toggles from View menu#1432

Merged
daschuer merged 1 commit intomixxxdj:masterfrom
Be-ing:simplify_menu_bar
Dec 22, 2017
Merged

remove effects and samplers toggles from View menu#1432
daschuer merged 1 commit intomixxxdj:masterfrom
Be-ing:simplify_menu_bar

Conversation

@Be-ing
Copy link
Copy Markdown
Contributor

@Be-ing Be-ing commented Dec 21, 2017

These are available in skins. Defining the options in the View menu was getting in the way of skins showing effects and samplers by default.

@uklotzde
Copy link
Copy Markdown
Contributor

Good idea. In full-screen mode I would even prefer if the top menu bar is invisible.

Just some thoughts:

  • "Options" in the top menu bar looks a bit odd. It contains items for very different aspects.
  • "Preferences" would also fit into "File": Rescan Library | ... | Preferences | Exit
  • Both "Load Track..." and ""Vinyl Control" are related to a "Deck"
  • Do we need the "Load Track ..." actions at all?
  • "Full Screen" is actually a "View" option
  • Keyboard shortcuts belong to the UI, though the general term "View" doesn't really fit here

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Dec 21, 2017

In full-screen mode I would even prefer if the top menu bar is invisible.

Me too. I'd like to take it further and get rid of the menu bar entirely. But that's not happening in 2 days.

"Preferences" would also fit into "File": Rescan Library | ... | Preferences | Exit

👍

Both "Load Track..." and ""Vinyl Control" are related to a "Deck"

What are you suggesting? Adding a "Decks" menu? I would like to reduce the number of menus.

Do we need the "Load Track ..." actions at all?

I think we could get rid of them. A few times I have accidentally activated them and been annoyed by the file selection dialog popping up. Any objections?

"Full Screen" is actually a "View" option

Yes, but I don't want to have a View menu just for one item.

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Dec 21, 2017

Do we need the "Load Track ..." actions at all?

I think we could get rid of them. A few times I have accidentally activated them and been annoyed by the file selection dialog popping up. Any objections?

Hm, I never actually used them. But in rare cases I'd consider them a fallback for loading tracks that aren't in the library but for example on a USB stick when you're not familiar with or currently not capable of ( 🍸 🍻 🍷 or jittery/unreliable touchpad) file drag'n'drop. In either case I wouldn't dive down the "My Computer" tree in the library

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Dec 21, 2017

Some controls are not (yet) available in Shade: Cover Art, Preview deck :/

@daschuer
Copy link
Copy Markdown
Member

It is no deal to add button for preview deck, and cover art into Shade.

Unfortunately this change also breaks various forum and legacy skins. So I like to delay this to 2.2.

In general, I consider the skin Menüs only as a workaround, because they are not well integrated into the OSs. Maybe with Qt5, we have the option to build rich Os integrated menus, replacing the tradition menu bar. In any case we have the chance to rethink this topic once we are on qt5.

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Dec 21, 2017

Unfortunately this change also breaks various forum and legacy skins. So I like to delay this to 2.2.

Delaying this will not change that. IMO this must be done for the beta release because it is blocking the effects showing on first startup.

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Dec 21, 2017

In general, I consider the skin Menüs only as a workaround, because they are not well integrated into the OSs. Maybe with Qt5, we have the option to build rich Os integrated menus, replacing the tradition menu bar. In any case we have the chance to rethink this topic once we are on qt5.

We should not have the same things in both the menu bar and the skin settings menus. We should pick one or the other and our skins have implemented quite comprehensive settings menus. We have a completely custom GUI with our skins, so I think integrating with the OS would actually be odd.

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Dec 21, 2017

I'm not convinced yet that we need the same extensive menu in Shade and LateNight like we have it in Deere and Tango.
Let's just remove Effects and Samplers from the menu --toggles are available in all skins-- and not repeat the discussion from #927 and other PRs here (now) so that beta works fine, okay?

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Dec 21, 2017

That's a workable compromise for me for now. But I think we should remove the menu bar entirely in the future. Getting rid of it for 2.2 when we introduce a nice new library GUI would make sense to me.

@sblaisot
Copy link
Copy Markdown
Member

Application menu bar is translatable. Skin menu is not. So I vote to keep application menu bar until we have a solution.

@sblaisot
Copy link
Copy Markdown
Member

sblaisot commented Dec 21, 2017

And ronso0 proposition also seems a good compromise to me for the beta

Having those there was preventing effects and samplers being shown
on first startup. All skins have toggles for these anyway.
@Be-ing Be-ing changed the title remove redundant items from top menu bar remove effects and samplers toggles from View menu Dec 22, 2017
@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Dec 22, 2017

Okay, compromise implemented. I tested starting Mixxx with a fresh profile and it shows Deere with effects and one row of samplers as desired.

@daschuer
Copy link
Copy Markdown
Member

Thank you. LGTM

@daschuer daschuer merged commit 7d8bd5d into mixxxdj:master Dec 22, 2017
@nopeppermint
Copy link
Copy Markdown
Contributor

I just opened a Bug on lunchpad (as this is not possible in your github repo..)
https://bugs.launchpad.net/mixxx/+bug/1740499
source of this usability bug is this PR

QMenu* pViewMenu = new QMenu(tr("&View"));

QString mayNotBeSupported = tr("May not be supported on all skins.");
QString showSamplersTitle = tr("Show Samplers");
Copy link
Copy Markdown
Contributor

@nopeppermint nopeppermint Dec 29, 2017

Choose a reason for hiding this comment

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

Why not introduce here a simple variable which tell if the current selected Skin has a "Show Samplers" Button or not, depending on that, show here this option, that would also solve @daschuer remark

Unfortunately this change also breaks various forum and legacy skins

new Skins which have those Buttons sets a variable and then the option is not visible in the menu, otherwise it is visible in the menu.

if(selected_skin.has_sample_button==0)
{
QString showSamplersTitle = tr("Show Samplers");
QString showSamplersText = tr("Show the sample deck section of the Mixxx interface.") +
" " + mayNotBeSupported;
auto pViewShowSamplers = new QAction(showSamplersTitle, this);
pViewShowSamplers->setCheckable(true);
pViewShowSamplers->setShortcut(
QKeySequence(m_pKbdConfig->getValue(ConfigKey("[KeyboardShortcuts]",
"ViewMenu_ShowSamplers"),
tr("Ctrl+1", "Menubar|View|Show Samplers"))));
pViewShowSamplers->setStatusTip(showSamplersText);
pViewShowSamplers->setWhatsThis(buildWhatsThis(showSamplersTitle, showSamplersText));
createVisibilityControl(pViewShowSamplers, ConfigKey("[Samplers]", "show_samplers"));
pViewMenu->addAction(pViewShowSamplers);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it is not that simple because the tangled initialization process makes things fragile. I would rather add toggles to Shade and work towards removing the menu bar entirely.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nopeppermint would you like to add those buttons to Shade?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I may try to do this, if will try to add some kind of "extensive menu" as in deere/tango, thats seems to be the best solution for me

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shade has this button already.
The tiny text labels above the vu-meters.
They are not very usable though.

I am currently working on a Shade Skin that solved it. The concept will re-use the current looping region for basic quick access buttons for various features, and toggle buttons to open additional skin regions with fine access to each feature.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am happy to have a helping hand for this. Unfortunately I have no access to my device with my work in progress Shade skin. I will publish it tomorrow.

createVisibilityControl(pViewShowPreviewDeck, ConfigKey("[PreviewDeck]", "show_previewdeck"));
pViewMenu->addAction(pViewShowPreviewDeck);

QString showEffectsTitle = tr("Show Effect Rack");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here for "Show Effect Rack":
if(selected_skin.has_sample_button==0)
{
QString showEffectsTitle = tr("Show Effect Rack");
QString showEffectsText = tr("Show the effect rack in the Mixxx interface.") +
" " + mayNotBeSupported;
auto pViewShowEffects = new QAction(showEffectsTitle, this);
pViewShowEffects->setCheckable(true);
pViewShowEffects->setShortcut(
QKeySequence(m_pKbdConfig->getValue(
ConfigKey("[KeyboardShortcuts]", "ViewMenu_ShowEffects"),
tr("Ctrl+5", "Menubar|View|Show Effect Rack"))));
pViewShowEffects->setStatusTip(showEffectsText);
pViewShowEffects->setWhatsThis(buildWhatsThis(showEffectsTitle, showEffectsText));
createVisibilityControl(pViewShowEffects, ConfigKey("[EffectRack1]", "show"));
pViewMenu->addAction(pViewShowEffects);
}

Copy link
Copy Markdown
Contributor

@nopeppermint nopeppermint left a comment

Choose a reason for hiding this comment

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

add some ideas

@Be-ing Be-ing added this to the 2.1.0 milestone Dec 29, 2017
@Be-ing Be-ing deleted the simplify_menu_bar branch December 29, 2017 16:23
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.

6 participants