-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
MainWindow: Use a map instead of an array for Tools menu #6719
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few formatting changes and readability enhancements.
formatting Co-authored-by: saker <sakertooth@Gmail.com>
formatting Co-authored-by: saker <sakertooth@Gmail.com>
formatting Co-authored-by: saker <sakertooth@Gmail.com>
formatting Co-authored-by: saker <sakertooth@Gmail.com>
formatting Co-authored-by: saker <sakertooth@Gmail.com>
formatting Co-authored-by: saker <sakertooth@Gmail.com>
explicit constness of read-only local variable Co-authored-by: saker <sakertooth@Gmail.com>
use explicit window flag name Co-authored-by: saker <sakertooth@Gmail.com>
formatting Co-authored-by: saker <sakertooth@Gmail.com>
How about deleting the underscore in function parameters? At least in the .h file, where the variable is only used once. Tested OK in windows 10/64b. |
Just checked against the coding conventions, and yes, that should be done where applicable as well. |
I'd prefer doing that kind of stuff in a separate PR, not only removing leading underscores but also changing parameter names to something more expressive than the 1-2 letter names we currently have. |
It wouldn't hurt to change it here, but I wouldn't consider it blocking by any means. |
menuBar()->addMenu( m_toolsMenu )->setText( tr( "&Tools" ) ); | ||
connect( m_toolsMenu, SIGNAL(triggered(QAction*)), | ||
this, SLOT(showTool(QAction*))); | ||
const auto objectName = QString("ToolsMenuAction") + desc->displayName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why you use displayName
while there is also name
. Plugins are instantiated using name
, so I guess name
is more natural here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@irrenhaus3 Friendly reminder that this has an open comment.
Old behavior: When clicking the n-th item in the Tools menu, MainWindow looks up the n-th entry in the m_tools array and instantiates the PluginView stored there.
New behavior: When clicking an item in the Tools menu, MainWindow retrieves the object name of the QAction belonging to that item, and looks up that object name in the m_tools map and instantiates the PluginView stored under that name.
The object name is constructed from the plugin's display name when populating the Tools menu.
This makes the menu more maintainable and allows for things like submenus and non-plugin tools.
Additional changes:
QAction*
toQString const&
.