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

Extract TemplatesMenu class from MainWindow #5125

Merged

Conversation

winniehell
Copy link
Contributor

Extracts the logic for the project template menu from MainWindow into a new class.

This does not change or add functionality. It's mainly intended to reduce the size of MainWindow and allow extracting the toolbar logic in the long term.

@winniehell
Copy link
Contributor Author

@lukas-w Can I ask you to take a look at this?

src/gui/menus/TemplatesMenu.cpp Outdated Show resolved Hide resolved
src/gui/menus/TemplatesMenu.cpp Outdated Show resolved Hide resolved
@winniehell winniehell force-pushed the winniehell-extract-templates-menu branch from ae95380 to 8fb20c0 Compare August 16, 2019 21:13
@Reflexe Reflexe added needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR needs testing This pull request needs more testing labels Aug 17, 2019
@winniehell
Copy link
Contributor Author

@lukas-w Thank you for the review! I have addressed your comments. Can you please take another look?

@lukas-w
Copy link
Member

lukas-w commented Aug 18, 2019

Thanks. Please avoid force-pushing to make the review process easier. We can squash commits later when merging.

Copy link
Member

@lukas-w lukas-w left a comment

Choose a reason for hiding this comment

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

There's inconsistent indentation in TemplatesMenu.h and TemplatesMenu.cpp. Please check that you use one tab for indentation everywhere.

@winniehell
Copy link
Contributor Author

Please avoid force-pushing to make the review process easier.

Sorry, I will try to remember in the future.

Just out of curiosity—can you explain how it makes reviews harder?

There's inconsistent indentation

Thank you for catching this! 👍 It should be fixed now. I must say I can't wait for #4690.

@lukas-w Can I ask you for yet another look please? 😃

@lukas-w
Copy link
Member

lukas-w commented Aug 19, 2019

Just out of curiosity—can you explain how it makes reviews harder?

If you don't force push but add new changes as additional commits, I can easily see what changes you made since I last viewed the PR.

@lukas-w lukas-w removed needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR labels Aug 19, 2019
@winniehell
Copy link
Contributor Author

I can easily see what changes you made since I last viewed the PR.

@lukas-w Are you aware that even with force push, you can click the force-pushed link to get a diff since the last push?

Bildschirmfoto vom 2019-08-19 19-24-50

Also I see you approve already but the needs testing label is still there. Anything I can do to help testing?

include/TemplatesMenu.h Outdated Show resolved Hide resolved
@winniehell
Copy link
Contributor Author

@lukas-w Please let me know if I can do anything to get this finished 😃

@lukas-w
Copy link
Member

lukas-w commented Aug 23, 2019

Just need to find time for testing (unless someone beats me to it).

m_templatesMenu doesn't need to be a member because it's only used once
in finalize().
@lukas-w
Copy link
Member

lukas-w commented Aug 24, 2019

Made one more minor change via ebf6b92, merging. Thanks @winniehell 👍

@lukas-w lukas-w merged commit a863830 into LMMS:master Aug 24, 2019
@winniehell
Copy link
Contributor Author

Thank you @lukas-w! 🙇‍♂️

@winniehell winniehell deleted the winniehell-extract-templates-menu branch August 24, 2019 10:38
artur-twardowski pushed a commit to artur-twardowski/lmms that referenced this pull request Aug 25, 2019
@zonkmachine zonkmachine removed the needs testing This pull request needs more testing label Sep 8, 2019
artur-twardowski pushed a commit to artur-twardowski/lmms that referenced this pull request Dec 22, 2019
artur-twardowski pushed a commit to artur-twardowski/lmms that referenced this pull request May 8, 2020
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
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.

5 participants