Fix HelpBrowser key shortcuts#7542
Conversation
e5e2041 to
9c16f4d
Compare
Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp update HelpBrowser.sc and HelpBrowser.schelp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp Update help_browser.cpp
f0bf977 to
14394a3
Compare
|
With Update overriding_action.hpp, “Ctrl + =” and “Ctrl + +” enlarge the font size on both Ubuntu and Windows. I will try blocking “Cmd + +” in the HelpBrowser so that it no longer enlarges the SC‑IDE code editor font size. |
|
I am giving up on resolving the issue where For |
dyfer
left a comment
There was a problem hiding this comment.
I don't know about the Qt changes, just caught a couple of cosmetic ones.
capital-G
left a comment
There was a problem hiding this comment.
Adjust font size (both inside and outside the code editor)
IMO zooming on the help browser should be independent of the editor, since the help browser also has text which the text editor can not display (i.e. non-monospaced text). I may want to therefore change the layout w/o changing my own written code.
|
|
||
| mActions[ZoomIn]->setShortcut(settings->shortcut("editor-enlarge-font")); | ||
| QList<QKeySequence> zoomInShortcuts; | ||
| zoomInShortcuts.append(settings->shortcut("editor-enlarge-font")); |
There was a problem hiding this comment.
IMO pressing Cmd + on the help browser shouldn't affect the editor font? I would it expect to zoom in on the layout of the help browser?
There was a problem hiding this comment.
I’d also like to hear your thoughts on this. To clarify the current situation: the behaviour is already correct on Windows and Linux. This inconsistent behaviour only occurs on macOS, and it is not introduced by this PR.
To avoid confusion, let's define the terms:
- SC‑IDE Code Editor → the main code editor in SC‑IDE
- HelpBrowser codeMirrorContainer → the
<div class="codeMirrorContainer">inside the HelpBrowser HTML
Here is how macOS behaves across different versions:
SC 3.14 to current (without this PR)
When clicking outside codeMirrorContainer:
Cmd +→ increases font size in the SC‑IDE Code Editor -Cmd =→ increases font size in the HelpBrowser
When clicking inside codeMirrorContainer:
Cmd +→ increases font size in the SC‑IDE Code Editor -Cmd =→ does nothing
SC 3.13 (and earlier) / With this PR applied
When clicking outside codeMirrorContainer:
Cmd +→ increases font size in the SC‑IDE Code EditorCmd =→ increases font size in the HelpBrowser
When clicking inside codeMirrorContainer:
Cmd +→ increases font size in the SC‑IDE Code Editor -Cmd =→ increases font size in the HelpBrowser
Summary
The issue you pointed out (Cmd + enlarges SC-IDE code editor font) is an older, pre-existing macOS‑specific problem. This PR simply fixes the regression introduced in SC 3.14 and restores the expected behaviour from SC 3.13 (where Cmd = at least works consistently inside the HelpBrowser codeMirrorContainer).
I did initially spend several hours investigating how to prevent Cmd + in the HelpBrowser from expanding the code editor text—not only in this PR, but also during my work on #7534 and #7521. However, after exploring various approaches, I realised that a proper fix requires a much deeper architectural change. Therefore, I have left it out of scope for this PR so we can focus on resolving the 3.14 regression first.
There was a problem hiding this comment.
This summary looks a bit too much ai assisted to me, not a fan of that^^
This PR simply fixes the regression
I'd disagree w/ that - I wouldn't want to have the zoom in shortcut performed on the browser to affect the code editor - I'd argue that the previous behavior is wrong as well, and instead of removing this wiring, you are introducing it here explicitly.
There was a problem hiding this comment.
I hope this doesn’t come across the wrong way. I just want to share my thoughts openly so we can clear up any misunderstandings and move forward effectively.
I'd disagree w/ that - I wouldn't want to have the zoom in shortcut performed on the browser to affect the code editor - I'd argue that the previous behavior is wrong as well, and instead of removing this wiring, you are introducing it here explicitly.
I think there might be a slight misunderstanding regarding my intention and what this PR actually does.
Let me try to clarify.
I'd argue that the previous behavior is wrong I'd argue that the previous behavior is wrong as well, and instead of removing this wiring, you are introducing it here explicitly.
If you mean I have not yet succeeded in removing the connection between the browser zoom and the editor zoom, then you are entirely right. That is exactly the limitation I was trying to explain.
However, if the meaning is one or all of the following, then that’s a misunderstanding:
- I think the previous behaviour (pressing
"cmd" + "shift" + "="in HelpBrowser enlarges the font size inSC-IDE Code Editor) is correct. - The issue that pressing
"cmd" + "shift" + "="in HelpBrowser enlarges the font size inSC-IDE Code Editorwas resolved in 3.14 or in 3.15 before Fix keyboard shortcuts and font reset behavior in HelpBrowser #7534. I am reintroducing the wrong behaviour in this PR.
To be absolutely clear:
- I agree with you 100% that pressing
"cmd" + "shift" + "="in theHelpBrowsershould not affect theSC-IDE Code Editor. It is wrong behaviour. - I did not introduce this wiring. This problem has existed long before this PR (in 3.13, 3.14, and likely earlier).
- I spent hours trying to completely remove this coupling, but as I mentioned, it seems to require a much deeper architectural change that is currently beyond my skill level. That is why I suggested treating it as a separate, out-of-scope issue rather than letting it block the other fixes here.
This summary looks a bit too much ai assisted to me, not a fan of that^^
I did use AI to help soften my phrasing and clean up my English (e.g., it suggested the phrase "a proper fix requires a much deeper architectural change" and it removed a sentence including "regression" which I then rewrote). However, it is strongly based on my own writing.
… purpose The previous implementation used a function-local static variable within HelpBrowser::onPageLoad() to store the Ctrl+R blocker action on Windows. While functional, this approach obscured the variable's scope and lifetime. This patch moves the variable into the HelpBrowser class as a private static member, making its role explicit. Additionally, a brief comment has been added to explain why this blocking action is necessary (to prevent the Ctrl+R shortcut from propagating to the parent SC-IDE editor). Ref: supercollider#7542 (comment)
|
I’ve spent an unexpectedly long time (including today, yesterday, and several earlier attempts) looking deeply into the keyboard‑shortcut issue in the HelpBrowser. Unfortunately, handling Since the |
There was a problem hiding this comment.
I must admit that the code didn't get better since the last review - mainly b/c the code is now much more platform dependent, which is always good to be avoided if possible.
Some comments haven't been addressed, such as the meta key. But while performing a small google search I came across that one https://doc.qt.io/archives/qt-5.15/qkeysequence.html#standard-shortcuts - maybe this could be relevant?
Alternatively - why don't we use the shortcut as defined in the editor? This seems cleanest to me overall
I think a good implementation could be to block these shortcuts from bubbling up (see review comment about eventfilter) and wire them up to the respective action on the helpbrowser widget. No platform dependent code necessary anymore, which is the most preferred solution, and the user can re-assign this and it becomes consistent across help browser and code editor.
This is probably not more than 20 loc?
| // Blocks the default `Ctrl + R` behavior on Windows, which would otherwise | ||
| // trigger an unwanted refresh action in the embedded browser widget. | ||
| static QAction* winCtrlRBlocker; |
There was a problem hiding this comment.
If you want to stop key events from bubbling up, you should use this pattern
supercollider/editors/sc-ide/widgets/lookup_dialog.cpp
Lines 202 to 215 in 926827c
You can read the docs about this at https://doc.qt.io/qt-6/eventsandfilters.html#event-filters
There was a problem hiding this comment.
I could not configure it. I left this.
|
|
||
| mActions[ZoomIn]->setShortcut(settings->shortcut("editor-enlarge-font")); | ||
| QList<QKeySequence> zoomInShortcuts; | ||
| zoomInShortcuts.append(settings->shortcut("editor-enlarge-font")); |
There was a problem hiding this comment.
This summary looks a bit too much ai assisted to me, not a fan of that^^
This PR simply fixes the regression
I'd disagree w/ that - I wouldn't want to have the zoom in shortcut performed on the browser to affect the code editor - I'd argue that the previous behavior is wrong as well, and instead of removing this wiring, you are introducing it here explicitly.
|
Regarding regressions, here is why I consider some behaviours in 3.14/3.15-dev to be a regression compared to 3.13 when interacting inside the
My goal with this PR is simply to restore the working 3.13 behaviors and fix the critical regressions so that the |
Update help_browser.cpp
1c2b399 to
7423e60
Compare
|
Current state of this PR:
The non-fixed issues are outside the scope of this PR due to my current implementation limitations:
If these two issues need to be resolved within this PR, I will need assistance from the coauthors. |
|
I think it is better to convert this PR to a draft. If someone later finds that this PR can be revived, I would be glad for them to continue. It is also completely fine with me if someone opens a new PR to address the issues discussed here and in #7539. What I have learned from this PR is that using AI in unfamiliar fields can be convenient, but also quite risky. My mistakes in this PR:
|
| mActions[ZoomIn]->setShortcuts(zoomInShortcuts); | ||
| mActions[ZoomOut]->setShortcut(settings->shortcut("editor-shrink-font")); |
There was a problem hiding this comment.
Have you tried "plain" QKeySequence::ZoomIn and QKeySequence::ZoomOut.
At least it seems off/inconsistent to use the user-provided shortcut for zoom out and some hardcoded values for zooming in ;)
I suggested to use the shortcuts from the editor, but since we are also not using them for refresh etc, it is maybe not bad to use the Qt provided ones?
If you are still interested in the PR, you could give that a try and see how it works out :)
from my experience: best results w/ AI are achieved when guiding it rather tight - at least if you care about understanding/maintainability of the code, which we care for here ;)
Reviewing is also a task ;) Certainly not as fun as coding (IMO), but really necessary and w/ AI this becomes even more important.
No need to apologize here at all! If you learned something along the way, thats fine. I certainly learned something as well :)
I don't think you do - C++ has some idiosyncrasies, but you can learn those as you go - changes such are these are actually quite manageable IMO - you certainly need to learn something new, but thats the fun part ;) The code looks much better r/n, and I think some open questions aren't too hard ;) Regarding the open problems, I took a closer look at this:
supercollider/QtCollider/widgets/QcWebView.cpp Lines 59 to 63 in 90f5676 See https://doc.qt.io/qt-6/qkeysequence.html#standard-shortcuts for how these translate to.
So when I now press |
For the current state of this PR, see #7542 (comment).
closes: #7539
Modifying
HelpBrowser.cppandoverriding_action.hppin this PR was assisted by some AIs, and I reviewed it based on the experience from #7534.Purpose and Motivation
This PR fixes most of the inconsistent behaviours described in #7539.
Cmd/Ctrl + R
Cmd/Ctrl + =, Cmd/Ctrl + 0, Cmd/Ctrl + -
Additionally, navigating to the previous and next pages now works consistently across all platforms, both inside and outside the code editor.
Types of changes
To‑do list