search

elementary / calendar

Desktop calendar app designed for elementary OS
https://elementary.io
GNU General Public License v3.0
130 stars 39 forks source link

[Testing] HyperTextView #692

Closed marbetschar closed 2 years ago

marbetschar commented 3 years ago

HyperTextView fixes https://github.com/elementary/calendar/issues/571 and superseds https://github.com/elementary/calendar/pull/656.

As @tintou pointed out in https://github.com/elementary/granite/pull/507#issuecomment-900266319 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?

mcclurgm commented 3 years ago

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 commented 3 years ago

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

marbetschar commented 3 years ago

@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 (https://github.com/elementary/granite/pull/507) with the changes introduced here once this review is passed.

mcclurgm commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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