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 keydown actions to resize the HelpBrowser #4932

Merged

Conversation

davidgranstrom
Copy link
Contributor

@davidgranstrom davidgranstrom commented May 16, 2020

Purpose and Motivation

There is no way to resize the (WebView based) HelpBrowser dynamically. This PR adds key down actions to be able to resize the content similar to the way the built-in HelpBrowsers docklet works in the IDE. It also fixes the key down action (Cmd-f on macOS) to toggle the "Find text" utility.

Types of changes

  • Bug fix
  • New feature

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review

@mossheim
Copy link
Contributor

thanks! haven't had time to test this on macOS, but isn't this a little inconsistent between IDE and sclang help browsers? IIRC macOS IDE help browser uses only cmd+_ shortcuts, but this will cause the sclang help browser to accept both cmd+_ and ctrl+_. i don't know if isCmd can be true on windows machines too, but just want to make sure that's been considered. i already checked myself that pressing my super key on arch linux doesn't set isMod.

also, would you mind adding comments about what those key code values are? i don't know them off the top of my head, haha.

@mossheim mossheim added comp: Qt GUI sclang Qt components -- for IDE tickets, use "env: SCIDE" instead enhancement labels May 16, 2020
@davidgranstrom
Copy link
Contributor Author

@brianlheim Thanks for the review, and good catch about the duplicate bindings for macOS! I've pushed a new commit in which the platform is considered. I have only been able to test on macOS so far, but I'm hoping to try it out on a windows machine soon.

On all platforms modifier +/= will zoom in the content as the code is structured now. When pressing Cmd on macOS + and = give the same keycode regardless of the Shift modifier. This seem to be the case for zooming in many native apps as well, but I'm not sure if that's the case on linux/windows?

Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

just tested this on my Arch linux machine. it is not quite right. here is what would match up with the IDE's help browser behavior on my system:

zoom in: key 43. currently Ctrl-= (key 61) also triggers this, which i don't want. if that isn't what you see on macOS you will need to figure out a way to distinguish + and = for that platform.
zoom out: key 45 (correct)
zoom reset: key 48 (correct)
find: key 70 (correct)

also, you should use exact matching for modifiers; on my system Ctrl+Alt+_ also triggers these which is not right.

make sure you note down what you see when you test on Windows.

i should also note that although i'm reviewing this i'm not too confident this is the right way to do it. i wasn't expecting so many differences between platforms, and it's possible that we're totally missing internationalization issues with different languages/keyboard layouts. but, this seems like a strict improvement over what we had before at least so i'm happy with how it's going so far.

@dyfer
Copy link
Member

dyfer commented May 21, 2020

It seems like QT has some definitions for shortcuts like these: https://doc.qt.io/qt-5/qkeysequence.html#StandardKey-enum

@paum3
Copy link
Contributor

paum3 commented May 21, 2020

hi, sorry if post on wrong place, but It could also accept vim-like up/down keys 'j' for down, 'k' for up.

@davidgranstrom
Copy link
Contributor Author

@brianlheim Yes, agreed that this was a bit trickier than expected to get right! I think the suggestion @dyfer posted might be the correct solution, but I'm unsure how that could be exposed to sclang?

I pushed an update which uses exact modifiers, will try on windows tomorrow and report back.

@davidgranstrom
Copy link
Contributor Author

Just tested on windows, it looks correct:

zoom in: key 43. (+)
zoom out: key 45 (-)
zoom reset: key 48 (0)
find: key 70 (f)

Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

thanks!

@mossheim mossheim merged commit 00476b2 into supercollider:develop Jun 9, 2020
@davidgranstrom davidgranstrom deleted the topic/helpbrowser-resize branch June 11, 2020 13:53
@patrickdupuis patrickdupuis mentioned this pull request Jul 6, 2020
4 tasks
mossheim added a commit that referenced this pull request Jul 7, 2020
@patrickdupuis patrickdupuis moved this from in progress to DONE in Patch release cherry-picks Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: Qt GUI sclang Qt components -- for IDE tickets, use "env: SCIDE" instead enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants