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

MainWindow: Use a map instead of an array for Tools menu #6719

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

irrenhaus3
Copy link
Contributor

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:

  • Rename showTool() to showPluginTool() and change the parameter from QAction* to QString const&.
  • Format header file w.r.t. pointer/reference notation attaching to the type instead of the variable name.

Copy link
Contributor

@sakertooth sakertooth left a 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.

include/MainWindow.h Outdated Show resolved Hide resolved
include/MainWindow.h Outdated Show resolved Hide resolved
include/MainWindow.h Outdated Show resolved Hide resolved
include/MainWindow.h Outdated Show resolved Hide resolved
include/MainWindow.h Outdated Show resolved Hide resolved
src/gui/MainWindow.cpp Outdated Show resolved Hide resolved
src/gui/MainWindow.cpp Outdated Show resolved Hide resolved
src/gui/MainWindow.cpp Show resolved Hide resolved
src/gui/MainWindow.cpp Outdated Show resolved Hide resolved
include/MainWindow.h Outdated Show resolved Hide resolved
irrenhaus3 and others added 10 commits May 30, 2023 13:12
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>
@superpaik
Copy link
Contributor

Just a few formatting changes and readability enhancements.

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.

@sakertooth
Copy link
Contributor

Just a few formatting changes and readability enhancements.

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.

@irrenhaus3
Copy link
Contributor Author

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.

@sakertooth
Copy link
Contributor

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;
Copy link
Member

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.

Copy link
Contributor

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.

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.

None yet

5 participants