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

Shortcut improvements #3584

Closed

Conversation

cmeriaux
Copy link
Contributor

@cmeriaux cmeriaux commented Aug 2, 2017

Fixes #3583

Shorcut mapper - main panel : new colums that show the category of the shortcut
Shorcut mapper - plugin panel : new colums that show the plugin name that the shortcut belongs to
Shorcut mapper - scintilla panel : it shows every shortcuts configured for one command

Shorcut mapper - main panel : new colums that show the category of the shortcut
Shorcut mapper - plugin panel : new colums that show the plugin name that the shortcut belongs to
Shorcut mapper - scintilla panel : it shows every shortcuts configured for one command
@@ -193,12 +194,42 @@ protected :

class CommandShortcut : public Shortcut {
public:
CommandShortcut(Shortcut sc, long id) : Shortcut(sc), _id(id) {};
CommandShortcut(Shortcut sc, long id) : Shortcut(sc), _id(id) {
long category_id = _id - _id % 500;
Copy link
Member

Choose a reason for hiding this comment

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

why 500? Magic number should be avoided.

default :
_category = TEXT(""); break;
}
};
unsigned long getID() const {return _id;};
Copy link
Member

Choose a reason for hiding this comment

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

Function long definition should be in cpp file.

Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

Remove magic number and move constructor in cpp file.

@donho donho self-assigned this Aug 10, 2017
@donho
Copy link
Member

donho commented Aug 10, 2017

@cmeriaux Nice PR.
No need to redo the PR, please just add new commit for my change request in this PR.

move long constructor from .hpp to .cpp
remove magic number
@cmeriaux
Copy link
Contributor Author

@donho I've done your change request.

I noticed that some constants are not declared as expected.

Wrong code:

    #define    IDM_EDIT_AUTOCOMPLETE                (50000 + 0)
    #define    IDM_EDIT_AUTOCOMPLETE_CURRENTFILE    (50000 + 1)
    #define    IDM_EDIT_FUNCCALLTIP                 (50000 + 2)
    #define    IDM_EDIT_AUTOCOMPLETE_PATH           (50000 + 6)

    #define    IDM_VIEW_GOTO_ANOTHER_VIEW        10001
    #define    IDM_VIEW_CLONE_TO_ANOTHER_VIEW    10002
    #define    IDM_VIEW_GOTO_NEW_INSTANCE        10003
    #define    IDM_VIEW_LOAD_IN_NEW_INSTANCE     10004

expected code:

    #define    IDM_EDIT_AUTOCOMPLETE                (IDM_EDIT + 100)
    #define    IDM_EDIT_AUTOCOMPLETE_CURRENTFILE    (IDM_EDIT + 101)
    #define    IDM_EDIT_FUNCCALLTIP                 (IDM_EDIT + 102)
    #define    IDM_EDIT_AUTOCOMPLETE_PATH           (IDM_EDIT + 103)

    #define    IDM_VIEW_GOTO_ANOTHER_VIEW        (IDM_VIEW + 200)
    #define    IDM_VIEW_CLONE_TO_ANOTHER_VIEW    (IDM_VIEW + 201)
    #define    IDM_VIEW_GOTO_NEW_INSTANCE        (IDM_VIEW + 202)
    #define    IDM_VIEW_LOAD_IN_NEW_INSTANCE     (IDM_VIEW + 203)

I didn't change anything to avoid regression. Do I fix it ?

_category = TEXT("Setting");
else if ( _id < IDM_EXECUTE)
_category = TEXT("Tool");
else if ( _id < IDM_EXECUTE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking for IDM_EXECUTE twice?

@donho
Copy link
Member

donho commented Aug 20, 2017

@cmeriaux

I didn't change anything to avoid regression. Do I fix it ?

No, please leave it as it is. They are being used for the translation.

@donho
Copy link
Member

donho commented Aug 20, 2017

@cmeriaux please remove duplicated code.

Copy link
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

Remove duplicated code.

@cmeriaux
Copy link
Contributor Author

@donho @MAPJe71 code review done. Sorry for the typo.

@donho
Copy link
Member

donho commented Aug 20, 2017

@cmeriaux It seems you add your new commits on the old branch which has been merged since long time. It's bad because there'll be the merge problem on my side.
Please create a new branch for this PR.
And keep in mind that for each PR, a new branch should be created to avoid the merge problem.

@donho donho closed this Aug 20, 2017
@donho donho added the reject label Aug 20, 2017
@cmeriaux
Copy link
Contributor Author

@donho done in #3635

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants