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.

Sign up for GitHub

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

Jump to bottom

Add keydown actions to resize the HelpBrowser #4932

Merged
merged 4 commits into from
Jun 9, 2020
Merged

Add keydown actions to resize the HelpBrowser #4932

merged 4 commits into from
Jun 9, 2020

Conversation

davidgranstrom
Copy link
Contributor

@davidgranstrom davidgranstrom commented May 16, 2020 •
edited
Loading

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 davidgranstrom force-pushed the topic/helpbrowser-resize branch from 876d069 to 243f76e Compare May 17, 2020 13:15
@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?

@davidgranstrom davidgranstrom force-pushed the topic/helpbrowser-resize branch from 243f76e to bb65b2f Compare May 17, 2020 13:27
mossheim
mossheim suggested changes May 17, 2020
View reviewed changes
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 davidgranstrom force-pushed the topic/helpbrowser-resize branch from bb65b2f to 0f95832 Compare June 2, 2020 15:43
@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)

mossheim
mossheim approved these changes Jun 9, 2020
View reviewed changes
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
Merged
4 tasks
mossheim added a commit that referenced this pull request Jul 7, 2020
@mossheim
Merge pull request #5060 from patrickdupuis/cherry-pick4932
d3db971 
Cherry-pick #4932 into 3.11
@davidgranstrom davidgranstrom mentioned this pull request Jul 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mossheim mossheim mossheim approved these changes

Assignees
No one assigned
Labels
comp: Qt GUI sclang Qt components -- for IDE tickets, use "env: SCIDE" instead enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidgranstrom @mossheim @dyfer @paum3