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

Breaks down the piano roll's toolbar into multiple smaller ones (#2286) #2287

Merged

Conversation

michaelgregorius
Copy link
Contributor

Solves #2286 by grouping the piano roll's actions into several toolbars. There
are now five groups / tool bars: "Transport controls", "Note controls",
"Copy paste controls", "Timeline controls" and "Zoom and note controls"
(this group is a bit mixed). Each group can be turned off and on using
the standard toolbar context menu provided by Qt.

The new default layout of the toolbars saves horizontal space by putting
the "Zoom and note controls" below the other toolbars. The toolbars can
be moved but their positions are not saved. Floating toolbars are
disabled, i.e. the toolbars will always stay inside the window (top,
bottom, left, right).

pianorollchanges-movabletoolsbars

@michaelgregorius
Copy link
Contributor Author

I have switched the other editor to use movable toolbars as well. Here is what they look like:
editor-toolbars

@tresf
Copy link
Member

tresf commented Aug 24, 2015

I'm a pretty big fan of this change. 👍

@grejppi
Copy link
Contributor

grejppi commented Aug 24, 2015

Oh, great. Go a step further: you should add that hideous handle to every single button too. That way the user has the option to set it however they like.

No but seriously:

  • What's the point of being able to move all of the toolbar?
  • Does it enhance your productivity?
  • Does it help your muscle memory?
  • What's the point anyway if your precious customized workflow gets reset every time?

Now look at the second screenshot you posted. In the piano roll and automation editors the tool buttons and view controls sit nicely on their own lines, and the editors are hardly wider than the rest.

Wouldn't that be enough?

@Wallacoloo
Copy link
Member

Although a bit harsh, @grejppi does have a point - the toolbar handles are ugly and the config being reset on each start takes away a lot of the other value in this PR.

One thing I think @grejppi may have missed is that this allows users to hide tools they don't use frequently (e.g. zoom/quantize), and this makes the interface cleaner and more compact for those users.

But I think perhaps it may be worth figuring out how to hide the handles before merging this PR. In the right-click menu you show, perhaps there could be an additional entry, "locked" (bolded, italicized, or separated with a <br> to indicate that it's not a toolbar entry), which, when checked, removes the handles so everything looks clean. If you can do that in this PR, and we default "locked" to being checked for the time being, then this features provides a net benefit with essentially zero drawbacks. Config saving could then be dealt with in a future PR.

I'm not sure if the implementation method I suggested is possible within Qt. If not, we could also go the route of either:

  • Handles are hidden by default. Long-pressing one of the buttons unhides its handle & allows it to be moved.
  • Handles are hidden by default but buttons can still be rearranged by dragging them. Worst-case, this could be accomplished with just a style hack.
  • Option in the application's view menu: "lock editor toolbars" which removes or adds the handles when checked/unchecked.

Personally, I prefer the context-menu "locked" entry as a solution.

@tresf
Copy link
Member

tresf commented Aug 25, 2015

image

QToolBar.setMovable(false); would hide the drag handles, which I agree have no benefit.

My original attraction to the toolbar grouping was I thought it fixed the overflowing toolbar problem when the MDI windows were smaller than the toolbar. After testing out the PR, I realize behavior is a bit different... when the toolbar is too small, it collapses onto itself, which is confusing with so many of them.

image

I do like the ability to hide the sections, but I'm not sure I like the collapsing behavior.

Oh, great.

You got your point across in the first section of your rant. We're trying to solve problems here. 👍

@Spekular
Copy link
Member

I think the ugly handles are a result of them being fully opaque. They're
meant to be semitransparent so they blend with the background like the
piano roll track handles.
On Aug 25, 2015 2:42 AM, "Tres Finocchiaro" notifications@github.com
wrote:

[image: image]
https://cloud.githubusercontent.com/assets/6345473/9456007/fb933858-4a9f-11e5-84a2-20aa570c2445.png

QToolBar.setMovable(false); would hide the drag handles, which I agree
have no benefit.

My original attraction to the toolbar grouping was I thought it fixed the
overflowing toolbar problem when the MDI windows were smaller than the
toolbar. After testing out the PR, I realize behavior is a bit different...
when the toolbar is too small, it collapses onto itself, which is confusing
with so many of them.

[image: image]
https://cloud.githubusercontent.com/assets/6345473/9456031/5c4a2634-4aa0-11e5-80ca-d6d699137658.png

I do like the ability to hide the sections, but I'm not sure I like the
collapsing behavior.

Oh, great.

You got your point across in the first section of your rant. We're trying
to solve problems here. [image: 👍]


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

@michaelgregorius
Copy link
Contributor Author

Here's what it looks like without the handles:
editor-toolbars-nohandle
The problem now is that the handles acted as separators and if you take them away it gets a bit crowded in the toolbar area. Adding spacer widgets at the end of the toolbar also feels wrong because logically the space would have to be in between the toolbars.

Shouldn't it be possible to style the handles via style sheets and to keep them?

@grejppi Calm down! 😉 I agree that the handles are not a thing of beauty but this is about trying to improve things by taking one step after another.

@Wallacoloo
Copy link
Member

@michaelgregorius Does the "Lock toolbars" button seem feasible? If so, it gives those of us who prefer the "crowded" look that possibility, while also allowing to keep some form of separation for those who prefer that, based on whether the toolbars are "locked" or not.

@michaelgregorius
Copy link
Contributor Author

@Wallacoloo Yes, I guess something like that could be implemented. I just don't know in how far Qt's code will honor all these settings. Let's for example assume that we have one locked toolbar in the middle of the toolbar area and there are other movable toolbars. What will happen if we move these toolbars in the area of the locked one? I also don't know whether Qt allows for non-movable bars to be in the middle of the area.

@grejppi
Copy link
Contributor

grejppi commented Aug 25, 2015

I just don't think this would make sense, especially in the B+B editor. Currently there are two distinct groups of buttons: on the right, the ones that deal with steps; and on the left, the ones that deal with tracks. It provides a visual clue about their relationship, and makes it almost impossible to click the wrong button.

Now in this design they are all grouped together, which requires a lot more effort to pick the right button.

@michaelgregorius
Copy link
Contributor Author

@grejppi I agree that the grouping was more distinct in the unchanged version. However, this might also be an indicator that the icons cannot be distinguished very well (most of them are blueish grey).

To provide an even more user friendly experience it might even be better to get rid of the tool buttons and to provide these actions in the areas where they are used, e.g. by having an add track button beneath the tracks similar to the button in the channel mixer. But that would be a lot more effort.

@grejppi
Copy link
Contributor

grejppi commented Aug 25, 2015

Why are you bringing icons to this? A different icon set wouldn't change the fact that the buttons are so close together your mouse will have to make smaller movements to get to the desired one.

In fact, this is the reason why the add buttons are on the right and the remove button is on the left.

I was the one who moved the buttons there in the first place. Initially I had placed the buttons in the opposite order (add on the left, remove on the right), but every time I moved the cursor to the top right corner to add steps I had to move the cursor back a tiny amount because I missed the button and was about to hit remove instead.

Then I swapped them and it was perfect.

It makes sense to have the buttons on the right for a simple reason: the patterns extend to the right. And usually one starts filling the pattern from the beginning (left) and moving to the right. Run out of steps? No problem, the add button is right in the direction where you're going. If this pull request is merged, you'll have to move your cursor back just to click one button, interrupting your workflow.

@michaelgregorius
Copy link
Contributor Author

@grejppi I brought the icons into this because I interpreted "a lot more effort to pick the right button" as "the buttons cannot be differentiated if they are so close to each other" and this might be caused by icons that are all very similar looking. I might have interpreted that wrong.

I agree with you that the separation made sense from a user's point of view: tracks are shown on the left so the buttons are on the left. Steps are on the right so their buttons are shown on the right. I think I have an idea how to solve this: I will make the two separate toolbars for track actions and step actions one toolbar again and reintroduce the stretch in that toolbar.

@michaelgregorius
Copy link
Contributor Author

Reintroduced the stretch. Current state:
editor-toolbars-correctedbbeditor

@tresf
Copy link
Member

tresf commented Aug 25, 2015

I just don't think this would make sense, especially in the B+B editor. Currently there are two distinct groups of buttons: on the right, the ones that deal with steps; and on the left, the ones that deal with tracks. It provides a visual clue about their relationship, and makes it almost impossible to click the wrong button.

Now in this design they are all grouped together, which requires a lot more effort to pick the right button.

[...]
Why are you bringing icons to this? A different icon set wouldn't change the fact that the buttons are so close together your mouse will have to make smaller movements to get to the desired one.

In fact, this is the reason why the add buttons are on the right and the remove button is on the left.

I was the one who moved the buttons there in the first place. Initially I had placed the buttons in the opposite order (add on the left, remove on the right), but every time I moved the cursor to the top right corner to add steps I had to move the cursor back a tiny amount because I missed the button and was about to hit remove instead.

Then I swapped them and it was perfect.

Couldn't this all haven summarized "Can we still float Add /remove BB steps to the right?"

I feel like there's some form of spiteful territorial undertone to this PR and it gets us nowhere.

I had originally liked this proposed idea because I thought it fixed the stacking/overflow problem illustrated in #387, but now that I realize it doesn't do overflowing at all, the only benefit is the ability to hide unused components and I think that needs to be weighed against the strange undesirable collapsing behavior that it introduces via #2287 (comment), 2nd screenshot.

-Tres

@michaelgregorius
Copy link
Contributor Author

@tresf It also fixes #2286 by adding the second row of toolbars. 😉

To get rid of the collapsing behavior and to implement the overflow the current toolbar implementation would likely have to be replaced with something custom.

@StakeoutPunch
Copy link

Just a quick question: are those screenshots or markups? The gradient is not constant throughout the toolbar, resetting for each cluster of buttons. It really breaks the look.

@michaelgregorius
Copy link
Contributor Author

@StakeoutPunch I guess that's somehow caused by the style sheet. I will have to check whether it behaves more consistent with the stylesheet disabled. If it also looks strange in that case then it is likely a Qt issue or caused by some code of the LMMS widgets.

@michaelgregorius
Copy link
Contributor Author

I have fixed the gradient problem and have also fixed the problem with the timeline controls which had a smaller size than all other buttons.

Furthermore, I took the liberty to adjust the CSS for the QToolBar and the QToolButton. This is the current state of the pull request:
editor-toolbars-css-new

  • Before these adjustments all tool buttons looked like they were pressed which made it for example hard to distinguish which mode is selected in the piano roll editor.
  • The hover effect is now a bit more subtle so that it looks not as intensive as when a button is finally pressed / checked.
  • I have adjusted the radii of the play and stop buttons to be symmetric so that they do not look as "skewed" as before.
  • The normal buttons also have a bit more radius now.

@Wallacoloo
Copy link
Member

@michaelgregorius 👍 on your CSS changes.

@tresf
Copy link
Member

tresf commented Sep 6, 2015

Removing fixed sizes 👍
Better indication when pressed 👍
Overall style 👍

@michaelgregorius
Copy link
Contributor Author

@Wallacoloo, @tresf Shall I rebase, squash and merge then?

@tresf
Copy link
Member

tresf commented Sep 11, 2015

I think you've addressed everyone's wants/needs, made it look better and given plenty of time for feedback. Good in my book.

The piano roll's actions have been grouped into several toolbars. There
are now five groups / tool bars: "Transport controls", "Note controls",
"Copy paste controls", "Timeline controls" and "Zoom and note controls"
(this group is a bit mixed). Each group can be turned off and on using
the standard toolbar context menu provided by Qt.

The new default layout of the toolbars saves horizontal space by putting
the "Zoom and note controls" below the other toolbars. The toolbars can
be hidden and shown via the context menu but these states are not
stored.

The "Song editor", "Beat+Bassline editor" and "Automation editor" have
been switched to using movable toolbars as well.

Adjusted the Editor class to have some other defaults for the "Transport
controls".

Added some methods to add toolbars to editors and changed the other
editors' code to use it. This way the properties of a standard editor
toolbar can be changed in a simple and central way.

Fixed the size of the timeline control buttons which are implemented as
NStateButton. Previously these had a fixed size and appeared smaller than
all other buttons. Now they behave like other ToolButton with respect to
the size. Also removed a fixed size call in ToolButton itself.

Made some adjustments to the CSS for QToolBar and QToolButton:
* Switched the QToolBar CSS to a vertical gradient and also increased
the padding to 2px on the way.
* Previously all buttons looked like they were pressed. This was fixed by
using the same linear gradient that is used for the QToolBar.
* The hover effect for QToolButtons is a bit more subtle now and looks
less intensive as when the button is pressed / checked.
* Gave the normal buttons a bit more radius.
* Adjusted the radii of the special play and stop buttons to be
symmetric so the do not look skewed.
michaelgregorius added a commit that referenced this pull request Sep 11, 2015
Breaks down the piano roll's toolbar into multiple smaller ones (#2286)
@michaelgregorius michaelgregorius merged commit 36a6fb3 into LMMS:master Sep 11, 2015
@michaelgregorius michaelgregorius deleted the 2286-piano-roll-toolbars branch September 14, 2015 19:29
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