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

Fix EffectRackView appearance (GUI). #5766

Merged
merged 2 commits into from
Nov 7, 2020
Merged

Conversation

DigArtRoks
Copy link
Contributor

afbeelding afbeelding

* Elide the name of the effect when it tends to be too big. (LMMS#5752)
* Evenly space the controls (W/D, Decay Gate). (LMMS#5750)
* Show the scrollbar in the default theme to close the gap. (LMMS#5752)
* Reduce the gap between the effect and the scrollbar. (LMMS#5757)
* Use always the same width for the EffectRackview (InstrumentTrack and SampleTrack) to avoid gaps or cutoffs of the background.
* Widen the background in the default theme.
* Widen the embossed space in the background in the classic theme to fit the controls.
@LmmsBot
Copy link

LmmsBot commented Nov 4, 2020

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://10196-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-754%2Bg86250d2-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10196?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://10198-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-754%2Bg86250d281-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10198?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://10195-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-754%2Bg86250d281-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10195?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/t4ofyola154yulgm/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36142671"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/8enjwsatgee85pn7/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36142671"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://10197-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-754%2Bg86250d281-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10197?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "e3c6b4fb765f47cd8eb0f8911d21e1684589bd36"}

@superpaik
Copy link
Contributor

It seems perfect to me. Tested on W10/64 bits (both MSVC and Mingw) and with different display resolutions.
Just one comment. You said (point 5): Use always the same width for the EffectRackview (InstrumentTrack and SampleTrack) to avoid gaps or cutoffs of the background.
I think that applies to Effects Chain in FX mixer as well. Or at least the width seems to be the same, which I find correct.

Thanks.

@DigArtRoks
Copy link
Contributor Author

Thansk for trying it out.

Yes the width of the EffectRackView is now the same for FxMixer, InstrumentTrack and SampleTrack.

I initially thought the issue was flagged for the FxMixer alone. After fixing that one, I still saw similar issues in the InstrumentTrack and SampleTrack. Hence my comment in point 5 only referring to the latter 2.

It turned out that for the 3 locations where the EffectRackView is used different widths (and heights) are used. So I fixed the width for all 3 to the same value. The widget does not really allow horizontal resizing due to the fixed background that is used for an EffectView. Either more space between the EffectView and the scrollbar appears or the background is cropped.

The width remains as was for the FxMixer, gets 5 pixels wider for the SampleTrack (which still fits) and is reduced about 5 pixels in the InstrumentTrack.

@Spekular
Copy link
Member

Spekular commented Nov 4, 2020

I've put together a before and after (Ignore the fact that my before image doesn't gray out disabled knobs):
image

I think the classic theme effects now end up a bit too close to the scrollbar, I reckon around halfway between the old width and the new one would look good.

Similarly, I perceive the right side padding on the default theme effects to be narrower than the left, even though they're both 4 pixels. Perhaps 5 pixels on the left would give the scrollbar a little more space to breathe (since it stands out more than the outline). Here's a mockup + comparison of how that would look:
image
(PR on the left, mockup on the right, space in the middle so you can focus on one at a time)

Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

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

Code looks good.

I do wonder, however, whether the knobs are a little too far apart now. Also, it looks like a dark line has appeared at the bottom right of the classic theme effect background, although it's not very noticeable.

* Reduce the background for the default theme by 1 pixel.
* Reduce the background for the classic theme by 2 pixels and remove the darker line at the bottom right.
* Reduce the width of long names of the plugin also by 2 pixels.
* Put the controls 2 pixels closer to each other.
@DigArtRoks
Copy link
Contributor Author

Thanks for the reviews.

I did an additional commit.

  • Reduce the background for the default theme by 1 pixel.
  • Reduce the background for the classic theme by 2 pixels and remove the darker line at the bottom right.
  • Reduce the width of long names of the plugin also by 2 pixels.
  • Put the controls 2 pixels closer to each other.

This is how it looks now:
afbeelding afbeelding

@superpaik
Copy link
Contributor

Yeah, much better now!

Copy link
Member

@Spekular Spekular left a comment

Choose a reason for hiding this comment

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

LGTM both code and design wise.

@sound8
Copy link

sound8 commented Nov 7, 2020

I was freaking out because I found a whole bunch of other little things to go with my posts, I created all these graphics, I rushed here to tell you guys.... and then I found this page where you not only found them all but applied them! :)
Wow, excellent job guys, everything looks great
Thank You for all this work.
Should I close my original posts?

@Spekular
Copy link
Member

Spekular commented Nov 7, 2020

Should I close my original posts?

We usually only close issues when the fix is merged, so leave it open :)

@Spekular
Copy link
Member

Spekular commented Nov 7, 2020

Actually though, Dom gave the changes after his latest review a thumbs up reaction, so I suppose this has two up to date + passing reviews and no CI failures. Merging!

@Spekular Spekular merged commit a6e3958 into LMMS:master Nov 7, 2020
@DigArtRoks DigArtRoks deleted the EffectRackView branch November 7, 2020 20:17
IanCaio pushed a commit to IanCaio/lmms that referenced this pull request Mar 28, 2021
* Fix EffectRackView appearance (GUI).

* Elide the name of the effect when it tends to be too big. (LMMS#5752)
* Evenly space the controls (W/D, Decay Gate). (LMMS#5750)
* Show the scrollbar in the default theme to close the gap. (LMMS#5752)
* Reduce the gap between the effect and the scrollbar. (LMMS#5757)
* Use always the same width for the EffectRackview (InstrumentTrack and SampleTrack) to avoid gaps or cutoffs of the background.
* Widen the background in the default theme.
* Widen the embossed space in the background in the classic theme to fit the controls.

* Changes for improving the EffectRackView after reviews.

* Reduce the background for the default theme by 1 pixel.
* Reduce the background for the classic theme by 2 pixels and remove the darker line at the bottom right.
* Reduce the width of long names of the plugin also by 2 pixels.
* Put the controls 2 pixels closer to each other.
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
* Fix EffectRackView appearance (GUI).

* Elide the name of the effect when it tends to be too big. (LMMS#5752)
* Evenly space the controls (W/D, Decay Gate). (LMMS#5750)
* Show the scrollbar in the default theme to close the gap. (LMMS#5752)
* Reduce the gap between the effect and the scrollbar. (LMMS#5757)
* Use always the same width for the EffectRackview (InstrumentTrack and SampleTrack) to avoid gaps or cutoffs of the background.
* Widen the background in the default theme.
* Widen the embossed space in the background in the classic theme to fit the controls.

* Changes for improving the EffectRackView after reviews.

* Reduce the background for the default theme by 1 pixel.
* Reduce the background for the classic theme by 2 pixels and remove the darker line at the bottom right.
* Reduce the width of long names of the plugin also by 2 pixels.
* Put the controls 2 pixels closer to each other.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants