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

#55 keyboard shortcuts #57

Merged
merged 5 commits into from Aug 4, 2021
Merged

Conversation

josflesan
Copy link
Contributor

Fixes #55

I have added the global shortcut bindings to close the application (q, esc, ctrl+w and ctrl+q) as well as the navigational shortcuts for the filter screen (pgUp, pgDown, Home, End).

However, I am not sure how the ctrl+arrow keys shortcut should behave from the issue description. Should they behave similarly to the home and end keys in the filter screen? If you could please clarify this point, I'd be happy to go in and implement that final shortcut too. I hope everything else is right and up to standard :)

@Davidy22 Davidy22 self-requested a review August 4, 2021 02:27
@Davidy22
Copy link
Owner

Davidy22 commented Aug 4, 2021

Yeah, regular CTRL+arrow key behavior, jump all the way to the end. I also wanted to see it on the main menu screen, so we'd be jumping left and right too. Github default for the auto lint for new contributors seems to have been on an approval basis, I've gone in and changed the setting so first time commits don't have to wait anymore, but now that it has it seems like there's a few lines that the lint wants a little bit more elaboration on. You can run
pre-commit install
In the project top level directory and it'll add some checks to your git setup so that it automatically alerts you before you commit if the lint would raise an issue.

@@ -28,6 +29,17 @@
)


def global_shortcuts(event):
"""Event handler for global shortcuts"""
ctrlQCode = 17
Copy link
Owner

Choose a reason for hiding this comment

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

Where possible, we probably want to import and use the named variable in the enumeration from asciimatics instead of set keycodes as rigid numbers because asciimatics might change the keycode internally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commit dbc3f42 should have fixed this. I swapped the hard-coded values for the Screen.ctrl() function

screen.set_scenes(scenes)
screen.set_scenes(scenes, unhandled_input=global_shortcuts)

if screen.current_scene == scenes[2]:
Copy link
Owner

Choose a reason for hiding this comment

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

We want code specific to a screen to be in the screen's file instead of main.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commit 7efeeba should have fixed this. Instead of writing the code directly in main.py, I overrode the process_event method for the Filter Frame class so it would handle the navigational shortcuts required.

fFrame.switch_focus(layout, 0, 0)
elif c == screen.KEY_END:
fFrame.switch_focus(layout, 0, len(fFrame.filterList)-1)
elif c == screen.KEY_PAGE_UP:
Copy link
Owner

Choose a reason for hiding this comment

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

pgup and pgdown usually move by more than one arrow keypress, would be a full page but we don't have enough filters to page that far currently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In commit 7efeeba, I created an empty function _skip_to_next_page with no implementation (pass) but which would contain this logic further down the line. Is this ok for now, or should I somehow emulate the pgup and pgdown navigation with the current set of filters?

Copy link
Owner

Choose a reason for hiding this comment

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

eh yeah sure this'll do for now, don't have enough filters to make this that relevant yet, we can make another easy ticket out of this

@Trisanu-007 Trisanu-007 added the enhancement New feature or request label Aug 4, 2021
@Trisanu-007 Trisanu-007 added this to In progress in skunkBooth via automation Aug 4, 2021
@Davidy22
Copy link
Owner

Davidy22 commented Aug 4, 2021

Gave it a spin and it works well enough, merging.

@Davidy22 Davidy22 merged commit 9fb1470 into Davidy22:main Aug 4, 2021
skunkBooth automation moved this from In progress to Done Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

Keyboard shortcuts
3 participants