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

Add open link under terminal cursor #2062

Merged
merged 8 commits into from
Apr 7, 2022
Merged

Conversation

thibthib18
Copy link
Contributor

@thibthib18 thibthib18 commented Mar 30, 2022

Add functionality to set a keyboard shortcut to open link under terminal cursor (similar to "ctrl click").

Following 2060

Status: current changes are enough to open links under cursor via a hardcoded shortcut.

Question: how to properly add a new user configurable shortcut?

@Davidy22
Copy link
Collaborator

Davidy22 commented Apr 1, 2022

You can add it to keybindings.py, stick it in the keys dict and test to see if the keybinding works and I'll check it after

@thibthib18
Copy link
Contributor Author

I added the bindings, after reloading the schemas this seems to work fine. I'll let you have a look.

Copy link
Collaborator

@Davidy22 Davidy22 left a comment

Choose a reason for hiding this comment

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

Works for me and changes look mostly fine. Forgot to ask for a release note file, just do a make reno SLUG=[descriptive_label] and fill in the release note file like how it is in other PRs. Also a thing about the type hint

guake/keybindings.py Show resolved Hide resolved
@@ -478,15 +478,23 @@ def handleTerminalMatch(self, matched_string):
if value:
return value

def get_link_under_terminal_cursor(self) -> Optional[str]:
Copy link
Collaborator

@Davidy22 Davidy22 Apr 6, 2022

Choose a reason for hiding this comment

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

Type hints would be a nice thing to do but probably should be consistent with the rest of the codebase and everything else isn't hinted at the current moment. Moving files onto hints could be a good bigger project though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this would be very helpful! I actually added a lot more locally to navigate and understand the code more easily. I left it out as this wasn't in the scope of this PR.

For this part specifically, I saw that Tuple and Optional were already used line 275 (though now a quick look shows this might have been the only case of typing in this codebase).

In my opinion every bit of typing help, whether in making code more understandable, or improving type errors and catching early type errors, with pretty much no down side. The fact that it can be adopted incrementally is also something to take advantage of, as larger nice-to-have projects tend to always be pushed back.

Removed it anyway, just food for thought for later PRs.

Copy link
Contributor Author

@thibthib18 thibthib18 Apr 7, 2022

Choose a reason for hiding this comment

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

Actually I wouldn't mind opening a PR to start adding some more type hints to the codebase. Surely not everything, but a good chunk of the low hanging fruits just to get things started. If that's something you'd be interested in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that'd be nice, can take that if you're willing to do it. Just need the release note file generated by make reno SLUG=[descriptive_label] filled in and I'll merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Release notes are now there.

Noted for the typing PR :D

@thibthib18 thibthib18 changed the title WIP: Add open link under terminal cursor Add open link under terminal cursor Apr 7, 2022
@Davidy22 Davidy22 merged commit 38dfd0e into Guake:master Apr 7, 2022
@Davidy22
Copy link
Collaborator

Davidy22 commented Apr 7, 2022

Alright looks fine, merging

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.

None yet

2 participants