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

#11108 Disabled hotkeys in scenarios where their usage was not intended #11109

Conversation

IlijaQ
Copy link
Contributor

@IlijaQ IlijaQ commented Jul 19, 2023

Fixes #11108

Proposed changes

  • Creating a new branch using hotkeys Ctrl + B does not prompt FromCreateBranch while in bare repository, neither when artificial commit is selected.
  • While selecting multiple commits, hotkey for creating new branch is disabled
  • Rebasing current branch using hotkeys Ctrl + Shift + E is disabled while artificial commit is selected
  • Creating a new tag using hotkeys Ctrl + T is disabled while artificial commit is selected

Test methodology

  • Manual with GE repo

Test environment(s)

  • Git Extensions 33.33.33
  • Build 8b5eab5
  • Git 2.30.0.windows.2 (recommended: 2.40.1 or later)
  • Microsoft Windows NT 10.0.19045.0
  • .NET 6.0.14
  • DPI 120dpi (125% scaling)

Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

Most similar issues should be handled by the check in the method returns false when conditions are not fulfilled (the action could be handled by another form then). This should be done here too.
I will not oppose a merge though, if another maintainer approves.

I am sure there are several other similar issues still even if several similar issues exists...

@IlijaQ
Copy link
Contributor Author

IlijaQ commented Jul 20, 2023

Yes, I have tried that. When the method returns false, the hotkey is passed to the context menu. Every time we open the context menu for the selected commit, it checks if create branch action or some other action should be enabled, but that is to late.

The problem is when we select a regular commit, right click, and then select an artificial commit - without right clicking on the artificial commit, and press the hotkey, we can still cause this bug.

If you are still in favor of passing the action handling to another form, we may want to add a double redundant validation in context menu click event handeling or change the context menu to perform update of Enabled propery of all context menu items every time a new commit is selected.

@gerhardol
Copy link
Member

I just want to move the logic ti the method handling the request, not in ExecuteCommand().
This is how all other hot keys are handled.

@IlijaQ
Copy link
Contributor Author

IlijaQ commented Jul 21, 2023

Ok,
Just in case, please specify the method where we want to move the validations to. Do we want to move the validations to the corresponding method in the switch statement? e.g. in case Command.CreateBranch: should we move the validation to StartCreateBranchDialog?

@ghost ghost assigned IlijaQ Jul 24, 2023
@IlijaQ
Copy link
Contributor Author

IlijaQ commented Jul 24, 2023

@gerhardol I have updated the code for your review, according to your suggestions.

@IlijaQ IlijaQ force-pushed the bugfix/11108_disabled_menu_items_still_accessible_through_hotkeys branch from 26a0308 to b27346c Compare July 25, 2023 00:39
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

Approved with two suggested changes.
Changing from draft.

@@ -440,6 +440,11 @@ public bool StartCreateBranchDialog(IWin32Window? owner, string? branch)

public bool StartCreateBranchDialog(IWin32Window? owner = null, ObjectId? objectId = null, string? newBranchNamePrefix = null)
{
if (Module.IsBareRepository() || (objectId is not null && objectId.IsArtificial))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (Module.IsBareRepository() || (objectId is not null && objectId.IsArtificial))
if (Module.IsBareRepository() || objectId?.IsArtificial is true)

GitUI/GitUICommands.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/FormBrowse.cs Show resolved Hide resolved
@gerhardol gerhardol marked this pull request as ready for review July 25, 2023 19:44
@IlijaQ IlijaQ force-pushed the bugfix/11108_disabled_menu_items_still_accessible_through_hotkeys branch from b27346c to 28daf37 Compare July 26, 2023 15:47
@IlijaQ
Copy link
Contributor Author

IlijaQ commented Jul 26, 2023

Hi, @gerhardol
I have implemented the changes that you suggested and rebased my branch to the latest develop. Can you reapprove this pull request?

@IlijaQ IlijaQ force-pushed the bugfix/11108_disabled_menu_items_still_accessible_through_hotkeys branch from 28daf37 to d81a054 Compare July 26, 2023 17:50
@IlijaQ
Copy link
Contributor Author

IlijaQ commented Jul 26, 2023

Sorry, my bad. I missed the question marks in my last commit. It should pass the build now.
It needs one more approval.
I apologize.

@IlijaQ IlijaQ force-pushed the bugfix/11108_disabled_menu_items_still_accessible_through_hotkeys branch from d81a054 to e3d5b0a Compare July 31, 2023 22:29
@RussKie
Copy link
Member

RussKie commented Aug 1, 2023

Thank you!

@RussKie RussKie merged commit c940ea1 into gitextensions:master Aug 1, 2023
3 checks passed
@ghost ghost added this to the vNext milestone Aug 1, 2023
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.

Disabled menu items are still accessible through hotkeys
3 participants