Skip to content

Fix track operations button alignment, size, and style - #7779

Merged
rubiefawn merged 16 commits into
LMMS:masterfrom
rubiefawn:fix/track-ops-btn
Mar 26, 2025
Merged

Fix track operations button alignment, size, and style#7779
rubiefawn merged 16 commits into
LMMS:masterfrom
rubiefawn:fix/track-ops-btn

Conversation

@rubiefawn

@rubiefawn rubiefawn commented Mar 14, 2025

Copy link
Copy Markdown
Contributor

This changes the gear icon for tracks from a .png to a .svg, and makes it consistently sized with the new mute and solo buttons. It also makes the backgrounds for all three buttons purely CSS.

Todo:

  • Delete unused assets
  • Minify SVG resources

image

@rubiefawn rubiefawn added needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing labels Mar 21, 2025
Comment thread include/InstrumentTrackWindow.h Outdated
Comment thread data/themes/default/style.css Outdated
@tresf

tresf commented Mar 22, 2025

Copy link
Copy Markdown
Member

I'm surprised to see that this PR was chosen to cleanup the stylesheets. It's well overdue, but makes reviewing this PR a bit harder. For example, a choice was made to place each item on it's own line... Another choice was made to remove in-line comments and place them after lines that they were describing.

This becomes almost comical when asked to follow our defacto rules is answered with this:

I'll happily conform to our coding conventions, if/when this is officially added to them.

Well, please link the CSS rules in the style guidelines and we'll get the pointer logic added. Edit: pointer logic added. 😄

@rubiefawn

rubiefawn commented Mar 22, 2025

Copy link
Copy Markdown
Contributor Author

The CSS formatting was completely unintentional, vscode-css-language-server has format-on-save enabled by default. I need to change that in my config right away, it's tripled the size of the diff and it's not even accurate to our coding conventions as you pointed out.

Edit: I disabled format on save, reset the CSS files to the state of master, then re-added my new rules, but I'm still seeing differences with things I didn't intend to modify, specifically all the gradients. There is tomfoolery yet to be remedied The tomfoolery has now been remedied

@tresf

tresf commented Mar 22, 2025

Copy link
Copy Markdown
Member

Thanks I do think it's in need of a cleanup though. Happy to discuss in another PR if interested.

@rubiefawn

Copy link
Copy Markdown
Contributor Author

Thanks I do think it's in need of a cleanup though. Happy to discuss in another PR if interested.

I can take a look at it later! (Without the "help" of the autoformatter, of course)

@tresf

tresf commented Mar 22, 2025

Copy link
Copy Markdown
Member

I had voiced strong opposition on Discord to changing the gear from a flat icon to a button. I made a promise that I would shut up and NOT stop progress if everyone agreed on this aesthetic. 😆.

With that said, I'll keep that promise and review it as-is.

I tested this PR and it works well.

-	PixmapButton *m_soloBtn;
+	AutomatableButton* m_soloBtn;

Out of morbid curiosity, I tried to automate these, but the UI didn't allow this. I assume this is deliberate, but it does bring to question... was this change done solely for stylistic reasons? I notice they light-up on hover now which is fantastic.

From a testing perspective, I'm approving this. I saw that @messmerd started a code review, but if we could get his sign-off that would be greatly appreciated.

@rubiefawn

rubiefawn commented Mar 22, 2025

Copy link
Copy Markdown
Contributor Author

Well, they are AutomatableButtons, so if they aren’t automatable, something is wrong. If I intended to change the behavior I would have just made them plain QPushButtons.

Edit: I am not able to reproduce this, it allows me to connect them to automation clips just fine.

Recording 2025-03-22 at 09 54 17

@tresf

tresf commented Mar 24, 2025

Copy link
Copy Markdown
Member

Well, they are AutomatableButtons, so if they aren’t automatable, something is wrong. If I intended to change the behavior I would have just made them plain QPushButtons.

Sorry, I forgot that it was already automatable based on class inheritance, I thought you were ADDING support, not REMOVING support but I made this observation based on the class-name alone, sorry for that!

Edit: I am not able to reproduce this, it allows me to connect them to automation clips just fine.

Well, I think we found another macOS-specific issue with the buttons reacting on click instead of release because I can't find a way to connect them unless I use the context-menu to add them to a controller. I believe the primary click on Mac is taking precedence over the Option + Drag.

This regression seems to be caused by #7325. Of course, before #7325, dragging was nearly unusable, but this means we now can't drag any pushbuttons on mac. I'll file a separate issue for this.

Edit: Bug filed:

@tresf

tresf commented Mar 25, 2025

Copy link
Copy Markdown
Member

@rubiefawn this should be good to merge once you fix the build failures.

/home/runner/work/lmms/lmms/src/gui/SampleTrackWindow.cpp:103:9: error: ‘m_muteBtn’ was not declared in this scope

@messmerd

Copy link
Copy Markdown
Member

It's buggy when trying to automate two different solo buttons to "1" at the same time. Maybe we should hold off on making the solo buttons automatable?

@rubiefawn

Copy link
Copy Markdown
Contributor Author

The solo buttons were already automatable (you can test this in 1.2 stable), they were previously PixmapButtons which inherit from AutomatableButton.

@messmerd messmerd left a comment

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 don't know enough about CSS to comment on the CSS changes, but the C++ changes look fine to me.

I didn't notice any issues while testing it, and projects with automated buttons saved in previous versions of LMMS continue to work with this PR.

@rubiefawn
rubiefawn merged commit 8afe95a into LMMS:master Mar 26, 2025
@rubiefawn
rubiefawn deleted the fix/track-ops-btn branch March 26, 2025 01:02
sakertooth pushed a commit to sakertooth/lmms that referenced this pull request Jun 1, 2025
Make the track ops button consistently sized and style it and the solo & mute buttons using CSS instead of SVG assets
@rubiefawn rubiefawn removed needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing labels Oct 24, 2025
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.

3 participants