This PR adds two options to control scrollwheel behavior.
* `scrollwheel_lines`: How many lines to scroll. * `scrollwheel_zoom_disable`: To disable zooming by control + scrollwheel.
Control of vertical scrolling is taken away from Scintilla to provide consistent behavior when the control key is or is not pressed. Lines option added because I don't see a way to get or set the value used by Scintilla.
Supersedes #2954 You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3835
-- Commit Summary --
* Add option to block scroll-wheel zooming * Add option to change number of scroll lines
-- File Changes --
M doc/geany.txt (3) M src/editor.c (18) M src/editor.h (2) M src/keyfile.c (4) M src/sciwrappers.c (6) M src/sciwrappers.h (1)
-- Patch Links --
https://github.com/geany/geany/pull/3835.patch https://github.com/geany/geany/pull/3835.diff
@b4n requested changes on this pull request.
@@ -2681,6 +2681,9 @@ indent_hard_tab_width The size of a tab character. Don't change 8
editor_ime_interaction Input method editor (IME)'s candidate 0 to new window behaviour. May be 0 (windowed) or documents 1 (inline) +scrollwheel_lines Number of lines the scrollwheel scrolls 3 immediately
First: why? Second: this is NOT the current value (which is 4, or a system value). Also, as suggested in the previous sentence, Scintilla's default can depend on the system configuration (currently, it does on Windows), so it's not necessarily a great idea to force another value, at least not with a good rationale. And, why change it from 4 to 3? or is the Windows's default 3? :)
sci_scroll_columns(editor->sci, amount);
return TRUE; } + else if (!event->state || (editor_prefs.scrollwheel_zoom_disable && (event->state & GDK_CONTROL_MASK)))
This doesn't actually do what you think it does (in some situations). `event->state` contains all modifier states, including e.g. <kbd>NumLock</kbd> and others, so in the non-disabled case it'll fallback to Scintilla's handling if e.g. <kbd>NumLock</kbd> is on, and your code if it's not. The proper solution is to filter through `keybindings_get_modifiers()`:
```c GdkModifierType modifiers = keybindings_get_modifiers(event->state);
if (modifiers & GDK_MOD1_MASK) … else if (modifiers & … ```
sci_scroll_columns(editor->sci, amount);
return TRUE; } + else if (!event->state || (editor_prefs.scrollwheel_zoom_disable && (event->state & GDK_CONTROL_MASK))) + { + /* scroll normally */ + gint lines = editor_prefs.scrollwheel_lines; + gint amount = (event->direction == GDK_SCROLL_DOWN) ? lines : (-1 * lines);
```suggestion gint amount = (event->direction == GDK_SCROLL_DOWN) ? lines : -lines; ```
+void sci_scroll_lines(ScintillaObject *sci, gint lines)
+{ + SSM(sci, SCI_LINESCROLL, 0, (uptr_t) lines); +} + +
I don't think it's worth adding a new Scintilla wrapper function for the single caller, given the call is pretty simple. Just inline the equivalent in the calling code.
@@ -2681,6 +2681,9 @@ indent_hard_tab_width The size of a tab character. Don't change 8
editor_ime_interaction Input method editor (IME)'s candidate 0 to new window behaviour. May be 0 (windowed) or documents 1 (inline) +scrollwheel_lines Number of lines the scrollwheel scrolls 3 immediately
I guess I see the reason why you added this: because you need a value when transforming <kbd>Ctrl+Scroll</kbd> to a "regular" scroll event, right? And then instead of hard-coding it, you added a setting. I guess that's a fair reason, but there's sill the issues mentioned above. And is it important that <kbd>Ctrl+Scroll</kbd> scrolls, or could it just do nothing?
At any rate, at least the default should be the same as the current one. I don't care as much for Windows I admit, but it's probably easy enough to mimic what Scintilla does there as well, isn't it? The semantic of the option becomes a bit hairy though, because how to conceal a dynamic system value with a user setting? I guess it could be `0` means use system default (and fallback to 4), and any other value this amount of lines or something like that…
Why is it transforming ctrl+scroll to scroll differently (as @b4n pointed out on the code) instead of doing nothing when `scrollwheel_zoom_disable` is set?
On #2954 you say:
I have the editor font sent to a comfortable size. Many shortcuts consist of combining the Control key with others. And of course, the touchpad is used for moving the cursor. So very frequently (multiple times per day), I accidentally engage the scrolling function of the touchpad while the control key is pressed. This causes the editor view to zoom, which is disruptive.
Simply disabling the ctrl+zoom will achieve your aim and won't unexpectedly scroll the view either.
@xiota commented on this pull request.
+void sci_scroll_lines(ScintillaObject *sci, gint lines)
+{ + SSM(sci, SCI_LINESCROLL, 0, (uptr_t) lines); +} + +
Same could be said of...
https://github.com/geany/geany/blob/a44df8bb570e5d43f882649826e5d34ca3907a63...
@xiota commented on this pull request.
@@ -2681,6 +2681,9 @@ indent_hard_tab_width The size of a tab character. Don't change 8
editor_ime_interaction Input method editor (IME)'s candidate 0 to new window behaviour. May be 0 (windowed) or documents 1 (inline) +scrollwheel_lines Number of lines the scrollwheel scrolls 3 immediately
I miscounted and the historical value when mouse wheels were introduced to consumers was 3. So I didn't bother recounting to confirm. Would be better to use the system default, but I don't know how.
@elextr commented on this pull request.
+void sci_scroll_lines(ScintillaObject *sci, gint lines)
+{ + SSM(sci, SCI_LINESCROLL, 0, (uptr_t) lines); +} + +
Disagree with @b4n, all Scintilla interfacing should go through wrappers so that changes Scintilla makes can be abstracted away, for example the recent string length change.
The performance difference will be immaterial unless the Scintilla call is being hammered in a loop (which it never is AFAICT). And if it _is_ a problem anywhere, define the function `inline` in `sciwrappers.h` (C99).
Why is it transforming ctrl+scroll to scroll differently ... instead of doing nothing ...?
The mouse wheel would look broken if scrolling stopped when control is accidentally pressed.
@xiota commented on this pull request.
sci_scroll_columns(editor->sci, amount);
return TRUE; } + else if (!event->state || (editor_prefs.scrollwheel_zoom_disable && (event->state & GDK_CONTROL_MASK)))
Some existing code will need to be revised then.
The mouse wheel would look broken if scrolling stopped when control is accidentally pressed.
Hmm, ok, in that case is it safe for us to simply edit the event to remove the control mask then let it go to Scintilla, maybe @b4n knows, or you could try it.
@b4n commented on this pull request.
sci_scroll_columns(editor->sci, amount);
return TRUE; } + else if (!event->state || (editor_prefs.scrollwheel_zoom_disable && (event->state & GDK_CONTROL_MASK)))
Possibly, did you see some? This very function was fine because it only checked whether some modifiers were present, not that they were specific -- and the ones checked weren't in the special cases for macos -- but I can totally imagine we have issues some other places (although I'd think they'd be discovered by now -- your case was a lot more subtle to see given that if your test failed the behavior was still very similar)
@b4n commented on this pull request.
+void sci_scroll_lines(ScintillaObject *sci, gint lines)
+{ + SSM(sci, SCI_LINESCROLL, 0, (uptr_t) lines); +} + +
@xiota sure there might be some already very seldom used, not sure it's a trend to follow… But if @elextr has a strong opinion, fine by me. FWIW, IMO it doesn't make much sense where it's a single cas that's just as easy to fix in the caller or refactor out when there are more users.
@b4n commented on this pull request.
+void sci_scroll_lines(ScintillaObject *sci, gint lines)
+{ + SSM(sci, SCI_LINESCROLL, 0, (uptr_t) lines); +} + +
@elextr I'm not concerned by any speed issues, just that although it looks "clean", it also makes the code somewhat more complex, adding a tiny bit of an additional constraints through new interfaces just to transform that interface (the wrapper doesn't have any logic of its own)
The mouse wheel would look broken if scrolling stopped when control is accidentally pressed.
I'd argue that it's just as true with any incorrect usage that doesn't do what you wanted. For a more unreasonable example, if I hit <kbd>Del</kbd> when I meant to write "hello", I wouldn't blame the software but myself.
I get that it's annoying the zoom changes by mistake, but is it truly a problem for you to re-scroll without control pressed?
@elextr hum… sounds hacky, but might happen to work, I don't know.
@xiota commented on this pull request.
sci_scroll_columns(editor->sci, amount);
return TRUE; } + else if (!event->state || (editor_prefs.scrollwheel_zoom_disable && (event->state & GDK_CONTROL_MASK)))
I didn't understand the purpose earlier. It makes more sense now that I see it's to work around how Macs interpret some keys.
@xiota commented on this pull request.
sci_scroll_columns(editor->sci, amount);
return TRUE; } + else if (!event->state || (editor_prefs.scrollwheel_zoom_disable && (event->state & GDK_CONTROL_MASK)))
If I want to remove the control mask, would either of the following work for mac? ``` event->state = event->state & !GDK_CONTROL_MASK; ```
``` GdkModifierType modifiers = keybindings_get_modifiers(event->state); event->state = modifiers & !GDK_CONTROL_MASK; ```
hum… sounds hacky, but might happen to work, I don't know.
Totally hacky :-). But if it works it keeps the scrolling under Scintilla's control and IIUC Scintilla makes attempts to match the environment scrolling speed.
@elextr commented on this pull request.
+void sci_scroll_lines(ScintillaObject *sci, gint lines)
+{ + SSM(sci, SCI_LINESCROLL, 0, (uptr_t) lines); +} + +
@b4n I am normally the one complaining about single use functions where a lump of functionality is broken out for no good reason, and that case totally matches your reasoning.
But for APIs wrapping is a useful way of getting encapsulation and hiding, and the Geany plugin API should have been done that way, but well, that ship has sailed, hit stormy seas, and sunk ;-D
Probably doesn't matter at this stage which this individual call uses but every little drop of water erodes the stone [proverb].
@b4n commented on this pull request.
sci_scroll_columns(editor->sci, amount);
return TRUE; } + else if (!event->state || (editor_prefs.scrollwheel_zoom_disable && (event->state & GDK_CONTROL_MASK)))
It's also to filter out uninteresting modifiers like NumLock.
For removing, you could try `event->state &= ~GDK_CONTROL_MASK;`, or if inside the `if (event->state & GDK_CONTOL_MASK)`, just `event->state ^= GDK_CONTROL_MASK;`.
@xiota pushed 1 commit.
33e4ade264f5f09f3c886c60a08eb263847159eb Change filtering algorithm, add more options
Changed the options and how they interact because I wanted to see what it would be like.
* scrollwheel_lines (4): Number of lines for normal scrolling. Still don't know how to get the system default.
* scrollwheel_alt_factor (6): Multiplier when alt is pressed, instead of pgup/pgdown. This is like a scrolling accelerator. Users can set it to scroll less than a full page so that context from jump to jump is still visible.
* scrollwheel_ctrl_lines (4): Number of lines to scroll when control is pressed, when it doesn't activate zooming. Fine scrolling is easier if set to a low number, like 1.
* scrollwheel_ctrl_zoom (T): Whether ctrl+scroll = zoom.
github-comments@lists.geany.org