-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix duplicate shortcut key configuration items in Keyboard shortcuts (fix #4387) #4453
base: main
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.
clang-tidy made some suggestions
@@ -586,6 +586,21 @@ class KeyboardShortcutsWindow : public app::gen::KeyboardShortcuts { | |||
deleteList(dragActions()); | |||
} | |||
|
|||
void removeRepeatedItems(ListBox* listBox) { |
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.
warning: method 'removeRepeatedItems' can be made static [readability-convert-member-functions-to-static]
void removeRepeatedItems(ListBox* listBox) { | |
static void removeRepeatedItems(ListBox* listBox) { |
@@ -586,6 +586,21 @@ | |||
deleteList(dragActions()); | |||
} | |||
|
|||
void removeRepeatedItems(ListBox* listBox) { |
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.
warning: pointer parameter 'listBox' can be pointer to const [readability-non-const-parameter]
void removeRepeatedItems(ListBox* listBox) { | |
void removeRepeatedItems(const ListBox* listBox) { |
fb29555
to
409ee21
Compare
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.
clang-tidy made some suggestions
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 haven't tested this, but seems that it should work because it removes duplicates. I'm just concerned about the underlaying cause of the duplicates. Maybe we could try to avoid duplicates in the first place?
Most duplicates appear because some commands have "params" and their "friendlyString" does not concatenate those specific parameters.
But in the command list box is showed:
Other duplicates I still don't know why they are duplicated (one of them is TiledMode::NONE which appears twice) So, I'll concatenate the param string to the base command. The following commands has duplicates:
|
409ee21
to
0a79626
Compare
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.
clang-tidy made some suggestions
@@ -586,6 +586,25 @@ class KeyboardShortcutsWindow : public app::gen::KeyboardShortcuts { | |||
deleteList(dragActions()); | |||
} | |||
|
|||
bool skipCommand(KeyItem* keyItem) { |
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.
warning: method 'skipCommand' can be made static [readability-convert-member-functions-to-static]
bool skipCommand(KeyItem* keyItem) { | |
static bool skipCommand(KeyItem* keyItem) { |
keyItem->text() == Strings::commands_SavePalette() || | ||
(keyItem->text() == toIndexed && keyItem->key()->params().size() > 1) || | ||
(keyItem->text() == Strings::commands_RunScript() && keyItem->key()->params().empty())) | ||
return true; |
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.
warning: redundant boolean literal in conditional return statement [readability-simplify-boolean-expr]
src/app/commands/cmd_keyboard_shortcuts.cpp:593:
- if ((keyItem->text() == Strings::commands_OpenFile() ||
- keyItem->text() == Strings::commands_Flip_Horizontally() ||
- keyItem->text() == Strings::commands_ExportSpriteSheet() ||
- keyItem->text() == Strings::commands_ExportSpriteSheet()) && !keyItem->key()->params().empty() ||
- keyItem->text() == Strings::commands_Launch() ||
- keyItem->text() == Strings::commands_OpenBrowser() ||
- keyItem->text() == Strings::commands_SavePalette() ||
- (keyItem->text() == toIndexed && keyItem->key()->params().size() > 1) ||
- (keyItem->text() == Strings::commands_RunScript() && keyItem->key()->params().empty()))
- return true;
- else
- return false;
+ return (keyItem->text() == Strings::commands_OpenFile() ||
+ keyItem->text() == Strings::commands_Flip_Horizontally() ||
+ keyItem->text() == Strings::commands_ExportSpriteSheet() ||
+ keyItem->text() == Strings::commands_ExportSpriteSheet()) && !keyItem->key()->params().empty() ||
+ keyItem->text() == Strings::commands_Launch() ||
+ keyItem->text() == Strings::commands_OpenBrowser() ||
+ keyItem->text() == Strings::commands_SavePalette() ||
+ (keyItem->text() == toIndexed && keyItem->key()->params().size() > 1) ||
+ (keyItem->text() == Strings::commands_RunScript() && keyItem->key()->params().empty());
(keyItem->text() == toIndexed && keyItem->key()->params().size() > 1) || | ||
(keyItem->text() == Strings::commands_RunScript() && keyItem->key()->params().empty())) | ||
return true; | ||
else |
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.
warning: do not use 'else' after 'return' [readability-else-after-return]
else
^
this fix will not be applied because it overlaps with another fix
@@ -64,6 +68,17 @@ void TiledModeCommand::onExecute(Context* ctx) | |||
Preferences::instance().document(doc).tiled.mode(m_mode); | |||
} | |||
|
|||
std::string TiledModeCommand::onGetFriendlyName() const { | |||
std::string mode; |
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.
warning: variable 'mode' of type 'std::string' (aka 'basic_string') can be declared 'const' [misc-const-correctness]
std::string mode; | |
std::string const mode; |
std::string TiledModeCommand::onGetFriendlyName() const { | ||
std::string mode; | ||
switch (m_mode) { | ||
case filters::TiledMode::NONE: mode = Strings::main_menu_view_tiled_mode_none(); break; |
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.
warning: switch has 4 consecutive identical branches [bugprone-branch-clone]
case filters::TiledMode::NONE: mode = Strings::main_menu_view_tiled_mode_none(); break;
^
Additional context
src/app/commands/cmd_tiled_mode.cpp:76: last of these clones ends here
case filters::TiledMode::Y_AXIS: mode = Strings::main_menu_view_tiled_mode_y(); break;
^
@martincapello What do you think about this approach?
Another possible approach: |
…Window (fix aseprite#4387) Introduced Key::isSkipListing() and Command::isSkipListing() to skip keyItem creation on Keyboard shortcut List Box. Removed commands: 'Launch' 'OpenBrowser' And removed unnecessary commands: 'Change Color Mode: Indexed' 'Contract Selection' 'Export Sprite Sheet' 'Flip Canvas Horizontally' 'Frame Properties' 'Load Palette' 'Open Sprite' 'Playback Speed 1x' 'Run Script' 'Save Palette' 'Select Used Colors' 'Set Palette Entry Size' 'Tileset Mode: Auto'
Before this fix, the following items were repeated in the list box: 'Zoom 600%' 'Zoom 800%' 'Zoom 1600%'
c917d68
to
4276729
Compare
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.
clang-tidy made some suggestions
@@ -64,6 +64,9 @@ class ExportSpriteSheetCommand : public CommandWithNewParams<ExportSpriteSheetPa | |||
protected: | |||
bool onEnabled(Context* context) override; | |||
void onExecute(Context* context) override; | |||
const bool isSkipListing(const Params& params) const override { |
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.
warning: return type 'const bool' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type]
const bool isSkipListing(const Params& params) const override { | |
bool isSkipListing(const Params& params) const override { |
@@ -64,6 +64,9 @@ | |||
protected: | |||
bool onEnabled(Context* context) override; | |||
void onExecute(Context* context) override; | |||
const bool isSkipListing(const Params& params) const override { |
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.
warning: method 'isSkipListing' can be made static [readability-convert-member-functions-to-static]
const bool isSkipListing(const Params& params) const override { | |
static const bool isSkipListing(const Params& params) override { |
@@ -20,6 +20,7 @@ namespace app { | |||
class LaunchCommand : public Command { | |||
public: | |||
LaunchCommand(); | |||
const bool isSkipListing(const Params& params) const override { return true; } |
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.
warning: parameter 'params' is unused [misc-unused-parameters]
const bool isSkipListing(const Params& params) const override { return true; } | |
const bool isSkipListing(const Params& /*params*/) const override { return true; } |
@@ -46,6 +46,9 @@ class ModifySelectionCommand : public Command { | |||
bool onEnabled(Context* context) override; | |||
void onExecute(Context* context) override; | |||
std::string onGetFriendlyName() const override; | |||
const bool isSkipListing(const Params& params) const override { |
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.
warning: parameter 'params' is unused [misc-unused-parameters]
const bool isSkipListing(const Params& params) const override { | |
const bool isSkipListing(const Params& /*params*/) const override { |
@@ -23,6 +24,7 @@ class OpenBrowserCommand : public Command { | |||
protected: | |||
void onLoadParams(const Params& params) override; | |||
void onExecute(Context* context) override; | |||
const bool isSkipListing(const Params& params) const override { return true; } |
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.
warning: parameter 'params' is unused [misc-unused-parameters]
const bool isSkipListing(const Params& params) const override { return true; } | |
const bool isSkipListing(const Params& /*params*/) const override { return true; } |
std::string TiledModeCommand::onGetFriendlyName() const { | ||
std::string mode; | ||
switch (m_mode) { | ||
case filters::TiledMode::NONE: mode = Strings::main_menu_view_tiled_mode_none(); break; |
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.
warning: switch has 4 consecutive identical branches [bugprone-branch-clone]
case filters::TiledMode::NONE: mode = Strings::main_menu_view_tiled_mode_none(); break;
^
Additional context
src/app/commands/cmd_tiled_mode.cpp:79: last of these clones ends here
case filters::TiledMode::Y_AXIS: mode = Strings::main_menu_view_tiled_mode_y(); break;
^
// Not all Commands must be listed on KeyBoard Shortcut list, so | ||
// this function returns if a key command should be listed or not. | ||
// Used on 'cmd_keyboard_shorcuts.cpp'. | ||
virtual const bool isSkipListing(const Params& params) const { |
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.
warning: return type 'const bool' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type]
virtual const bool isSkipListing(const Params& params) const {
^
// Not all Commands must be listed on KeyBoard Shortcut list, so | ||
// this function returns if a key command should be listed or not. | ||
// Used on 'cmd_keyboard_shorcuts.cpp'. | ||
virtual const bool isSkipListing(const Params& params) const { |
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.
warning: parameter 'params' is unused [misc-unused-parameters]
virtual const bool isSkipListing(const Params& params) const { | |
virtual const bool isSkipListing(const Params& /*params*/) const { |
@@ -26,6 +26,9 @@ class SetPlaybackSpeedCommand : public CommandWithNewParams<SetPlaybackSpeedPara | |||
bool onChecked(Context* ctx) override; | |||
void onExecute(Context* ctx) override; | |||
std::string onGetFriendlyName() const override; | |||
const bool isSkipListing(const Params& params) const override { |
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.
warning: return type 'const bool' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type]
const bool isSkipListing(const Params& params) const override { | |
bool isSkipListing(const Params& params) const override { |
@@ -26,6 +26,9 @@ | |||
bool onChecked(Context* ctx) override; | |||
void onExecute(Context* ctx) override; | |||
std::string onGetFriendlyName() const override; | |||
const bool isSkipListing(const Params& params) const override { |
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.
warning: method 'isSkipListing' can be made static [readability-convert-member-functions-to-static]
const bool isSkipListing(const Params& params) const override { | |
static const bool isSkipListing(const Params& params) override { |
New approach to review. Pending: To go ahead I need to know if this approach is OK. |
I'll review this later. |
Ok, I will just push-force the last movement of the removeEscapeCharFromText() function from 'laf/base/string' to 'ui/intern' |
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.
clang-tidy made some suggestions
std::string TiledModeCommand::onGetFriendlyName() const { | ||
std::string mode; | ||
switch (m_mode) { | ||
case filters::TiledMode::NONE: mode = Strings::main_menu_view_tiled_mode_none(); break; |
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.
warning: switch has 4 consecutive identical branches [bugprone-branch-clone]
case filters::TiledMode::NONE: mode = Strings::main_menu_view_tiled_mode_none(); break;
^
Additional context
src/app/commands/cmd_tiled_mode.cpp:80: last of these clones ends here
case filters::TiledMode::Y_AXIS: mode = Strings::main_menu_view_tiled_mode_y(); break;
^
while (int chr = decode.next()) { | ||
if (chr == escapeChar) { | ||
chr = decode.next(); | ||
if (!chr) |
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.
warning: implicit conversion 'int' -> bool [readability-implicit-bool-conversion]
if (!chr) | |
if (chr == 0) |
@@ -34,6 +35,9 @@ namespace ui { | |||
void reinitThemeForAllWidgets(); | |||
int old_guiscale(); | |||
|
|||
std::string removeEscapeCharFromText(const std::string& original, |
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.
warning: use of undeclared identifier 'std' [clang-diagnostic-error]
std::string removeEscapeCharFromText(const std::string& original,
^
@@ -34,6 +35,9 @@ | |||
void reinitThemeForAllWidgets(); | |||
int old_guiscale(); | |||
|
|||
std::string removeEscapeCharFromText(const std::string& original, |
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.
warning: use of undeclared identifier 'std' [clang-diagnostic-error]
std::string removeEscapeCharFromText(const std::string& original,
^
fix #4387