Modernize the toolbar #5261
Modernize the toolbar #5261
Conversation
🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩
Windows
Linux
macOS
🤖{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://6757-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.701-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/6757?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://6754-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.701-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/6754?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/qc6sl59pmn5boyou/artifacts/build/lmms-1.2.1-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/33239093"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/dbumvqq8lt3i6uj9/artifacts/build/lmms-1.2.1-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/33239093"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://6756-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.701-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/6756?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://6755-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.701-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/6755?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "193ff61820ddc6a78832831d410ce20bf7f0298e"} |
Reflexe
added
needs code review
|
Very nice work! I hope to review it in the next week or so. Thanks! |
|
Hi, I just saw this project and it's great! but where would you put the oscilloscope? I made a montage in gimp to give you a suggestion.
I suggest that you place the marker next to the buttons to take advantage of space and to make it more pleasing to the eye, also to place the oscilloscope, while the empty space on the right can be reserved for other future projects. regards |
|
If both min:sec & Bar:Beat clocks can be Shown side-by-side simultaneously , then it would look to be more professional. |
|
@Sawuare Ah, you're right! I'll update the file descriptions and copyright information. What are the conventions for adding in developer names and such? @Gabrielxd195 I'm not sure we should change it back to that layout, as it would ultimately look cluttered again which is exactly what I wanted to avoid. Note that my implementation has been based on budislav's design (see #1911) — I think we should adhere to that unless we have a good reason not to. Any additional widgets shouldn't be added to the right of the widget, but be properly incorporated in an intuitive position in the toolbar. Doing otherwise would be bad user experience. For that reason, I'll probably eventually move those old buttons as well, but that's not for this pull request. For the same reason, I think we also shouldn't add back the oscilloscope, since it serves no real function in the toolbar other than to take up space. The only reasons to use that oscilloscope is to view the volume output levels (which the 'OUT' widget in the upper right corner does much more efficiently), since the output was way too stuttery (and the widget too small) to provide any real information about the waveform. The toolbar just isn't the right place for this oscilloscope. @CaBachhav Interesting idea. I'll have to see how well it'd fit in the window, since I would want to avoid wasting too much real-estate when laying it out horizontally. I think this would have to be discussed with the design folks. Perhaps it can be designed as an additional mode for the clock widget in which the beat/bar and min/sec are stacked vertically, but then the numbers would have to be smaller. |
|
Oh, general question: I'm not too familiar with the travis-ci setup over here. Does anyone know why it's giving an error? |
|
You can ignore it. We are in the process of getting rid of it.
…On Mon, 21 Oct 2019, 13:46 Lathigos, ***@***.***> wrote:
Oh, general question: I'm not too familiar with the travis-ci setup over
here. Does anyone know why it's giving an error?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#5261?email_source=notifications&email_token=AFOMGKAKMMP4VARJ3IT3GVDQPWB7VA5CNFSM4JCVUJ3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBZ4R5Q#issuecomment-544458998>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFOMGKF63HJFCKIVIOAOLP3QPWB7VANCNFSM4JCVUJ3A>
.
|
|
Works nicely for me, everything was fine as it should be in terms of usability. But, but. Those buttons on left side looks quite "old" with new toolbar mechanics and design, they are cramped in small space. |
|
@qnebra Yeah, I totally agree. I haven't touched the buttons yet because there are too many of them — and in the single-window design at least half of them would disappear anyway — but perhaps I could change some things to make it look better. I'm currently working on a mockup .png to show a possible solution, but it's a bit problematic with the amount of buttons there are. Perhaps some buttons could be removed to make the toolbar less cluttered? Most of the top buttons have their behaviour mirrored in the menubar anyway, and some of the bottom buttons as well (namely 'show piano roll', 'show BB editor', and 'show automation editor' can all be activated by double-clicking the respective track in the Song Editor, which I think is more intuitive anyway). So perhaps we should reconsider what buttons we really want to keep as quickbuttons in the toolbar. That is, what actions really warrant being able to do it in one click as opposed to two? Things such as creating a new template-based file is not something I do every five minutes in LMMS, mostly just once per session. Those stuff could be removed as quickbuttons and just remain in the menubar instead. |
|
@Lathigos The copyright notice here is generally of the form: |
|
If all the file operations were removed (since they're accessible in the
File menu, and some via shortcuts), the remaining top row buttons would
probably fit fine on the bottom row. This could let us reduce the height of
the toolbar, if desired.
…On Mon, Oct 21, 2019, 18:10 Lathigos ***@***.***> wrote:
@qnebra <https://github.com/qnebra> Yeah, I totally agree. I haven't
touched the buttons yet because there are too many of them — and in the
single-window design at least half of them would disappear anyway — but
perhaps I could change some things to make it look better. I'm currently
working on a mockup .png to show a possible solution, but it's a bit
problematic with the amount of buttons there are.
Perhaps some buttons could be removed to make the toolbar less cluttered?
Most of the top buttons have their behaviour mirrored in the menubar
anyway, and some of the bottom buttons as well (namely 'show piano roll',
'show BB editor', and 'show automation editor' can all be activated by
double-clicking the respective track in the Song Editor, which I think is
more intuitive anyway).
So perhaps we should reconsider what buttons we really want to keep as
*quickbuttons* in the toolbar. That is, what actions *really* warrant
being able to do it in one click as opposed to two? Things such as creating
a new template-based file is not something I do every five minutes in LMMS,
mostly just once per session. Those stuff could be removed as quickbuttons
and just remain in the menubar instead.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#5261?email_source=notifications&email_token=ACEBLGWKAM5Q442PJO3INTLQPXIA7A5CNFSM4JCVUJ3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEB24BFY#issuecomment-544587927>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACEBLGXMM7VS2CZAUX5TBX3QPXIA7ANCNFSM4JCVUJ3A>
.
|
|
Hi @Lathigos, nice work! Here are some of my observations:
|
Create from template was rarely used I think. Also I made some small amateur mockup, with how I will see this in lmms. Also, also, I was kinda forgot about making toolbar thinner by 7 px (that was a lot of space ;) )
|
|
@qnebra I was thinking of something like this, perhaps (just a quick mockup): The file operations and stuff would be on the left hand side of the widget, and the window visibility buttons would be flushed to the right side of the window. This would also help distinguish their different functionalities. Of course, we could then indeed remove the margin at the top and bottom of the widget to make the entire toolbar more thin! 😃 Perhaps in the final design I can move the metronome button into the central widget as opposed to next to it, more like the original design. And perhaps I should make all buttons the same size, I don't know.
Alright, many thanks for the feedback, I'll work on them!
That'd be a great addition! If I find the time, I'll work on that as well.
This is the only point which I fundamentally disagree about. I understand that everybody is used to having a left-flushed layout in a toolbar (it took me some time to get used to it as well), but it's an outdated design choice. Once we move towards a single-window design, the centered widget looks much more modern, clean, and above all intuitive — as opposed to cluttered and confusing. Besides, the unused space on the toolbar is wasted space either way. So I'll experiment around with it, but I'm not sure I like the idea unless there's some really compelling reason to do it. |
|
Great work here! I have a few ideas regarding the colors and contrast. Will comment with some hexes as soon as I get the chance. |
Great idea for me, currently this left side area was really crowded, and right side feels really empty.
And maybe flat color? But this gradient in your mockup looks really good. |
|
I also wanted to chime in on the matter of scaling that I agree on the center alignment will work on large monitors now that the idea is to put the icons on either side. If you are proposing to remove the oscilloscope, do we have an idea of where it may be placed or accessed instead? Can we say that this is universally unused and pointless or could this be something which users would find missing? We need to consider this design with the smallest screen scenario as well as the largest, especially if the point is to go in the direction of scalable UI. |
I suppose we could do flat color once we have a single-window UI, but for now I think the gradient is more in tune with the rest of the UI of LMMS so that the toolbar won't clash too much.
That'd be great, thanks!
I would personally vote for "universally unused", since the widget was too small to comfortably use and the waveform wasn't centered, so you'd only see a flickering line giving no information about the waveform whatsoever other than volume (which is now included in the widget). I think it's safe to remove, but in the end some sort of spectrum visualizer could be useful.
Yeah, I agree. I wanted to make it possible for the window to become at least as small as half the width of a 1920x1080 screen, so that the window can be "docked" on one half of the screen in modern operating systems on standard monitors. This probably also covers the smallest-screen scenario, which I guess would be 1024x768? |
|
@Umcaruje Alright, I finally got around to coding a little bit again. I started experimenting with removing the zeroes from the time sig widget. It looks good when there's a single digit on both sides, as well as when there's two digits at both sides. But then I noticed this annoying case, a single digit on one side and two digits on the other:
Now the time sig widget doesn't look centered at all. Of course, one could center the entire widget, but then the "/" would start shaking around as you scroll through the spinboxes and that looks really awful. So I propose to keep the zeroes in the time sig widget. |
|
The oscilloscope is useful as it can detect clipping & DC bias and examine timbre & stereo separation, so do not remove it. |
|
I think that if it's really needed to remove the oscilloscope, it should become a lmms native plugin |
|
Is there a reason that the oscilloscope could be added to the centerpiece along with the volume controls now that we have found a better layout? Otherwise, if it can remain as a separate plugin, that may work. I think it is best to not remove it entirely in any case. |
|
Here's a rough mockup of what direction I think we should take for the colors which gives some nice contrast improvements and matches the single-window design a bit closer: Rough mockup: Single window: @Umcaruje thoughts? Note: For whatever reason, my inkscape HSL values are not reflecting the right numbers when converted so I will give hex codes after that is resolved on my end for good measure. |
|
There was a lot of discussion on this, but it's been silent for a while now. Is there currently anything blocking this from being merged in? |
|
I'd love to see @Lathiagos update the original post with a screenshot of what this PR currently looks like. That would make it easier to do a review without necessarily building/installing the PR. There are also a few merge conflicts to be fixed, according to github. |
|
Basically, at this moment toolbar with this PR merged is far less usable than in previous version. You had gigantic empty space with few buttons, and all file operations hidden in "File" menu. I will edit this comment to post screenshot. |
|
Afaik it has been reached a consensus what should be done with this PR. Further things were postponed. After that @Lathigos made the changes. Since then it has been waiting to be merged or for a final review. Don't know who is responsible or whether they are aware of the state. |
|
Current look of toolbar with this PR. |
Yep, for now, we came to the best compromise and conclusion with the information available. We decided to go ahead with this layout and make possible changes or enhancements as another stage.
This looks good to me, this should be ready to test and merge? |
|
There seem to be conflicting files. I think the original dev has to merge the latest master? Also I'm willing to test, will get onto it after a while. |
|
The code needs review as well, and there are surprisingly big changes here. Some of them look suspect to me, but I haven't had time to look into it myself. |
|
Alright, I'll try some code review as well. I'm not the best coder myself, but I'll look for suspicious changes. |
| m_peakLeft = peakSamples.left * v; | ||
| m_peakRight = peakSamples.right * v; |
There was a problem hiding this comment.
For reference: there used to be a QMax to ensure it doesn't cross some maximum amount (peakSamples.left or .right respectively I believe)
| void CPULoadWidget::paintEvent( QPaintEvent * ) | ||
| { | ||
| if( m_changed == true ) | ||
| { | ||
| m_changed = false; | ||
|
|
||
| m_temp.fill( QColor(0,0,0,0) ); | ||
| QPainter p( &m_temp ); | ||
| p.drawPixmap( 0, 0, m_background ); | ||
|
|
||
| // as load-indicator consists of small 2-pixel wide leds with | ||
| // 1 pixel spacing, we have to make sure, only whole leds are | ||
| // shown which we achieve by the following formula | ||
| int w = ( m_leds.width() * m_currentLoad / 300 ) * 3; | ||
| if( w > 0 ) | ||
| { | ||
| p.drawPixmap( 23, 3, m_leds, 0, 0, w, | ||
| m_leds.height() ); | ||
| } | ||
| } | ||
| QPainter p( this ); | ||
| p.drawPixmap( 0, 0, m_temp ); | ||
| } |
There was a problem hiding this comment.
Why's paint event deleted? Didn't exactly understand
After testing, here's what I found
Screenshot
This is all I found after some testing. I can do some more to see what else is wrong though I think beyond these points there's nothing much else to worry about. @Spekular, I'll leave the code review to you since I don't think I can manage figuring out all of that long code. @Lathigos, sorry for being so late, and great thanks for your contribution! |
|
I still don't have time for a full review right now, but just as I suspected, IntegerSpinBox is a copy paste of LcdSpinBox. Making a new, mostly duplicated class in order to change how it's drawn is not a good solution. The common functionality should be in a single class (probably a new abstract superclass) and only the difference (draw method) should be separate. This way we're not repeating ourselves (having to make all future changes to this class in two places). This issue alone makes the PR unmergeable. |
|
I spotted a bug: the reset function is completely useless for text-based widgets atm, it just suggests again the current value that was set. Example with master volume: Obviously is the same also with tempo, time signature and pitch. |
|
Im late to this discussion but here's what i found
So here's my take on what's to do with this pr:
This is just my opinion. If you differ, feel free to discuss. The point of this reply is to initiate discussion. |
|
will this ever be pulled in after the requested changes by Sawuare? |
|
Don't we need to fix the merge conflicts. @Lathigos is it fine if someone else take over this pr? |
|
@Rossmaxx TBH Lathigos doesn't seem to be active on GitHub for more than 4 years now. I might be mistaken, but the data would seem to indicate so I think. |
|
In that case, I'll try taking over. |
|
I did look into this a bit. seems like it's better to start from scratch and refactor the entire toolbar code into it's own file on the way. But I'm postponing the resurrection to after 1.3. |
This pull request updates the main toolbar to be much more modern, and moves the looks of LMMS more towards @budislav's design (see #1911).
There are some key changes from that design:
This is what it looks like in a full window. Note that the widget is dynamically positioned to the center of the screen:
