-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
refactor: GlossaryTextArea: action with lambda #1046
base: master
Are you sure you want to change the base?
refactor: GlossaryTextArea: action with lambda #1046
Conversation
Signed-off-by: Hiroshi Miura <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Hiroshi Miura <[email protected]>
Signed-off-by: Hiroshi Miura <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Hiroshi Miura <[email protected]>
Signed-off-by: Hiroshi Miura <[email protected]>
Signed-off-by: Hiroshi Miura <[email protected]>
Signed-off-by: Hiroshi Miura <[email protected]>
Signed-off-by: Hiroshi Miura <[email protected]>
Now ready for review and merge. |
Signed-off-by: Hiroshi Miura <[email protected]>
Updated with a feedback in dev-ML discussion that @t-cordonnier suggests lambda instead of old fashioned Action class |
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.
For action command I let you decide, but if you plan to re-use the methods in the future you should move them in a dedicated class right now.
@@ -451,4 +446,29 @@ public void actionPerformed(ActionEvent e) { | |||
.setPreference(Preferences.GLOSSARY_SORT_BY_LENGTH, sortOrderLocLength.isSelected())); | |||
menu.add(sortOrderLocLength); | |||
} | |||
|
|||
private static JMenuItem createMenuItem(String titleKey, String command, Runnable action) { |
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.
If you plan to use the same methodology for main menu, you should move these methods to a common class, for example org.omegat.gui.common.MenuFactory
private static JMenuItem createMenuItem(String titleKey, String command, ActionListener actionListener) { | ||
JMenuItem result = new JMenuItem(); | ||
setLocalizedText(result, OStrings.getString(titleKey)); | ||
result.setActionCommand(command); |
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 think also action command is not necessary. In the past it was used when same action listener was shared by multiple menus, which is the case in MainWindowMenu actually.
But with a distinct lambda for each menu, does not seem necessary
This refactor GlossaryTextArea class.
The proposed change improve readability of code and help debug to avoid anonymous action listener class.
Pull request type
refactor
Which ticket is resolved?
What does this PR change?
openWritableGlossaryFile
inProjectUICommand
classOther information
dev-ML discussion
https://sourceforge.net/p/omegat/mailman/omegat-development/thread/2ab347a7-536f-4e3b-bac6-a8856c00112d%40northside.tokyo/#msg58776083
Suggest that we can use Java standard Actons interface for menu items