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

[Testing] HyperTextView #692

Closed
wants to merge 13 commits into from
Closed

[Testing] HyperTextView #692

wants to merge 13 commits into from

Conversation

marbetschar
Copy link
Member

HyperTextView fixes #571 and superseds #656.

As @tintou pointed out in elementary/granite#507 (comment) this PR is to evaluate the current implementation before having it merged into Granite.

My tests showed that the key_press_event and key_release_event events are not fired - @mcclurgm any idea why?

This also includes the change
from TOOLTIP_SECONDARY_TEXT_MARKUP
to Granite.TOOLTIP_SECONDARY_TEXT_MARKUP
@mcclurgm
Copy link
Collaborator

I don't know much about Gtk, but in Calendar toplevel_window is the main calendar window, not the editor. It seems plausible to me that the event could be sent to the editor dialog and not the calendar toplevel. Since the callback is based on the toplevel, then the callback isn't triggered. Again, this is all a guess though.

@marbetschar
Copy link
Member Author

Sounds like a great hint - thanks! Will do some testing during the weekend.

@marbetschar
Copy link
Member Author

marbetschar commented Aug 21, 2021

@mcclurgm you were right! Thanks again for the pointer. The code now simply binds to all toplevel windows - which now enables us to grab Control presses, even if the text view is not focused. So I guess we are ready for review now.

I will update the original PR in the Granite repo (elementary/granite#507) with the changes introduced here once this review is passed.

@marbetschar marbetschar marked this pull request as ready for review August 21, 2021 07:15
@marbetschar marbetschar marked this pull request as draft August 26, 2021 06:51
@marbetschar marbetschar changed the title HyperTextView [Testing] HyperTextView Aug 26, 2021
@mcclurgm
Copy link
Collaborator

Mostly seems good to me. One small issue I noticed is that when you ctrl+click on a link with another window focused, it won't work until you release and re-press the ctrl key because it doesn't receive key press event. I think that's a very minor issue though, and maybe even desirable that we don't let users click a link when switching windows.

@mcclurgm
Copy link
Collaborator

Similarly, one more limitation I found is that when you press ctrl and the cursor isn't hovering over the link, it won't ever change to a link cursor until you re-press ctrl while hovering over the link. Again small, but one more limitation.

Overall, it seems to work well for me. It's fairly performant (on my decently high-end machine from 2018, to be fair) and detects the links I expect without over-reaching. It does accept the ctrl+click input even when other widgets are focused, which is what I would expect to happen. I don't have a lot of insight on code style, but it seems like you worked out some of that in the Granite PR.

@marbetschar
Copy link
Member Author

Granite.HyperTextView was just released, so we are done with testing here.

@marbetschar marbetschar closed this Dec 1, 2021
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.

Links in comments aren't clickable
2 participants