The Wayback Machine - https://web.archive.org/web/20201109044144/https://github.com/fyne-io/fyne/pull/1290
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

implement RemoveShortcut #1290

Merged
merged 4 commits into from Sep 21, 2020
Merged

Conversation

@kevingentile
Copy link
Contributor

@kevingentile kevingentile commented Sep 6, 2020

Description:

Addresses #1278 for shortcut cleanup

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

  • Public APIs match existing style.
  • Any breaking changes have a deprecation path or have been discussed.
Copy link
Member

@toaster toaster left a comment

Looks good!
Just two minor typo/naming things, see suggestions.

@@ -15,6 +15,22 @@ func TestShortcutHandler_AddShortcut(t *testing.T) {
assert.Equal(t, 2, len(handle.entry))
}

func TestShortcutHandler_RemoveShortut(t *testing.T) {

This comment has been minimized.

@toaster

toaster Sep 8, 2020
Member
Suggested change
func TestShortcutHandler_RemoveShortut(t *testing.T) {
func TestShortcutHandler_RemoveShortcut(t *testing.T) {

handle := &ShortcutHandler{}
handle.AddShortcut(&ShortcutCopy{}, func(shortcut Shortcut) {})
handle.AddShortcut(&ShortcutPaste{}, func(shortcut Shortcut) {})

assert.Equal(t, 2, len(handle.entry))

handle.RemoveShortcut(&ShortcutCopy{})

assert.Equal(t, 1, len(handle.entry))

handle.RemoveShortcut(&ShortcutPaste{})

assert.Equal(t, 0, len(handle.entry))
Comment on lines 19 to 31

This comment has been minimized.

@toaster

toaster Sep 8, 2020
Member
Suggested change
handle := &ShortcutHandler{}
handle.AddShortcut(&ShortcutCopy{}, func(shortcut Shortcut) {})
handle.AddShortcut(&ShortcutPaste{}, func(shortcut Shortcut) {})
assert.Equal(t, 2, len(handle.entry))
handle.RemoveShortcut(&ShortcutCopy{})
assert.Equal(t, 1, len(handle.entry))
handle.RemoveShortcut(&ShortcutPaste{})
assert.Equal(t, 0, len(handle.entry))
handler := &ShortcutHandler{}
handler.AddShortcut(&ShortcutCopy{}, func(shortcut Shortcut) {})
handler.AddShortcut(&ShortcutPaste{}, func(shortcut Shortcut) {})
assert.Equal(t, 2, len(handler.entry))
handler.RemoveShortcut(&ShortcutCopy{})
assert.Equal(t, 1, len(handler.entry))
handler.RemoveShortcut(&ShortcutPaste{})
assert.Equal(t, 0, len(handler.entry))

This comment has been minimized.

@kevingentile

kevingentile Sep 8, 2020
Author Contributor

Note, this change is inconsistent with the existing tests in this file

@andydotxyz
Copy link
Member

@andydotxyz andydotxyz commented Sep 16, 2020

Looks good - @toaster can you please confirm if you are happy with the changes

@andydotxyz
Copy link
Member

@andydotxyz andydotxyz commented Sep 16, 2020

Looks llike the file was changed a lot so some merge conflicts to resolve @kevingentile sorry

@kevingentile kevingentile force-pushed the kevingentile:remove-shortcut branch from 9d7788d to a5ff42a Sep 16, 2020
@kevingentile kevingentile requested a review from andydotxyz Sep 16, 2020
@kevingentile kevingentile force-pushed the kevingentile:remove-shortcut branch from a5ff42a to b0dedcb Sep 16, 2020
@andydotxyz
Copy link
Member

@andydotxyz andydotxyz commented Sep 17, 2020

Static check failing:

/home/runner/work/fyne/fyne/shortcut.go
(40, 2) S1033 unnecessary guard around call to delete

@andydotxyz andydotxyz merged commit 1a163ac into fyne-io:develop Sep 21, 2020
2 checks passed
2 checks passed
checks
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kevingentile kevingentile deleted the kevingentile:remove-shortcut branch Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.